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: <20230405075050.2fbc4502@kernel.org>
Date:   Wed, 5 Apr 2023 07:50:50 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Liang Chen <liangchen.linux@...il.com>
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, 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.

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

> 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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