Re: [2/3] gso: Handle new frag_list of frags GRO packets

Ben Hutchings
2013-11-07T18:16:49Z

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.

--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.