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

[Sabrina Dubroca <sd@queasysnail.net>]

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.

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

-- 
Sabrina