[PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list

[PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list

[Shmulik Ladkani]

Historically, support for frag_list packets entering skb_segment() was
limited to frag_list members terminating on exact same gso_size
boundaries. This is verified with a BUG_ON since commit 89319d3801d1
("net: Add frag_list support to skb_segment"), quote:

    As such we require all frag_list members terminate on exact MSS
    boundaries.  This is checked using BUG_ON.
    As there should only be one producer in the kernel of such packets,
    namely GRO, this requirement should not be difficult to maintain.

However, since commit 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper"),
the "exact MSS boundaries" assumption no longer holds:
An eBPF program using bpf_skb_change_proto() DOES modify 'gso_size', but
leaves the frag_list members as originally merged by GRO with the
original 'gso_size'. Example of such programs are bpf-based NAT46 or
NAT64.

This lead to a kernel BUG_ON for flows involving:
 - GRO generating a frag_list skb
 - bpf program performing bpf_skb_change_proto() or bpf_skb_adjust_room()
 - skb_segment() of the skb

See example BUG_ON reports in [0].

In commit 13acc94eff12 ("net: permit skb_segment on head_frag frag_list skb"),
skb_segment() was modified to support the "gso_size mangling" case of
a frag_list GRO'ed skb, but *only* for frag_list members having
head_frag==true (having a page-fragment head).

Alas, GRO packets having frag_list members with a linear kmalloced head
(head_frag==false) still hit the BUG_ON.

This commit adds support to skb_segment() for a 'head_skb' packet having
a frag_list whose members are *non* head_frag, with gso_size mangled, by
disabling SG and thus falling-back to copying the data from the given
'head_skb' into the generated segmented skbs - as suggested by Willem de
Bruijn [1].

Since this approach involves the penalty of skb_copy_and_csum_bits()
when building the segments, care was taken in order to enable this
solution only when required:
 - untrusted gso_size, by testing SKB_GSO_DODGY is set
   (SKB_GSO_DODGY is set by any gso_size mangling functions in
    net/core/filter.c)
 - the frag_list is non empty, its item is a non head_frag, *and* the
   headlen of the given 'head_skb' does not match the gso_size.

[0]
https://lore.kernel.org/netdev/20190826170724.25ff616f@pixies/
https://lore.kernel.org/netdev/9265b93f-253d-6b8c-f2b8-4b54eff1835c@fb.com/

[1]
https://lore.kernel.org/netdev/CA+FuTSfVsgNDi7c=GUU8nMg2hWxF2SjCNLXetHeVPdnxAW5K-w@mail.gmail.com/

Fixes: 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper")
Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Alexander Duyck <alexander.duyck@gmail.com>
Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
---
 net/core/skbuff.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ea8e8d332d85..c4bd1881acff 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3678,6 +3678,24 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 	sg = !!(features & NETIF_F_SG);
 	csum = !!can_checksum_protocol(features, proto);
 
+	if (mss != GSO_BY_FRAGS &&
+	    (skb_shinfo(head_skb)->gso_type & SKB_GSO_DODGY)) {
+		/* gso_size is untrusted.
+		 *
+		 * If head_skb has a frag_list with a linear non head_frag
+		 * item, and head_skb's headlen does not fit requested
+		 * gso_size, fall back to copying the skbs - by disabling sg.
+		 *
+		 * We assume checking the first frag suffices, i.e if either of
+		 * the frags have non head_frag data, then the first frag is
+		 * too.
+		 */
+		if (list_skb && skb_headlen(list_skb) && !list_skb->head_frag &&
+		    (mss != skb_headlen(head_skb) - doffset)) {
+			sg = false;
+		}
+	}
+
 	if (sg && csum && (mss != GSO_BY_FRAGS))  {
 		if (!(features & NETIF_F_GSO_PARTIAL)) {
 			struct sk_buff *iter;
-- 
2.19.1

Re: [PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list

[Alexander Duyck]

On Thu, Sep 5, 2019 at 11:36 AM Shmulik Ladkani
<shmulik@metanetworks.com> wrote:
>
> Historically, support for frag_list packets entering skb_segment() was
> limited to frag_list members terminating on exact same gso_size
> boundaries. This is verified with a BUG_ON since commit 89319d3801d1
> ("net: Add frag_list support to skb_segment"), quote:
>
>     As such we require all frag_list members terminate on exact MSS
>     boundaries.  This is checked using BUG_ON.
>     As there should only be one producer in the kernel of such packets,
>     namely GRO, this requirement should not be difficult to maintain.
>
> However, since commit 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper"),
> the "exact MSS boundaries" assumption no longer holds:
> An eBPF program using bpf_skb_change_proto() DOES modify 'gso_size', but
> leaves the frag_list members as originally merged by GRO with the
> original 'gso_size'. Example of such programs are bpf-based NAT46 or
> NAT64.
>
> This lead to a kernel BUG_ON for flows involving:
>  - GRO generating a frag_list skb
>  - bpf program performing bpf_skb_change_proto() or bpf_skb_adjust_room()
>  - skb_segment() of the skb
>
> See example BUG_ON reports in [0].
>
> In commit 13acc94eff12 ("net: permit skb_segment on head_frag frag_list skb"),
> skb_segment() was modified to support the "gso_size mangling" case of
> a frag_list GRO'ed skb, but *only* for frag_list members having
> head_frag==true (having a page-fragment head).
>
> Alas, GRO packets having frag_list members with a linear kmalloced head
> (head_frag==false) still hit the BUG_ON.
>
> This commit adds support to skb_segment() for a 'head_skb' packet having
> a frag_list whose members are *non* head_frag, with gso_size mangled, by
> disabling SG and thus falling-back to copying the data from the given
> 'head_skb' into the generated segmented skbs - as suggested by Willem de
> Bruijn [1].
>
> Since this approach involves the penalty of skb_copy_and_csum_bits()
> when building the segments, care was taken in order to enable this
> solution only when required:
>  - untrusted gso_size, by testing SKB_GSO_DODGY is set
>    (SKB_GSO_DODGY is set by any gso_size mangling functions in
>     net/core/filter.c)
>  - the frag_list is non empty, its item is a non head_frag, *and* the
>    headlen of the given 'head_skb' does not match the gso_size.
>
> [0]
> https://lore.kernel.org/netdev/20190826170724.25ff616f@pixies/
> https://lore.kernel.org/netdev/9265b93f-253d-6b8c-f2b8-4b54eff1835c@fb.com/
>
> [1]
> https://lore.kernel.org/netdev/CA+FuTSfVsgNDi7c=GUU8nMg2hWxF2SjCNLXetHeVPdnxAW5K-w@mail.gmail.com/
>
> Fixes: 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper")
> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
> ---
>  net/core/skbuff.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index ea8e8d332d85..c4bd1881acff 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3678,6 +3678,24 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>         sg = !!(features & NETIF_F_SG);
>         csum = !!can_checksum_protocol(features, proto);
>
> +       if (mss != GSO_BY_FRAGS &&
> +           (skb_shinfo(head_skb)->gso_type & SKB_GSO_DODGY)) {
> +               /* gso_size is untrusted.
> +                *
> +                * If head_skb has a frag_list with a linear non head_frag
> +                * item, and head_skb's headlen does not fit requested
> +                * gso_size, fall back to copying the skbs - by disabling sg.
> +                *
> +                * We assume checking the first frag suffices, i.e if either of
> +                * the frags have non head_frag data, then the first frag is
> +                * too.
> +                */
> +               if (list_skb && skb_headlen(list_skb) && !list_skb->head_frag &&
> +                   (mss != skb_headlen(head_skb) - doffset)) {
> +                       sg = false;
> +               }
> +       }
> +

I would change the order of the tests you use here so that we can
eliminate the possibility of needing to perform many tests for the
more common cases. You could probably swap "list_skb" and "mss !=
GSO_BY_FRAGS" since list_skb is more likely to be false for many of
the common cases such as a standard TSO send from a socket. You might
even consider moving the GSO_BY_FRAGS check toward the end of your
checks since SCTP is the only protocol that I believe uses it and the
likelihood of encountering it is much lower compared to other
protocols.

You could probably test for !list_skb->head_frag before seeing if
there is a headlen since many NICs would be generating frames using
head_frag, so in the GRO case you mentioned above it could probably
save you some effort on a number of NICs.

You might also consider moving this code up before we push the mac
header back on and instead of setting sg to false you could just clear
the NETIF_F_SG flag from features. It would save you from having to
then remove doffset in your last check.

>         if (sg && csum && (mss != GSO_BY_FRAGS))  {
>                 if (!(features & NETIF_F_GSO_PARTIAL)) {
>                         struct sk_buff *iter;
> --
> 2.19.1
>

Re: [PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list

[Shmulik Ladkani]

On Thu, 5 Sep 2019 14:49:44 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:

> I would change the order of the tests you use here so that we can
> eliminate the possibility of needing to perform many tests for the
> more common cases. You could probably swap "list_skb" and "mss !=
> GSO_BY_FRAGS" since list_skb is more likely to be false for many of
> the common cases such as a standard TSO send from a socket. You might
> even consider moving the GSO_BY_FRAGS check toward the end of your
> checks since SCTP is the only protocol that I believe uses it and the
> likelihood of encountering it is much lower compared to other
> protocols.
> 
> You could probably test for !list_skb->head_frag before seeing if
> there is a headlen since many NICs would be generating frames using
> head_frag, so in the GRO case you mentioned above it could probably
> save you some effort on a number of NICs.
> 
> You might also consider moving this code up before we push the mac
> header back on and instead of setting sg to false you could just clear
> the NETIF_F_SG flag from features. It would save you from having to
> then remove doffset in your last check.

Thanks Alexander for the input. Will encorporate as much as possible
into a v2 patch.

BTW, I've strugged with myself regarding order of tests, and came
up with this suggestion, as my motivation was to have the tests order
tell a coherent logical story when read top-to-bottom left-to-right, FWIW.
For example, although 'mss != skb_headlen(head_skb)' could be tested
earlier, the condition by itself isn't meaningful unless we have an
existing frag_list and with a !head_frag.

Best
Shmulik

Re: [PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list

[Willem de Bruijn]

On Thu, Sep 5, 2019 at 2:36 PM Shmulik Ladkani <shmulik@metanetworks.com> wrote:
>
> Historically, support for frag_list packets entering skb_segment() was
> limited to frag_list members terminating on exact same gso_size
> boundaries. This is verified with a BUG_ON since commit 89319d3801d1
> ("net: Add frag_list support to skb_segment"), quote:
>
>     As such we require all frag_list members terminate on exact MSS
>     boundaries.  This is checked using BUG_ON.
>     As there should only be one producer in the kernel of such packets,
>     namely GRO, this requirement should not be difficult to maintain.
>
> However, since commit 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper"),
> the "exact MSS boundaries" assumption no longer holds:
> An eBPF program using bpf_skb_change_proto() DOES modify 'gso_size', but
> leaves the frag_list members as originally merged by GRO with the
> original 'gso_size'. Example of such programs are bpf-based NAT46 or
> NAT64.
>
> This lead to a kernel BUG_ON for flows involving:
>  - GRO generating a frag_list skb
>  - bpf program performing bpf_skb_change_proto() or bpf_skb_adjust_room()
>  - skb_segment() of the skb
>
> See example BUG_ON reports in [0].
>
> In commit 13acc94eff12 ("net: permit skb_segment on head_frag frag_list skb"),
> skb_segment() was modified to support the "gso_size mangling" case of
> a frag_list GRO'ed skb, but *only* for frag_list members having
> head_frag==true (having a page-fragment head).
>
> Alas, GRO packets having frag_list members with a linear kmalloced head
> (head_frag==false) still hit the BUG_ON.
>
> This commit adds support to skb_segment() for a 'head_skb' packet having
> a frag_list whose members are *non* head_frag, with gso_size mangled, by
> disabling SG and thus falling-back to copying the data from the given
> 'head_skb' into the generated segmented skbs - as suggested by Willem de
> Bruijn [1].
>
> Since this approach involves the penalty of skb_copy_and_csum_bits()
> when building the segments, care was taken in order to enable this
> solution only when required:
>  - untrusted gso_size, by testing SKB_GSO_DODGY is set
>    (SKB_GSO_DODGY is set by any gso_size mangling functions in
>     net/core/filter.c)
>  - the frag_list is non empty, its item is a non head_frag, *and* the
>    headlen of the given 'head_skb' does not match the gso_size.
>
> [0]
> https://lore.kernel.org/netdev/20190826170724.25ff616f@pixies/
> https://lore.kernel.org/netdev/9265b93f-253d-6b8c-f2b8-4b54eff1835c@fb.com/
>
> [1]
> https://lore.kernel.org/netdev/CA+FuTSfVsgNDi7c=GUU8nMg2hWxF2SjCNLXetHeVPdnxAW5K-w@mail.gmail.com/
>
> Fixes: 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper")
> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
> ---
>  net/core/skbuff.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index ea8e8d332d85..c4bd1881acff 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3678,6 +3678,24 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>         sg = !!(features & NETIF_F_SG);
>         csum = !!can_checksum_protocol(features, proto);
>
> +       if (mss != GSO_BY_FRAGS &&
> +           (skb_shinfo(head_skb)->gso_type & SKB_GSO_DODGY)) {
> +               /* gso_size is untrusted.
> +                *
> +                * If head_skb has a frag_list with a linear non head_frag
> +                * item, and head_skb's headlen does not fit requested
> +                * gso_size, fall back to copying the skbs - by disabling sg.
> +                *
> +                * We assume checking the first frag suffices, i.e if either of
> +                * the frags have non head_frag data, then the first frag is
> +                * too.
> +                */
> +               if (list_skb && skb_headlen(list_skb) && !list_skb->head_frag &&
> +                   (mss != skb_headlen(head_skb) - doffset)) {

I thought the idea was to check skb_headlen(list_skb), as that is the
cause of the problem. Is skb_headlen(head_skb) a good predictor of
that? I can certainly imagine that it is, just not sure.

Thanks for preparing the patch, and explaining the problem and
solution clearly in the commit message. I'm pretty sure I'll have
forgotten the finer details next time we have to look at this
function again.

Re: [PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list

[Shmulik Ladkani]

On Thu, 5 Sep 2019 17:51:20 -0400
Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:

> On Thu, Sep 5, 2019 at 2:36 PM Shmulik Ladkani <shmulik@metanetworks.com> wrote:
> >
> > +       if (mss != GSO_BY_FRAGS &&
> > +           (skb_shinfo(head_skb)->gso_type & SKB_GSO_DODGY)) {
> > +               /* gso_size is untrusted.
> > +                *
> > +                * If head_skb has a frag_list with a linear non head_frag
> > +                * item, and head_skb's headlen does not fit requested
> > +                * gso_size, fall back to copying the skbs - by disabling sg.
> > +                *
> > +                * We assume checking the first frag suffices, i.e if either of
> > +                * the frags have non head_frag data, then the first frag is
> > +                * too.
> > +                */
> > +               if (list_skb && skb_headlen(list_skb) && !list_skb->head_frag &&
> > +                   (mss != skb_headlen(head_skb) - doffset)) {  
> 
> I thought the idea was to check skb_headlen(list_skb), as that is the
> cause of the problem. Is skb_headlen(head_skb) a good predictor of
> that? I can certainly imagine that it is, just not sure.

Yes, 'mss != skb_headlen(HEAD_SKB)' seems to be a very good predictor,
both for the test reproducer, and what's observered on a live system.

We *CANNOT* use 'mss != skb_headlen(LIST_SKB)' as the test condition.
The packet could have just a SINGLE frag_list member, and that member could
be a "small remainder" not reaching the full mss size - so we could hit
the test condition EVEN FOR NON gso_size mangled frag_list skbs -
which is not desired.

Also, is we test 'mss != skb_headlen(list_skb)' and execute 'sg=false'
ONLY IF 'list_skb' is *NOT* the last item, this is still bogus.
Imagine a gso_size mangled packet having just head_skb and a single
"small remainder" frag. This packet will hit the BUG_ON, as the
'sg=false' solution is now skipped according to the revised condition.

> Thanks for preparing the patch, and explaining the problem and
> solution clearly in the commit message. I'm pretty sure I'll have
> forgotten the finer details next time we have to look at this
> function again.

Indeed. Apparently I've been there myself few years back and forgot all
the gritty details :) see [0]

[0] https://patchwork.ozlabs.org/patch/661419/ 

Re: [PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list

[Willem de Bruijn]

On Fri, Sep 6, 2019 at 2:47 AM Shmulik Ladkani <shmulik@metanetworks.com> wrote:
>
> On Thu, 5 Sep 2019 17:51:20 -0400
> Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>
> > On Thu, Sep 5, 2019 at 2:36 PM Shmulik Ladkani <shmulik@metanetworks.com> wrote:
> > >
> > > +       if (mss != GSO_BY_FRAGS &&
> > > +           (skb_shinfo(head_skb)->gso_type & SKB_GSO_DODGY)) {
> > > +               /* gso_size is untrusted.
> > > +                *
> > > +                * If head_skb has a frag_list with a linear non head_frag
> > > +                * item, and head_skb's headlen does not fit requested
> > > +                * gso_size, fall back to copying the skbs - by disabling sg.
> > > +                *
> > > +                * We assume checking the first frag suffices, i.e if either of
> > > +                * the frags have non head_frag data, then the first frag is
> > > +                * too.
> > > +                */
> > > +               if (list_skb && skb_headlen(list_skb) && !list_skb->head_frag &&
> > > +                   (mss != skb_headlen(head_skb) - doffset)) {
> >
> > I thought the idea was to check skb_headlen(list_skb), as that is the
> > cause of the problem. Is skb_headlen(head_skb) a good predictor of
> > that? I can certainly imagine that it is, just not sure.
>
> Yes, 'mss != skb_headlen(HEAD_SKB)' seems to be a very good predictor,
> both for the test reproducer, and what's observered on a live system.
>
> We *CANNOT* use 'mss != skb_headlen(LIST_SKB)' as the test condition.
> The packet could have just a SINGLE frag_list member, and that member could
> be a "small remainder" not reaching the full mss size - so we could hit
> the test condition EVEN FOR NON gso_size mangled frag_list skbs -
> which is not desired.

The last segment. Yes, good point.

> Also, is we test 'mss != skb_headlen(list_skb)' and execute 'sg=false'
> ONLY IF 'list_skb' is *NOT* the last item, this is still bogus.
> Imagine a gso_size mangled packet having just head_skb and a single
> "small remainder" frag. This packet will hit the BUG_ON, as the
> 'sg=false' solution is now skipped according to the revised condition.

Right, I wouldn't suggest that.

But I wonder whether it is a given that head_skb has headlen.

Btw, it seems slightly odd to me tot test head_frag before testing
headlen in the v2 patch.

Re: [PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list

[Shmulik Ladkani]

On Fri, 6 Sep 2019 10:49:55 -0400
Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:

> But I wonder whether it is a given that head_skb has headlen.

This is what I observed for GRO packets that do have headlen frag_list
members: the 'head_skb' itself had a headlen too, and its head was
built using the original gso_size (similar to the frag_list members).

Maybe Eric can comment better.

> Btw, it seems slightly odd to me tot test head_frag before testing
> headlen in the v2 patch.

Requested by Alexander. I'm fine either way.

Thanks
Shmulik

Re: [PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list

[Alexander Duyck]

On Fri, Sep 6, 2019 at 8:37 AM Shmulik Ladkani <shmulik@metanetworks.com> wrote:
>
> On Fri, 6 Sep 2019 10:49:55 -0400
> Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>
> > But I wonder whether it is a given that head_skb has headlen.
>
> This is what I observed for GRO packets that do have headlen frag_list
> members: the 'head_skb' itself had a headlen too, and its head was
> built using the original gso_size (similar to the frag_list members).
>
> Maybe Eric can comment better.
>
> > Btw, it seems slightly odd to me tot test head_frag before testing
> > headlen in the v2 patch.
>
> Requested by Alexander. I'm fine either way.

Yeah, my thought on that was "do we care about the length if the data
is stored in a head_frag?". I suppose you could flip the logic and
make it "do we care about it being a head_frag if there is no data
there?". The reason I had suggested the head_frag test first was
because it was a single test bit whereas the length requires reading
two fields and doing a comparison.

For either ordering it is fine by me. So if we need to feel free to
swap those two tests for a v3.

Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Re: [PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list

[Willem de Bruijn]

On Fri, Sep 6, 2019 at 11:44 AM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> On Fri, Sep 6, 2019 at 8:37 AM Shmulik Ladkani <shmulik@metanetworks.com> wrote:
> >
> > On Fri, 6 Sep 2019 10:49:55 -0400
> > Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> >
> > > But I wonder whether it is a given that head_skb has headlen.
> >
> > This is what I observed for GRO packets that do have headlen frag_list
> > members: the 'head_skb' itself had a headlen too, and its head was
> > built using the original gso_size (similar to the frag_list members).

That makes sense.

I was thinking of, say, a driver that combines napi_gro_frags with a
copy break optimization. But given that gso_size is the same for all
segments expect perhaps the last, all those segments will have taken
the same path.

And if we're wrong we'll find out soon enough and can return to this
topic yet again. skb_segment really puts the fun in function.

> >
> > Maybe Eric can comment better.
> >
> > > Btw, it seems slightly odd to me tot test head_frag before testing
> > > headlen in the v2 patch.
> >
> > Requested by Alexander. I'm fine either way.
>
> Yeah, my thought on that was "do we care about the length if the data
> is stored in a head_frag?". I suppose you could flip the logic and
> make it "do we care about it being a head_frag if there is no data
> there?". The reason I had suggested the head_frag test first was
> because it was a single test bit whereas the length requires reading
> two fields and doing a comparison.
>
> For either ordering it is fine by me. So if we need to feel free to
> swap those two tests for a v3.

Got it. I don't feel strongly either. No need for a v3 for that.

> Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list

[Shmulik Ladkani]

> > Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>  
> 
> Reviewed-by: Willem de Bruijn <willemb@google.com>

Thank you Alexander and Willem.

Care to reply with you Reviewed-by tags on the v2 thread?

Best,
Shmulik