13 Dec, 2018

1 commit

  • [ Upstream commit d7d8bbb40a5b1f682ee6589e212934f4c6b8ad60 ]

    The complete size ("total_size") of the fragmented packet is stored in the
    fragment header and in the size of the fragment chain. When the fragments
    are ready for merge, the skbuff's tail of the first fragment is expanded to
    have enough room after the data pointer for at least total_size. This means
    that it gets expanded by total_size - first_skb->len.

    But this is ignoring the fact that after expanding the buffer, the fragment
    header is pulled by from this buffer. Assuming that the tailroom of the
    buffer was already 0, the buffer after the data pointer of the skbuff is
    now only total_size - len(fragment_header) large. When the merge function
    is then processing the remaining fragments, the code to copy the data over
    to the merged skbuff will cause an skb_over_panic when it tries to actually
    put enough data to fill the total_size bytes of the packet.

    The size of the skb_pull must therefore also be taken into account when the
    buffer's tailroom is expanded.

    Fixes: 610bfc6bc99b ("batman-adv: Receive fragmented packets and merge")
    Reported-by: Martin Weinelt
    Co-authored-by: Linus Lüssing
    Signed-off-by: Sven Eckelmann
    Signed-off-by: Simon Wunderlich
    Signed-off-by: Sasha Levin

    Sven Eckelmann
     

30 May, 2018

