Re: [PATCH ipsec-next 01/10] xfrm: state: fix sparse warnings on xfrm_state_hold_rcu

[Leon Romanovsky <leon@kernel.org>]

On Thu, Mar 12, 2026 at 03:36:21PM +0100, Sabrina Dubroca wrote:
> 2026-03-11, 09:36:35 +0200, Leon Romanovsky wrote:
> > On Tue, Mar 10, 2026 at 10:41:46PM +0100, Sabrina Dubroca wrote:
> > > (adding Simon: maybe you can help with the "comment wording" issue here :))
> > > 
> > > 2026-03-10, 22:10:21 +0200, Leon Romanovsky wrote:
> > > > On Tue, Mar 10, 2026 at 08:49:44PM +0100, Florian Westphal wrote:
> > > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > > You can only use xfrm_state_hold() if the refcount is already > 0.
> > > > > > > xfrm_state_hold uses refcount_inc(), so you get a UaF warn splat
> > > > > > > if this assuption doesn't hold true.
> > > > > > 
> > > > > > I know it, the thing that bothers me is that it is unclear how
> > > > > > xfrm_state_hold_rcu() can have refcount equal to 0.
> > > > > > 
> > > > > > xfrm_state_put() decreases refcount and when it is zero, it calls
> > > > > > to __xfrm_state_destroy(). The latter assumes that the state was
> > > > > > already removed from various hlists.
> > > > > 
> > > > > Yes, insertion in the table means refcount is 1, but userspace
> > > > > can zap states at any time, e.g.:
> > > > > 
> > > > > xfrm_del_sa -> xfrm_state_delete -> __xfrm_state_delete (which
> > > > > unlinks from hash lists).
> > > > > 
> > > > > The last xfrm_state_put() in that function may cause 1 -> 0
> > > > > transition.  Parallel lookup can still observe that state,
> > > > > so it has to pretend it wasn't there to begin with.
> > > 
> > > Thanks Florian.
> > > 
> > > 
> > > > Yes, this is possible scenario and this is what is worth to document.
> > > 
> > > 
> > > We could add something like:
> > > 
> > > /* Take a reference to @x, when we know the state has a refcount >= 1.
> > >  * In this case, we can avoid refcount_inc_not_zero and the error
> > >  * handling it requires.
> > >  * In contexts where concurrent state deletion is possible and we
> > >  * don't already hold a reference to that state, xfrm_state_hold_rcu
> > >  * must be used.
> > >  */
> > > 
> > > Though it may not make much sense to refer to xfrm_state_hold_rcu
> > > (implemented in net/xfrm/xfrm_state.c) from include/net/xfrm.h.
> > > 
> > > And if we consider the hashtables to be private to
> > > net/xfrm/xfrm_state.c, nothing outside of it should ever see a state
> > > with refcount=0, since they will only ever see states that already
> > > have one reference held by whatever gave them the pointer.
> > > 
> > > So maybe it's more xfrm_state_hold_rcu that needs a mention of
> > > "concurrent state deletion could bring the refcount to 0 while we're
> > > doing the lookup"? I don't know, for me it's pretty obvious with the
> > > _rcu suffix that RCU -> unlocked -> could be deleted in parallel.
> > 
> > It was the case when xfrm_state had __rcu marker, it is less obvious
> > now. I agree with you that xfrm_state_hold_rcu() needs to be documented
> > and not xfrm_state_hold().
> 
> Ok. Would that work for you?

Yes, it is fine, thanks

> 
> /* Take a reference to @x.
>  * This must be used (and the error handled) for unlocked lookups
>  * where concurrent state deletion could bring the refcount to 0.
>  *
>  * When we know that the state has a refcount >= 1, xfrm_state_hold
>  * can be used.
>  */
> 
> If not, what other information do you need?
> Then I can send the patch separately, as Steffen said.
> 
> -- 
> Sabrina