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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAKhg4tJLW35D-euBFM+_ph=deSq1uHjpYQVWuZUHdCL8D3h5Og@mail.gmail.com>
Date:   Thu, 6 Apr 2023 11:22:30 +0800
From:   Liang Chen <liangchen.linux@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Alexander H Duyck <alexander.duyck@...il.com>,
        ilias.apalodimas@...aro.org, hawk@...nel.org, davem@...emloft.net,
        edumazet@...gle.com, pabeni@...hat.com, netdev@...r.kernel.org
Subject: Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

On Wed, Apr 5, 2023 at 10:50 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 5 Apr 2023 16:18:47 +0800 Liang Chen wrote:
> > > 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.
>
> Well, that's pretty much what Alex suggested minus the optimization he
> put in for "was never cloned" which is probably worth having. So if
> you're gonna do this just use his code.
>
> My point was that !from->pp_recycle requires the reader to understand
> the relationship between this check and the previous condition at entry.
> While to->pp_recycle == from->pp_recycle seems much more obvious to me -
> directly shifting frags between skbs with different refcount styles is
> dangerous.
>

Yeah, I agree with the point that to->pp_recycle == from->pp_recycle
is easier to understand, and will use it in the next iteration of the
patch. Thanks!

> Maybe it's just me, so whatever.
> Make sure you write a good comment.

Sure.
>
> > 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))"
> >

I took a closer look at the code path for the "fragstolen" case, and
it indeed requires a special handle for the situation addressed in
this patch. Something like,

    if( to->pp_recycle != from->pp_recycle )
        get_page(page);

before  "*fragstolen = true;".   But this makes the logic a bit
complicated. Anyway, I will include the logic in the v2 patch. Let's
see if it looks better.

> > Thanks for the reviews.