netdev - Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

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]

Message-ID: <CAC_iWjJD-g34ABOhu8f9wMLF0a9YYAZdh_uh2Vq44C-fAU3Nag@mail.gmail.com>
Date:   Thu, 6 Apr 2023 13:46:18 +0300
From:   Ilias Apalodimas <ilias.apalodimas@...aro.org>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Liang Chen <liangchen.linux@...il.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, hawk@...nel.org,
        davem@...emloft.net, pabeni@...hat.com, netdev@...r.kernel.org
Subject: Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

On Thu, 6 Apr 2023 at 12:56, Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Thu, Apr 6, 2023 at 5:28 AM Liang Chen <liangchen.linux@...il.com> wrote:
> >
> > On Wed, Apr 5, 2023 at 11:06 PM Alexander Duyck
> > <alexander.duyck@...il.com> wrote:
> > >
> > > On Wed, Apr 5, 2023 at 1:19 AM Liang Chen <liangchen.linux@...il.com> wrote:
> > > >
> > > > On Wed, Apr 5, 2023 at 9:21 AM Jakub Kicinski <kuba@...nel.org> wrote:
> > > > >
> > > > > On Tue, 04 Apr 2023 08:51:18 -0700 Alexander H Duyck wrote:
> > > > > > I'm not quite sure I agree with the fix. Couldn't we just modify the
> > > > > > check further down that does:
> > > > > >
> > > > > >         if (!skb_cloned(from))
> > > > > >                 from_shinfo->nr_frags = 0;
> > > > > >
> > > > > > And instead just make that:
> > > > > >       if (!skb->cloned || (!skb_cloned(from) && !from->pp_recycle))
> > > > > >                 from_shinfo->nr_frags = 0;
> > > > > >
> > > > > > With that we would retain the existing behavior and in the case of
> > > > > > cloned from frames we would take the references and let the original
> > > > > > from skb freed to take care of pulling the pages from the page pool.
> > > > >
> > > > > Sounds like a better fix, indeed. But this sort of code will require
> > > > > another fat comment above to explain why. This:
> > > > >
> > > > >         if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))
> > > > >
> > > > > is much easier to understand, no?
> > > > >
> > > > > We should at least include that in the explanatory comment, I reckon...
> > > >
> > > > Sure, the idea of dealing with the case where @from transitioned into non cloned
> > > > skb in the function retains the existing behavior, and gives more
> > > > opportunities to
> > > > coalesce skbs. And it seems (!skb_cloned(from) && !from->pp_recycle) is enough
> > > > here.
> > > > I will take a closer look at the code path for the fragstolen case
> > > > before making v2
> > > > patch  -  If @from transitioned into non cloned skb before "if
> > > > (skb_head_is_locked(from))"
> > > >
> > > > Thanks for the reviews.
> > >
> > > Actually I am not sure that works now that I look at it closer. The
> > > problem with using (!skb_cloned(from) && !from->pp_recycle) is that it
> > > breaks the case where both from and to are pp_recycle without being
> > > cloned.
> >
> > Yeah, it would break that case. Thanks!
> > >
> > > So it probably needs to be something actually the setup Jakub
> > > suggested would probably work better:
> > >   if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))
> > >
> >
> > I agree. That's better.
>
> Same feeling on my side.
> I prefer not trying to merge mixed pp_recycle skbs "just because we
> could" at the expense
> of adding more code in a fast path.

+1 here.  The intention of recycling was to affect the normal path as
less as possible.  On top of that, we've some amount of race
conditions over the years, trying to squeeze more performance with
similar tricks.  I'd much rather be safe here, since recycling by
itself is a great performance boost

Regards
/Ilias