1 commit

  • [ Upstream commit 3bf2a09da956b43ecfaa630a2ef9a477f991a46a ]

    A more sophisticated implementation could try to combine fragment checksums
    when all fragments have CHECKSUM_COMPLETE and are split at even offsets.
    For now, we just set ip_summed to CHECKSUM_NONE to avoid "hw csum failure"
    warnings in the kernel log when fragmented frames are received. In
    consequence, skb_pull_rcsum() can be replaced with skb_pull().

    Note that in usual setups, packets don't reach batman-adv with
    CHECKSUM_COMPLETE (I assume NICs bail out of checksumming when they see
    batadv's ethtype?), which is why the log messages do not occur on every
    system using batman-adv. I could reproduce this issue by stacking
    batman-adv on top of a VXLAN interface.

    Fixes: 610bfc6bc99b ("batman-adv: Receive fragmented packets and merge")
    Tested-by: Maximilian Wilhelm
    Signed-off-by: Matthias Schiffer
    Signed-off-by: Sven Eckelmann
    Signed-off-by: Simon Wunderlich
    Signed-off-by: Sasha Levin
    Signed-off-by: Greg Kroah-Hartman

    Matthias Schiffer
     

16 Jun, 2017

1 commit

  • A common pattern with skb_put() is to just want to memcpy()
    some data into the new space, introduce skb_put_data() for
    this.

    An spatch similar to the one for skb_put_zero() converts many
    of the places using it:

    @@
    identifier p, p2;
    expression len, skb, data;
    type t, t2;
    @@
    (
    -p = skb_put(skb, len);
    +p = skb_put_data(skb, data, len);
    |
    -p = (t)skb_put(skb, len);
    +p = skb_put_data(skb, data, len);
    )
    (
    p2 = (t2)p;
    -memcpy(p2, data, len);
    |
    -memcpy(p, data, len);
    )

    @@
    type t, t2;
    identifier p, p2;
    expression skb, data;
    @@
    t *p;
    ...
    (
    -p = skb_put(skb, sizeof(t));
    +p = skb_put_data(skb, data, sizeof(t));
    |
    -p = (t *)skb_put(skb, sizeof(t));
    +p = skb_put_data(skb, data, sizeof(t));
    )
    (
    p2 = (t2)p;
    -memcpy(p2, data, sizeof(*p));
    |
    -memcpy(p, data, sizeof(*p));
    )

    @@
    expression skb, len, data;
    @@
    -memcpy(skb_put(skb, len), data, len);
    +skb_put_data(skb, data, len);

    (again, manually post-processed to retain some comments)

    Reviewed-by: Stephen Hemminger
    Signed-off-by: Johannes Berg
    Signed-off-by: David S. Miller

    Johannes Berg
     

17 Mar, 2017

1 commit


05 Mar, 2017

1 commit

  • The batman-adv fragmentation packets have the design problem that they
    cannot be refragmented and cannot handle padding by the underlying link.
    The latter often leads to problems when networks are incorrectly configured
    and don't use a common MTU.

    The sender could for example fragment a 1271 byte frame (plus external
    ethernet header (14) and batadv unicast header (10)) to fit in a 1280 bytes
    large MTU of the underlying link (max. 1294 byte frames). This would create
    a 1294 bytes large frame (fragment 2) and a 55 bytes large frame
    (fragment 1). The extra 54 bytes are the fragment header (20) added to each
    fragment and the external ethernet header (14) for the second fragment.

    Let us assume that the next hop is then not able to transport 1294 bytes to
    its next hop. The 1294 byte large frame will be dropped but the 55 bytes
    large fragment will still be forwarded to its destination.

    Or let us assume that the underlying hardware requires that each frame has
    a minimum size (e.g. 60 bytes). Then it will pad the 55 bytes frame to 60
    bytes. The receiver of the 60 bytes frame will no longer be able to
    correctly assemble the two frames together because it is not aware that 5
    bytes of the 60 bytes frame are padding and don't belong to the reassembled
    frame.

    This can partly be avoided by splitting frames more equally. In this
    example, the 675 and 674 bytes large fragment frames could both potentially
    reach its destination without being too large or too small.

    Reported-by: Martin Weinelt
    Fixes: ee75ed88879a ("batman-adv: Fragment and send skbs larger than mtu")
    Signed-off-by: Sven Eckelmann
    Acked-by: Linus Lüssing
    Signed-off-by: Simon Wunderlich

    Sven Eckelmann
     

03 Mar, 2017

1 commit


22 Feb, 2017

2 commits

  • Trying to split and transmit a unicast packet in 16 parts will fail for
    the final fragment: After having sent the 15th one with a frag_packet.no
    index of 14, we will increase the the index to 15 - and return with an
    error code immediately, even though one more fragment is due for
    transmission and allowed.

    Fixing this issue by moving the check before incrementing the index.

    While at it, adding an unlikely(), because the check is actually more of
    an assertion.

    Fixes: ee75ed88879a ("batman-adv: Fragment and send skbs larger than mtu")
    Signed-off-by: Linus Lüssing
    Signed-off-by: Sven Eckelmann
    Signed-off-by: Simon Wunderlich

    Linus Lüssing
     
  • The function batadv_frag_skb_buffer was supposed not to consume the skbuff
    on errors. This was followed in the helper function
    batadv_frag_insert_packet when the skb would potentially be inserted in the
    fragment queue. But it could happen that the next helper function
    batadv_frag_merge_packets would try to merge the fragments and fail. This
    results in a kfree_skb of all the enqueued fragments (including the just
    inserted one). batadv_recv_frag_packet would detect the error in
    batadv_frag_skb_buffer and try to free the skb again.

    The behavior of batadv_frag_skb_buffer (and its helper
    batadv_frag_insert_packet) must therefore be changed to always consume the
    skbuff to have a common behavior and avoid the double kfree_skb.

    Fixes: 610bfc6bc99b ("batman-adv: Receive fragmented packets and merge")
    Signed-off-by: Sven Eckelmann
    Signed-off-by: Simon Wunderlich

    Sven Eckelmann
     

28 Jan, 2017

1 commit


26 Jan, 2017

1 commit


04 Jan, 2017

1 commit


30 Oct, 2016

2 commits


19 Oct, 2016

2 commits


04 Jul, 2016

1 commit


30 Jun, 2016

2 commits

  • Unfragmented frames which traverse a node have their skb->priority set
    by looking at the IP ToS byte, or the 802.1p header. However for
    fragments this is not possible, only one of the fragments will contain
    the headers. Instead, place the priority into the fragment header and
    on receiving a fragment, use this information to set the skb->priority
    for when the fragment is forwarded.

    Signed-off-by: Andrew Lunn
    Signed-off-by: Marek Lindner
    Signed-off-by: Sven Eckelmann
    Signed-off-by: Simon Wunderlich

    Andrew Lunn
     
  • BATMAN will set the skb->priority based on the IP precedence or 802.1q
    tag. However, if it needs to fragment the frame, it currently leaves
    the fragment skb with the default priority and actually overwrites the
    priority in the unfragmented frame. Fix this.

    Signed-off-by: Andrew Lunn
    Signed-off-by: Marek Lindner
    Signed-off-by: Sven Eckelmann
    Signed-off-by: Simon Wunderlich

    Andrew Lunn
     

04 May, 2016

1 commit


29 Feb, 2016

1 commit

  • To enable ELP to send probing packets over wireless links
    only if needed, batman-adv must keep track of the last time
    it sent a unicast packet towards every neighbour.

    For this purpose a 2 main changes are introduced:
    1) a new member of the elp_neigh_node structure stores the
    last time a unicast packet was sent towards this neighbour;
    2) a wrapper function for sending unicast packets is
    implemented. This function will simply update the member
    describe din point 1) and then forward the packet to the
    real sending routine.

    Point 2) implies that any code-path leading to a unicast
    sending now has to use the new wrapper.

    Signed-off-by: Antonio Quartulli
    Signed-off-by: Marek Lindner

    Antonio Quartulli
     

