netdev - Re: [PATCH net] tun: correct header offsets in napi frags mode

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] [day] [month] [year] [list]

Message-ID: <CA+FuTScnmJ3aSA06+FndrLvCFSNN_JwPxvtUqPh0xx_+aGd+8A@mail.gmail.com>
Date:   Sat, 30 May 2020 15:47:04 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH net] tun: correct header offsets in napi frags mode

On Fri, May 29, 2020 at 11:14 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> On Fri, May 29, 2020 at 8:27 PM David Miller <davem@...emloft.net> wrote:
> >
> > From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> > Date: Thu, 28 May 2020 13:05:32 -0400
> >
> > > Temporarily pull ETH_HLEN to make control flow the same for frags and
> > > not frags. Then push the header just before calling napi_gro_frags.
> >  ...
> > >       case IFF_TAP:
> > > -             if (!frags)
> > > -                     skb->protocol = eth_type_trans(skb, tun->dev);
> > > +             if (frags && !pskb_may_pull(skb, ETH_HLEN)) {
> > > +                     err = -ENOMEM;
> > > +                     goto drop;
> > > +             }
> > > +             skb->protocol = eth_type_trans(skb, tun->dev);
> >  ...
> > >               /* Exercise flow dissector code path. */
> > > -             u32 headlen = eth_get_headlen(tun->dev, skb->data,
> > > -                                           skb_headlen(skb));
> > > +             skb_push(skb, ETH_HLEN);
> > > +             headlen = eth_get_headlen(tun->dev, skb->data,
> > > +                                       skb_headlen(skb));
> >
> > I hate to be a stickler on wording in the commit message, but the
> > change is not really "pulling" the ethernet header from the SKB.
> >
> > Instead it is invoking pskb_may_pull() which just makes sure the
> > header is there in the linear SKB data area.
> >
> > Can you please refine this description and resubmit?
>
> Of course. How is this
>
> "
>     Ensure the link layer header lies in linear as eth_type_trans pulls
>     ETH_HLEN. Then take the same code paths for frags as for not frags.
>     Push the link layer header back just before calling napi_gro_frags.
>
>     By pulling up to ETH_HLEN from frag0 into linear, this disables the
>     frag0 optimization in the special case when IFF_NAPI_FRAGS is
>     called with zero length iov[0] (and thus empty skb->linear).
> "
>
> Seemed good to add the extra clarification. I don't see a reasonable way
> to avoid that consequence, especially as I cannot restore the first skb frag
> (iov[1]) if it was exactly ETH_HLEN bytes and thus freed by __skb_pull_tail.

Sent. Probably faster that way. Do let me know if still too fast and
loose with wording. I can always do a v3.

Or to add some frags gymnastics to try to maintain the frag0 optimization
when iov[1] > ETH_LEN and frag0 thus can be restored. That just makes
for a more complicated fix.