| Message ID | 20131107070647.GB31638@gondor.apana.org.au |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2013-11-07 at 15:06 +0800, Herbert Xu wrote: > Recently GRO started generating packets with frag_lists of frags. > This was not handled by GSO, thus leading to a crash. > > Thankfully these packets are of a regular form and are easy to > handle. This patch handles them by calling skb_segment for each > frag_list entry. The depth of recursion is limited to just one. > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 88b7dc6..bcc3f1c 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c [...] > @@ -2855,8 +2853,40 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features) > nskb->data - tnl_hlen, > doffset + tnl_hlen); > > - if (fskb != skb_shinfo(skb)->frag_list) > - goto perform_csum_check; > + if (fskb != skb_shinfo(skb)->frag_list) { > + struct sk_buff *nsegs; > + > + if (nskb->len == len + doffset) > + goto perform_csum_check; > + > + SKB_FRAG_ASSERT(nskb); > + > + __skb_pull(nskb, doffset); > + skb_shinfo(nskb)->gso_size = mss; > + nsegs = skb_segment(nskb, features); > + > + err = PTR_ERR(nsegs); > + if (IS_ERR(nsegs)) { > + kfree(nskb); Should be kfree_skb(). > + goto err; > + } > + err = -ENOMEM; It would be clearer to put this in front of the 'err' label: err_nomem: err = -ENOMEM; and change the allocation failure paths to goto err_nomem. > + if (segs) > + tail->next = nsegs; > + else > + segs = nsegs; > + > + tail = nsegs; > + while (tail->next) > + tail = tail->next; > + > + BUG_ON(fskb && tail->len != len + doffset); Deserves a comment, maybe? > + len = nskb->len; > + kfree(nskb); > + continue; > + } > > if (!sg) { > nskb->ip_summed = CHECKSUM_NONE; Ben.
On Thu, Nov 07, 2013 at 06:16:44PM +0000, Ben Hutchings wrote: > On Thu, 2013-11-07 at 15:06 +0800, Herbert Xu wrote: > > Recently GRO started generating packets with frag_lists of frags. > > This was not handled by GSO, thus leading to a crash. > > > > Thankfully these packets are of a regular form and are easy to > > handle. This patch handles them by calling skb_segment for each > > frag_list entry. The depth of recursion is limited to just one. > > > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index 88b7dc6..bcc3f1c 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > [...] > > @@ -2855,8 +2853,40 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features) > > nskb->data - tnl_hlen, > > doffset + tnl_hlen); > > > > - if (fskb != skb_shinfo(skb)->frag_list) > > - goto perform_csum_check; > > + if (fskb != skb_shinfo(skb)->frag_list) { > > + struct sk_buff *nsegs; > > + > > + if (nskb->len == len + doffset) > > + goto perform_csum_check; > > + > > + SKB_FRAG_ASSERT(nskb); > > + > > + __skb_pull(nskb, doffset); > > + skb_shinfo(nskb)->gso_size = mss; > > + nsegs = skb_segment(nskb, features); > > + > > + err = PTR_ERR(nsegs); > > + if (IS_ERR(nsegs)) { > > + kfree(nskb); > > Should be kfree_skb(). Thanks for catching this and I have incorporated this into the newer (albeit completely different) version of the patch.
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 88b7dc6..bcc3f1c 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -2816,8 +2816,6 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features) hsize = len; if (!hsize && i >= nfrags) { - BUG_ON(fskb->len != len); - pos += len; nskb = skb_clone(fskb, GFP_ATOMIC); fskb = fskb->next; @@ -2855,8 +2853,40 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features) nskb->data - tnl_hlen, doffset + tnl_hlen); - if (fskb != skb_shinfo(skb)->frag_list) - goto perform_csum_check; + if (fskb != skb_shinfo(skb)->frag_list) { + struct sk_buff *nsegs; + + if (nskb->len == len + doffset) + goto perform_csum_check; + + SKB_FRAG_ASSERT(nskb); + + __skb_pull(nskb, doffset); + skb_shinfo(nskb)->gso_size = mss; + nsegs = skb_segment(nskb, features); + + err = PTR_ERR(nsegs); + if (IS_ERR(nsegs)) { + kfree(nskb); + goto err; + } + err = -ENOMEM; + + if (segs) + tail->next = nsegs; + else + segs = nsegs; + + tail = nsegs; + while (tail->next) + tail = tail->next; + + BUG_ON(fskb && tail->len != len + doffset); + + len = nskb->len; + kfree(nskb); + continue; + } if (!sg) { nskb->ip_summed = CHECKSUM_NONE;
Recently GRO started generating packets with frag_lists of frags. This was not handled by GSO, thus leading to a crash. Thankfully these packets are of a regular form and are easy to handle. This patch handles them by calling skb_segment for each frag_list entry. The depth of recursion is limited to just one. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>