Re: [PATCH 3.2 105/131] net: Correctly set segment mac_len in skb_segment().

[Vlad Yasevich]

On 09/11/2014 08:32 AM, Ben Hutchings wrote:
> 3.2.63-rc1 review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Vlad Yasevich <vyasevic@xxxxxxxxxx>
> 
> [ Upstream commit fcdfe3a7fa4cb74391d42b6a26dc07c20dab1d82 ]
> 
> When performing segmentation, the mac_len value is copied right
> out of the original skb.  However, this value is not always set correctly
> (like when the packet is VLAN-tagged) and we'll end up copying a bad
> value.
> 
> One way to demonstrate this is to configure a VM which tags
> packets internally and turn off VLAN acceleration on the forwarding
> bridge port.  The packets show up corrupt like this:
> 16:18:24.985548 52:54:00:ab:be:25 > 52:54:00:26:ce:a3, ethertype 802.1Q
> (0x8100), length 1518: vlan 100, p 0, ethertype 0x05e0,
>         0x0000:  8cdb 1c7c 8cdb 0064 4006 b59d 0a00 6402 ...|...d@.....d.
>         0x0010:  0a00 6401 9e0d b441 0a5e 64ec 0330 14fa ..d....A.^d..0..
>         0x0020:  29e3 01c9 f871 0000 0101 080a 000a e833)....q.........3
>         0x0030:  000f 8c75 6e65 7470 6572 6600 6e65 7470 ...unetperf.netp
>         0x0040:  6572 6600 6e65 7470 6572 6600 6e65 7470 erf.netperf.netp
>         0x0050:  6572 6600 6e65 7470 6572 6600 6e65 7470 erf.netperf.netp
>         0x0060:  6572 6600 6e65 7470 6572 6600 6e65 7470 erf.netperf.netp
>         ...
> 
> This also leads to awful throughput as GSO packets are dropped and
> cause retransmissions.
> 
> The solution is to set the mac_len using the values already available
> in then new skb.  We've already adjusted all of the header offset, so we
> might as well correctly figure out the mac_len using skb_reset_mac_len().
> After this change, packets are segmented correctly and performance
> is restored.
> 
> CC: Eric Dumazet <edumazet@xxxxxxxxxx>
> Signed-off-by: Vlad Yasevich <vyasevic@xxxxxxxxxx>
> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
> Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> ---
>  net/core/skbuff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 7121d9b..0ccfb53 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2669,7 +2669,6 @@ struct sk_buff *skb_segment(struct sk_buff *skb, u32 features)
>  		tail = nskb;
>  
>  		__copy_skb_header(nskb, skb);
> -		nskb->mac_len = skb->mac_len;
>  
>  		/* nskb and skb might have different headroom */
>  		if (nskb->ip_summed == CHECKSUM_PARTIAL)
> @@ -2679,6 +2678,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, u32 features)
>  		skb_set_network_header(nskb, skb->mac_len);
>  		nskb->transport_header = (nskb->network_header +
>  					  skb_network_header_len(skb));
> +		skb_reset_mac_len(nskb);

This will not fix the problem here because the network header above will
already be set incorrectly based on the old mac_len.

This patch depends on
commit 030737bcc3c404e273e97dbe06fe9561699a411b
Author: Eric Dumazet <edumazet@xxxxxxxxxx>
Date:   Sat Oct 19 11:42:54 2013 -0700

    net: generalize skb_segment()

that correctly populates the header offsets in the new segment.

-vlad

>  		skb_copy_from_linear_data(skb, nskb->data, doffset);
>  
>  		if (fskb != skb_shinfo(skb)->frag_list)
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/