[maint,2/3] batman-adv: Keep fragments equally sized

  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 1257 byte frame (plus inner
ethernet header (14) and batadv unicast header (10)) to fit in a 1280 bytes
large MTU of the underlying link. This would create a 1280 large frame
(fragment 2) and a 41 bytes large frame (fragment 1). The extra 40 bytes
are the fragment header (20) added to each fragment.

Let us assume that the next hop is then not able to transport 1280 bytes to
its next hop. The 1280 byte large packet will be dropped but the 41 bytes
large packet 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 41 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 19
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 661 and 660 bytes large fragment frames could both potentially
reach its destination without being to large or too small.

Reported-by: Martin Weinelt <martin@darmstadt.freifunk.net>
Fixes: db56e4ecf5c2 ("batman-adv: Fragment and send skbs larger than mtu")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Acked-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
 net/batman-adv/fragmentation.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)
  

On 03/04/2017 04:33 PM, Sven Eckelmann wrote:
> 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 1257 byte frame (plus inner
> ethernet header (14) and batadv unicast header (10)) to fit in a 1280 bytes
> large MTU of the underlying link. This would create a 1280 large frame
> (fragment 2) and a 41 bytes large frame (fragment 1). The extra 40 bytes
> are the fragment header (20) added to each fragment.
> 
> Let us assume that the next hop is then not able to transport 1280 bytes to
> its next hop. The 1280 byte large packet will be dropped but the 41 bytes
> large packet 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 41 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 19
> bytes of the 60 bytes frame are padding and don't belong to the reassembled
> frame.

Just nitpicking, but for Ethernet the 14byte header counts into frame size,
so it's actually a 55 byte frame that is padded to 60 bytes. This means
that for a given hardif MTU (and 2 fragments), there is a range of
precisely 5 byte lengths that will trigger the issue, which explains how it
could stay undetected for so long.

Matthias

> 
> This can partly be avoided by splitting frames more equally. In this
> example, the 661 and 660 bytes large fragment frames could both potentially
> reach its destination without being to large or too small.
> 
> Reported-by: Martin Weinelt <martin@darmstadt.freifunk.net>
> Fixes: db56e4ecf5c2 ("batman-adv: Fragment and send skbs larger than mtu")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> Acked-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---
>  net/batman-adv/fragmentation.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/net/batman-adv/fragmentation.c b/net/batman-adv/fragmentation.c
> index 11a23fd6..8f964bea 100644
> --- a/net/batman-adv/fragmentation.c
> +++ b/net/batman-adv/fragmentation.c
> @@ -404,7 +404,7 @@ bool batadv_frag_skb_fwd(struct sk_buff *skb,
>   * batadv_frag_create - create a fragment from skb
>   * @skb: skb to create fragment from
>   * @frag_head: header to use in new fragment
> - * @mtu: size of new fragment
> + * @fragment_size: size of new fragment
>   *
>   * Split the passed skb into two fragments: A new one with size matching the
>   * passed mtu and the old one with the rest. The new skb contains data from the
> @@ -414,11 +414,11 @@ bool batadv_frag_skb_fwd(struct sk_buff *skb,
>   */
>  static struct sk_buff *batadv_frag_create(struct sk_buff *skb,
>  					  struct batadv_frag_packet *frag_head,
> -					  unsigned int mtu)
> +					  unsigned int fragment_size)
>  {
>  	struct sk_buff *skb_fragment;
>  	unsigned int header_size = sizeof(*frag_head);
> -	unsigned int fragment_size = mtu - header_size;
> +	unsigned int mtu = fragment_size + header_size;
>  
>  	skb_fragment = netdev_alloc_skb(NULL, mtu + ETH_HLEN);
>  	if (!skb_fragment)
> @@ -456,7 +456,7 @@ int batadv_frag_send_packet(struct sk_buff *skb,
>  	struct sk_buff *skb_fragment;
>  	unsigned int mtu = neigh_node->if_incoming->net_dev->mtu;
>  	unsigned int header_size = sizeof(frag_header);
> -	unsigned int max_fragment_size, max_packet_size;
> +	unsigned int max_fragment_size, num_fragments;
>  	int ret;
>  
>  	/* To avoid merge and refragmentation at next-hops we never send
> @@ -464,10 +464,15 @@ int batadv_frag_send_packet(struct sk_buff *skb,
>  	 */
>  	mtu = min_t(unsigned int, mtu, BATADV_FRAG_MAX_FRAG_SIZE);
>  	max_fragment_size = mtu - header_size;
> -	max_packet_size = max_fragment_size * BATADV_FRAG_MAX_FRAGMENTS;
> +
> +	if (skb->len == 0 || max_fragment_size == 0)
> +		return -EINVAL;
> +
> +	num_fragments = (skb->len - 1) / max_fragment_size + 1;
> +	max_fragment_size = (skb->len - 1) / num_fragments + 1;
>  
>  	/* Don't even try to fragment, if we need more than 16 fragments */
> -	if (skb->len > max_packet_size) {
> +	if (num_fragments > BATADV_FRAG_MAX_FRAGMENTS) {
>  		ret = -EAGAIN;
>  		goto free_skb;
>  	}
> @@ -507,7 +512,8 @@ int batadv_frag_send_packet(struct sk_buff *skb,
>  			goto put_primary_if;
>  		}
>  
> -		skb_fragment = batadv_frag_create(skb, &frag_header, mtu);
> +		skb_fragment = batadv_frag_create(skb, &frag_header,
> +						  max_fragment_size);
>  		if (!skb_fragment) {
>  			ret = -ENOMEM;
>  			goto put_primary_if;
>