23 Feb, 2016

3 commits


02 Feb, 2016

3 commits


16 Dec, 2015

1 commit

  • The chain pointer was already created in batadv_frag_purge_orig to make the
    checks more readable. Just use the chain pointer everywhere instead of
    having the same dereference + array access in the most lines of this
    function.

    Signed-off-by: Sven Eckelmann
    Acked-by: Martin Hundebøll
    Signed-off-by: Marek Lindner
    Signed-off-by: Antonio Quartulli

    Sven Eckelmann
     

25 Aug, 2015

2 commits


07 Jun, 2015

1 commit

  • The header files could not be build indepdent from each other. This is
    happened because headers didn't include the files for things they've used.
    This was problematic because the success of a build depended on the
    knowledge about the right order of local includes.

    Also source files were not including everything they've used explicitly.
    Instead they required that transitive includes are always stable. This is
    problematic because some transitive includes are not obvious, depend on
    config settings and may not be stable in the future.

    The order for include blocks are:

    * primary headers (main.h and the *.h file of a *.c file)
    * global linux headers
    * required local headers
    * extra forward declarations for pointers in function/struct declarations

    The only exceptions are linux/bitops.h and linux/if_ether.h in packet.h.
    This header file is shared with userspace applications like batctl and must
    therefore build together with userspace applications. The header
    linux/bitops.h is not part of the uapi headers and linux/if_ether.h
    conflicts with the musl implementation of netinet/if_ether.h. The
    maintainers rejected the use of __KERNEL__ preprocessor checks and thus
    these two headers are only in main.h. All files using packet.h first have
    to include main.h to work correctly.

    Reported-by: Markus Pargmann
    Signed-off-by: Sven Eckelmann
    Signed-off-by: Marek Lindner

    Sven Eckelmann
     

29 May, 2015

