Eric Dumazet wrote, On 09/01/2010 12:50 PM: > [PATCH] gro: fix different skb headrooms > > packets entering GRO might have different headrooms, even for a given > flow (because of implementation details in drivers, like copybreak). > We cant force drivers to deliver packets with a fixed headroom. > > 1) fix skb_segment() > > skb_segment() makes the false assumption headrooms of fragments are same > than the head. When CHECKSUM_PARTIAL is used, this can give csum_start > errors, and crash later in skb_copy_and_csum_dev() > > 2) allocate a minimal skb for head of frag_list > > skb_gro_receive() uses netdev_alloc_skb(headroom + skb_gro_offset(p)) to > allocate a fresh skb. This adds NET_SKB_PAD to a padding already > provided by netdevice, depending on various things, like copybreak. > > Use alloc_skb() to allocate an exact padding, to reduce cache line > needs: > NET_SKB_PAD + NET_IP_ALIGN > > bugzilla : https://bugzilla.kernel.org/show_bug.cgi?id=16626 > > Many thanks to Plamen Petrov, testing many debugging patches ! > With help of Jarek Poplawski. > > Reported-by: Plamen Petrov <[email protected]> > Signed-off-by: Eric Dumazet <[email protected]> > CC: Jarek Poplawski <[email protected]> > --- > patch against linux-2.6 current tree > > net/core/skbuff.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c ... > @@ -2702,8 +2706,8 @@ int skb_gro_receive(struct sk_buff **head, struct > sk_buff *skb) > } else if (skb_gro_len(p) != pinfo->gso_size) > return -E2BIG; > > - headroom = skb_headroom(p); > - nskb = netdev_alloc_skb(p->dev, headroom + skb_gro_offset(p)); > + headroom = NET_SKB_PAD + NET_IP_ALIGN; > + nskb = alloc_skb(headroom + skb_gro_offset(p), GFP_ATOMIC); > if (unlikely(!nskb)) > return -ENOMEM;
Hi again, Just had a second look, and unless I miss something... Plamen, could you test this patch, too? (Without removing the previous one.) Thanks, Jarek P. -------------------> [PATCH] gro: Re-fix different skb headrooms The patch: "gro: fix different skb headrooms" in its part: "2) allocate a minimal skb for head of frag_list" is buggy. The copied skb has p->data set at the ip header at the moment, and skb_gro_offset is the length of ip + tcp headers. So, after the change the length of mac header is skipped. Later skb_set_mac_header() sets it into the NET_SKB_PAD area (if it's long enough) and ip header is misaligned at NET_SKB_PAD + NET_IP_ALIGN offset. There is no reason to assume the original skb was wrongly allocated, so let's copy it as it was. bugzilla : https://bugzilla.kernel.org/show_bug.cgi?id=16626 fixes commit: 3d3be4333fdf6faa080947b331a6a19bce1a4f57 Reported-by: Plamen Petrov <[email protected]> Signed-off-by: Jarek Poplawski <[email protected]> CC: Eric Dumazet <[email protected]> --- diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 26396ff..c83b421 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -2706,7 +2706,7 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb) } else if (skb_gro_len(p) != pinfo->gso_size) return -E2BIG; - headroom = NET_SKB_PAD + NET_IP_ALIGN; + headroom = skb_headroom(p); nskb = alloc_skb(headroom + skb_gro_offset(p), GFP_ATOMIC); if (unlikely(!nskb)) return -ENOMEM; -- To unsubscribe from this list: send the line "unsubscribe kernel-testers" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
