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

[Leon Romanovsky <leon@kernel.org>]

On Tue, Mar 10, 2026 at 08:24:27PM +0100, Florian Westphal wrote:
> Leon Romanovsky <leon@kernel.org> wrote:
> > > > This change makes me wonder why we need both xfrm_state_hold_rcu() and
> > > > xfrm_state_hold().
> > > 
> > > Commit 02efdff7e209 ("xfrm: state: use atomic_inc_not_zero to
> > > increment refcount") and the series around it [0] introduced the
> > > possibility of that refcount increment failing.
> > > 
> > > I can't tell you why a 10-years-old commit made some choice, but
> > > keeping both variants has the benefit of documenting that one
> > > increment is expected to never fail (because we already hold a ref on
> > > the object on that path) and we can skip the error handling. We don't
> > > want to add error handling that will never get reached, it always goes
> > > wrong (because it's untested) and it adds uneeded complexity to the
> > > code.
> > > 
> > > So I wouldn't get rid of xfrm_state_hold.
> > 
> > So let's add some comment when this function should be used.
> 
> Before rcu-ification, everything was serialized via state lock.
> Entries still in the state table thus always had > 0 refcounts.
> 
> 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.

For example:
  1175 static struct xfrm_state *__xfrm_state_lookup(const struct xfrm_hash_state_ptrs *state_ptrs
  1176                                               u32 mark,                                   
  1177                                               const xfrm_address_t *daddr,               
  1178                                               __be32 spi, u8 proto,                     
  1179                                               unsigned short family)                   
  1180 {                                                                                     
  1181         unsigned int h = __xfrm_spi_hash(daddr, spi, proto, family, state_ptrs->hmask);
  1182         struct xfrm_state *x;                                                         
  1183                                                                                      
  1184         hlist_for_each_entry_rcu(x, state_ptrs->byspi + h, byspi) {                

<...>

  1193                 if (!xfrm_state_hold_rcu(x))                               
  1194                         continue;                                         
  1195                 return x;                                                
  1196         }                                                               
  1197                                                                        
  1198         return NULL;                                                  
  1199 }    

Thanks