3 commits

  • The fragment queueing code now validates the total_size of each fragment,
    checks when enough fragments are queued to allow to merge them into a
    single packet and if the fragments have the correct size. Therefore, it is
    not required to have any other parameter for the merging function than a
    list of queued fragments.

    This change should avoid problems like in the past when the different skb
    from the list and the function parameter were mixed incorrectly.

    Signed-off-by: Sven Eckelmann
    Acked-by: Martin Hundebøll
    Signed-off-by: Marek Lindner

    Sven Eckelmann
     
  • The fragmentation code was replaced in
    610bfc6bc99bc83680d190ebc69359a05fc7f605 ("batman-adv: Receive fragmented
    packets and merge") by an implementation which handles the queueing+merging
    of fragments based on their size and the total_size of the non-fragmented
    packet. This total_size is announced by each fragment. The new
    implementation doesn't check if the the total_size information of the
    packets inside one chain is consistent.

    This is consistency check is recommended to allow using any of the packets
    in the queue to decide whether all fragments of a packet are received or
    not.

    Signed-off-by: Sven Eckelmann
    Acked-by: Martin Hundebøll
    Signed-off-by: Marek Lindner

    Sven Eckelmann
     
  • Signed-off-by: Sven Eckelmann
    Signed-off-by: Marek Lindner

    Sven Eckelmann
     

08 Jan, 2015

1 commit


24 Dec, 2014

2 commits

  • The fragmentation code was replaced in 610bfc6bc99bc83680d190ebc69359a05fc7f605
    ("batman-adv: Receive fragmented packets and merge") by an implementation which
    can handle up to 16 fragments of a packet. The packet is prepared for the split
    in fragments by the function batadv_frag_send_packet and the actual split is
    done by batadv_frag_create.

    Both functions calculate the size of a fragment themself. But their calculation
    differs because batadv_frag_send_packet also subtracts ETH_HLEN. Therefore,
    the check in batadv_frag_send_packet "can a full fragment can be created?" may
    return true even when batadv_frag_create cannot create a full fragment.

    The function batadv_frag_create doesn't check the size of the skb before
    splitting it and therefore might try to create a larger fragment than the
    remaining buffer. This creates an integer underflow and an invalid len is given
    to skb_split.

    Signed-off-by: Sven Eckelmann
    Signed-off-by: David S. Miller

    Sven Eckelmann
     
  • The fragmentation code was replaced in 610bfc6bc99bc83680d190ebc69359a05fc7f605
    ("batman-adv: Receive fragmented packets and merge"). The new code provided a
    mostly unused parameter skb for the merging function. It is used inside the
    function to calculate the additionally needed skb tailroom. But instead of
    increasing its own tailroom, it is only increasing the tailroom of the first
    queued skb. This is not correct in some situations because the first queued
    entry can be a different one than the parameter.

    An observed problem was:

    1. packet with size 104, total_size 1464, fragno 1 was received
    - packet is queued
    2. packet with size 1400, total_size 1464, fragno 0 was received
    - packet is queued at the end of the list
    3. enough data was received and can be given to the merge function
    (1464 == (1400 - 20) + (104 - 20))
    - merge functions gets 1400 byte large packet as skb argument
    4. merge function gets first entry in queue (104 byte)
    - stored as skb_out
    5. merge function calculates the required extra tail as total_size - skb->len
    - pskb_expand_head tail of skb_out with 64 bytes
    6. merge function tries to squeeze the extra 1380 bytes from the second queued
    skb (1400 byte aka skb parameter) in the 64 extra tail bytes of skb_out

    Instead calculate the extra required tail bytes for skb_out also using skb_out
    instead of using the parameter skb. The skb parameter is only used to get the
    total_size from the last received packet. This is also the total_size used to
    decide that all fragments were received.

    Reported-by: Philipp Psurek
    Signed-off-by: Sven Eckelmann
    Acked-by: Martin Hundebøll
    Signed-off-by: David S. Miller

    Sven Eckelmann
     

17 Aug, 2014

1 commit

  • 1d023284c31a4e40a94d5bbcb7dbb7a35ee0bcbc ("list: fix order of arguments for
    hlist_add_after(_rcu)") was incorrectly rebased on top of
    d9124268d84a836f14a6ead54ff9d8eee4c43be5 ("batman-adv: Fix out-of-order
    fragmentation support"). The parameter order change of the rebased patch was
    not re-applied as expected. This causes a memory leak and can cause crashes
    when out-of-order packets are received. hlist_add_behind will try to access the
    uninitalized list pointers of frag_entry_new to find the previous/next entry
    and may modify/read random memory locations.

    Signed-off-by: Sven Eckelmann
    Cc: Andrew Morton
    Signed-off-by: David S. Miller

    Sven Eckelmann
     

07 Aug, 2014

1 commit

  • All other add functions for lists have the new item as first argument
    and the position where it is added as second argument. This was changed
    for no good reason in this function and makes using it unnecessary
    confusing.

    The name was changed to hlist_add_behind() to cause unconverted code to
    generate a compile error instead of using the wrong parameter order.

    [akpm@linux-foundation.org: coding-style fixes]
    Signed-off-by: Ken Helias
    Cc: "Paul E. McKenney"
    Acked-by: Jeff Kirsher [intel driver bits]
    Cc: Hugh Dickins
    Cc: Christoph Hellwig
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Ken Helias
     

05 Aug, 2014

1 commit

  • batadv_frag_insert_packet was unable to handle out-of-order packets because it
    dropped them directly. This is caused by the way the fragmentation lists is
    checked for the correct place to insert a fragmentation entry.

    The fragmentation code keeps the fragments in lists. The fragmentation entries
    are kept in descending order of sequence number. The list is traversed and each
    entry is compared with the new fragment. If the current entry has a smaller
    sequence number than the new fragment then the new one has to be inserted
    before the current entry. This ensures that the list is still in descending
    order.

    An out-of-order packet with a smaller sequence number than all entries in the
    list still has to be added to the end of the list. The used hlist has no
    information about the last entry in the list inside hlist_head and thus the
    last entry has to be calculated differently. Currently the code assumes that
    the iterator variable of hlist_for_each_entry can be used for this purpose
    after the hlist_for_each_entry finished. This is obviously wrong because the
    iterator variable is always NULL when the list was completely traversed.

    Instead the information about the last entry has to be stored in a different
    variable.

    This problem was introduced in 610bfc6bc99bc83680d190ebc69359a05fc7f605
    ("batman-adv: Receive fragmented packets and merge").

    Signed-off-by: Sven Eckelmann
    Signed-off-by: Marek Lindner
    Signed-off-by: Antonio Quartulli

    Sven Eckelmann
     

16 May, 2014

1 commit

  • In the new fragmentation code the batadv_frag_send_packet()
    function obtains a reference to the primary_if, but it does
    not release it upon return.

    This reference imbalance prevents the primary_if (and then
    the related netdevice) to be properly released on shut down.

    Fix this by releasing the primary_if in batadv_frag_send_packet().

    Introduced by ee75ed88879af88558818a5c6609d85f60ff0df4
    ("batman-adv: Fragment and send skbs larger than mtu")

    Cc: Martin Hundebøll
    Signed-off-by: Antonio Quartulli
    Signed-off-by: Marek Lindner
    Acked-by: Martin Hundebøll

    Antonio Quartulli