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

[Ben Hutchings]

On Thu, 2014-09-11 at 08:48 -0400, Vlad Yasevich wrote:
> 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.

I can't apply that because 3.2 doesn't even have the
skb_headers_offset_update() function.  So I'm going to drop this patch
for now, but if you or David can provide a complete backport for this
fix I would very much appreciate it.

Ben.

-- 
Ben Hutchings
I'm always amazed by the number of people who take up solipsism because
they heard someone else explain it. - E*Borg on alt.fan.pratchett

Attachment: signature.asc
Description: This is a digitally signed message part