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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 6 Sep 2019 12:44:04 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Shmulik Ladkani <shmulik@...anetworks.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>, eyal@...anetworks.com,
        netdev <netdev@...r.kernel.org>,
        Shmulik Ladkani <shmulik.ladkani@...il.com>
Subject: Re: [PATCH net] net: gso: Fix skb_segment splat when splitting
 gso_size mangled skb having linear-headed frag_list

On Fri, Sep 6, 2019 at 11:44 AM Alexander Duyck
<alexander.duyck@...il.com> wrote:
>
> On Fri, Sep 6, 2019 at 8:37 AM Shmulik Ladkani <shmulik@...anetworks.com> wrote:
> >
> > On Fri, 6 Sep 2019 10:49:55 -0400
> > Willem de Bruijn <willemdebruijn.kernel@...il.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@...ux.intel.com>

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