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 12:33:23PM +0100, Sabrina Dubroca wrote:
> 2026-03-10, 12:31:35 +0200, Leon Romanovsky wrote:
> > On Mon, Mar 09, 2026 at 11:32:34AM +0100, Sabrina Dubroca wrote:
> > > In all callers, x is not an __rcu pointer. We can drop the annotation to
> > > avoid sparse warnings:
> > > 
> > > net/xfrm/xfrm_state.c:58:39: warning: incorrect type in argument 1 (different address spaces)
> > > net/xfrm/xfrm_state.c:58:39:    expected struct refcount_struct [usertype] *r
> > > net/xfrm/xfrm_state.c:58:39:    got struct refcount_struct [noderef] __rcu *
> > > net/xfrm/xfrm_state.c:1166:42: warning: incorrect type in argument 1 (different address spaces)
> > > net/xfrm/xfrm_state.c:1166:42:    expected struct xfrm_state [noderef] __rcu *x
> > > net/xfrm/xfrm_state.c:1166:42:    got struct xfrm_state *[assigned] x
> > > (repeated for each caller)
> > > 
> > > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > > ---
> > >  net/xfrm/xfrm_state.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > > index 98b362d51836..7a68c594ce37 100644
> > > --- a/net/xfrm/xfrm_state.c
> > > +++ b/net/xfrm/xfrm_state.c
> > > @@ -53,7 +53,7 @@ static DECLARE_WORK(xfrm_state_gc_work, xfrm_state_gc_task);
> > >  static HLIST_HEAD(xfrm_state_gc_list);
> > >  static HLIST_HEAD(xfrm_state_dev_gc_list);
> > >  
> > > -static inline bool xfrm_state_hold_rcu(struct xfrm_state __rcu *x)
> > > +static inline bool xfrm_state_hold_rcu(struct xfrm_state *x)
> > >  {
> > >  	return refcount_inc_not_zero(&x->refcnt);
> > >  }
> > 
> > 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.

Thanks

> 
> [0] https://lore.kernel.org/netdev/1470737769-30438-1-git-send-email-fw@strlen.de/
> 
> -- 
> Sabrina