[PATCH ipsec-next 00/10] xfrm: fix most sparse warnings

[PATCH ipsec-next 00/10] xfrm: fix most sparse warnings

[Sabrina Dubroca]

This series fixes most of the sparse warnings currently reported about
RCU pointers for files under net/xfrm. There's no actual bug in the
current code, we only need to use the correct helpers in each context.

Sabrina Dubroca (10):
  xfrm: state: fix sparse warnings on xfrm_state_hold_rcu
  xfrm: state: fix sparse warnings in xfrm_state_init
  xfrm: state: fix sparse warnings around XFRM_STATE_INSERT
  xfrm: state: add xfrm_state_deref_prot to state_by* walk under lock
  xfrm: remove rcu/state_hold from xfrm_state_lookup_spi_proto
  xfrm: state: silence sparse warnings during netns exit
  xfrm: policy: fix sparse warnings in xfrm_policy_{init,fini}
  xfrm: policy: silence sparse warning in xfrm_policy_unregister_afinfo
  xfrm: add rcu_access_pointer to silence sparse warning for
    xfrm_input_afinfo
  xfrm: avoid RCU warnings around the per-netns netlink socket

 include/net/netns/xfrm.h |   2 +-
 net/xfrm/xfrm_input.c    |   5 +-
 net/xfrm/xfrm_policy.c   |  10 ++--
 net/xfrm/xfrm_state.c    | 115 +++++++++++++++++++++------------------
 net/xfrm/xfrm_user.c     |  25 ++++++---
 5 files changed, 90 insertions(+), 67 deletions(-)

-- 
2.51.2

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

[Sabrina Dubroca]

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);
 }
-- 
2.51.2

[PATCH ipsec-next 02/10] xfrm: state: fix sparse warnings in xfrm_state_init

[Sabrina Dubroca]

Use rcu_assign_pointer, and tmp variables for freeing on the error
path without accessing net->xfrm.state_by*.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/xfrm/xfrm_state.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 7a68c594ce37..e9bf160a30f8 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -3258,6 +3258,7 @@ EXPORT_SYMBOL(xfrm_init_state);
 
 int __net_init xfrm_state_init(struct net *net)
 {
+	struct hlist_head *ndst, *nsrc, *nspi, *nseq;
 	unsigned int sz;
 
 	if (net_eq(net, &init_net))
@@ -3268,18 +3269,25 @@ int __net_init xfrm_state_init(struct net *net)
 
 	sz = sizeof(struct hlist_head) * 8;
 
-	net->xfrm.state_bydst = xfrm_hash_alloc(sz);
-	if (!net->xfrm.state_bydst)
+	ndst = xfrm_hash_alloc(sz);
+	if (!ndst)
 		goto out_bydst;
-	net->xfrm.state_bysrc = xfrm_hash_alloc(sz);
-	if (!net->xfrm.state_bysrc)
+	rcu_assign_pointer(net->xfrm.state_bydst, ndst);
+
+	nsrc = xfrm_hash_alloc(sz);
+	if (!nsrc)
 		goto out_bysrc;
-	net->xfrm.state_byspi = xfrm_hash_alloc(sz);
-	if (!net->xfrm.state_byspi)
+	rcu_assign_pointer(net->xfrm.state_bysrc, nsrc);
+
+	nspi = xfrm_hash_alloc(sz);
+	if (!nspi)
 		goto out_byspi;
-	net->xfrm.state_byseq = xfrm_hash_alloc(sz);
-	if (!net->xfrm.state_byseq)
+	rcu_assign_pointer(net->xfrm.state_byspi, nspi);
+
+	nseq = xfrm_hash_alloc(sz);
+	if (!nseq)
 		goto out_byseq;
+	rcu_assign_pointer(net->xfrm.state_byseq, nseq);
 
 	net->xfrm.state_cache_input = alloc_percpu(struct hlist_head);
 	if (!net->xfrm.state_cache_input)
@@ -3295,13 +3303,13 @@ int __net_init xfrm_state_init(struct net *net)
 	return 0;
 
 out_state_cache_input:
-	xfrm_hash_free(net->xfrm.state_byseq, sz);
+	xfrm_hash_free(nseq, sz);
 out_byseq:
-	xfrm_hash_free(net->xfrm.state_byspi, sz);
+	xfrm_hash_free(nspi, sz);
 out_byspi:
-	xfrm_hash_free(net->xfrm.state_bysrc, sz);
+	xfrm_hash_free(nsrc, sz);
 out_bysrc:
-	xfrm_hash_free(net->xfrm.state_bydst, sz);
+	xfrm_hash_free(ndst, sz);
 out_bydst:
 	return -ENOMEM;
 }
-- 
2.51.2

[PATCH ipsec-next 03/10] xfrm: state: fix sparse warnings around XFRM_STATE_INSERT

[Sabrina Dubroca]

We're under xfrm_state_lock in all those cases, use
xfrm_state_deref_prot(state_by*) to avoid sparse warnings:

net/xfrm/xfrm_state.c:2597:25: warning: cast removes address space '__rcu' of expression
net/xfrm/xfrm_state.c:2597:25: warning: incorrect type in argument 2 (different address spaces)
net/xfrm/xfrm_state.c:2597:25:    expected struct hlist_head *h
net/xfrm/xfrm_state.c:2597:25:    got struct hlist_head [noderef] __rcu *

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/xfrm/xfrm_state.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index e9bf160a30f8..3ad25b85cd92 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1563,23 +1563,23 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 			list_add(&x->km.all, &net->xfrm.state_all);
 			h = xfrm_dst_hash(net, daddr, saddr, tmpl->reqid, encap_family);
 			XFRM_STATE_INSERT(bydst, &x->bydst,
-					  net->xfrm.state_bydst + h,
+					  xfrm_state_deref_prot(net->xfrm.state_bydst, net) + h,
 					  x->xso.type);
 			h = xfrm_src_hash(net, daddr, saddr, encap_family);
 			XFRM_STATE_INSERT(bysrc, &x->bysrc,
-					  net->xfrm.state_bysrc + h,
+					  xfrm_state_deref_prot(net->xfrm.state_bysrc, net) + h,
 					  x->xso.type);
 			INIT_HLIST_NODE(&x->state_cache);
 			if (x->id.spi) {
 				h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, encap_family);
 				XFRM_STATE_INSERT(byspi, &x->byspi,
-						  net->xfrm.state_byspi + h,
+						  xfrm_state_deref_prot(net->xfrm.state_byspi, net) + h,
 						  x->xso.type);
 			}
 			if (x->km.seq) {
 				h = xfrm_seq_hash(net, x->km.seq);
 				XFRM_STATE_INSERT(byseq, &x->byseq,
-						  net->xfrm.state_byseq + h,
+						  xfrm_state_deref_prot(net->xfrm.state_byseq, net) + h,
 						  x->xso.type);
 			}
 			x->lft.hard_add_expires_seconds = net->xfrm.sysctl_acq_expires;
@@ -1730,25 +1730,29 @@ static void __xfrm_state_insert(struct xfrm_state *x)
 
 	h = xfrm_dst_hash(net, &x->id.daddr, &x->props.saddr,
 			  x->props.reqid, x->props.family);
-	XFRM_STATE_INSERT(bydst, &x->bydst, net->xfrm.state_bydst + h,
+	XFRM_STATE_INSERT(bydst, &x->bydst,
+			  xfrm_state_deref_prot(net->xfrm.state_bydst, net) + h,
 			  x->xso.type);
 
 	h = xfrm_src_hash(net, &x->id.daddr, &x->props.saddr, x->props.family);
-	XFRM_STATE_INSERT(bysrc, &x->bysrc, net->xfrm.state_bysrc + h,
+	XFRM_STATE_INSERT(bysrc, &x->bysrc,
+			  xfrm_state_deref_prot(net->xfrm.state_bysrc, net) + h,
 			  x->xso.type);
 
 	if (x->id.spi) {
 		h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto,
 				  x->props.family);
 
-		XFRM_STATE_INSERT(byspi, &x->byspi, net->xfrm.state_byspi + h,
+		XFRM_STATE_INSERT(byspi, &x->byspi,
+				  xfrm_state_deref_prot(net->xfrm.state_byspi, net) + h,
 				  x->xso.type);
 	}
 
 	if (x->km.seq) {
 		h = xfrm_seq_hash(net, x->km.seq);
 
-		XFRM_STATE_INSERT(byseq, &x->byseq, net->xfrm.state_byseq + h,
+		XFRM_STATE_INSERT(byseq, &x->byseq,
+				  xfrm_state_deref_prot(net->xfrm.state_byseq, net) + h,
 				  x->xso.type);
 	}
 
@@ -1868,10 +1872,12 @@ static struct xfrm_state *__find_acq_core(struct net *net,
 			      ktime_set(net->xfrm.sysctl_acq_expires, 0),
 			      HRTIMER_MODE_REL_SOFT);
 		list_add(&x->km.all, &net->xfrm.state_all);
-		XFRM_STATE_INSERT(bydst, &x->bydst, net->xfrm.state_bydst + h,
+		XFRM_STATE_INSERT(bydst, &x->bydst,
+				  xfrm_state_deref_prot(net->xfrm.state_bydst, net) + h,
 				  x->xso.type);
 		h = xfrm_src_hash(net, daddr, saddr, family);
-		XFRM_STATE_INSERT(bysrc, &x->bysrc, net->xfrm.state_bysrc + h,
+		XFRM_STATE_INSERT(bysrc, &x->bysrc,
+				  xfrm_state_deref_prot(net->xfrm.state_bysrc, net) + h,
 				  x->xso.type);
 
 		net->xfrm.state_num++;
@@ -2602,7 +2608,9 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
 		if (!x0) {
 			x->id.spi = newspi;
 			h = xfrm_spi_hash(net, &x->id.daddr, newspi, x->id.proto, x->props.family);
-			XFRM_STATE_INSERT(byspi, &x->byspi, net->xfrm.state_byspi + h, x->xso.type);
+			XFRM_STATE_INSERT(byspi, &x->byspi,
+					  xfrm_state_deref_prot(net->xfrm.state_byspi, net) + h,
+					  x->xso.type);
 			spin_unlock_bh(&net->xfrm.xfrm_state_lock);
 			err = 0;
 			goto unlock;
-- 
2.51.2

[PATCH ipsec-next 04/10] xfrm: state: add xfrm_state_deref_prot to state_by* walk under lock

[Sabrina Dubroca]

We're under xfrm_state_lock for all those walks, we can use
xfrm_state_deref_prot to silence sparse warnings such as:

net/xfrm/xfrm_state.c:933:17: warning: dereference of noderef expression

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/xfrm/xfrm_state.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 3ad25b85cd92..c234431e1967 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -870,7 +870,7 @@ xfrm_state_flush_secctx_check(struct net *net, u8 proto, bool task_valid)
 	for (i = 0; i <= net->xfrm.state_hmask; i++) {
 		struct xfrm_state *x;
 
-		hlist_for_each_entry(x, net->xfrm.state_bydst+i, bydst) {
+		hlist_for_each_entry(x, xfrm_state_deref_prot(net->xfrm.state_bydst, net) + i, bydst) {
 			if (xfrm_id_proto_match(x->id.proto, proto) &&
 			   (err = security_xfrm_state_delete(x)) != 0) {
 				xfrm_audit_state_delete(x, 0, task_valid);
@@ -891,7 +891,7 @@ xfrm_dev_state_flush_secctx_check(struct net *net, struct net_device *dev, bool
 		struct xfrm_state *x;
 		struct xfrm_dev_offload *xso;
 
-		hlist_for_each_entry(x, net->xfrm.state_bydst+i, bydst) {
+		hlist_for_each_entry(x, xfrm_state_deref_prot(net->xfrm.state_bydst, net) + i, bydst) {
 			xso = &x->xso;
 
 			if (xso->dev == dev &&
@@ -931,7 +931,7 @@ int xfrm_state_flush(struct net *net, u8 proto, bool task_valid)
 	for (i = 0; i <= net->xfrm.state_hmask; i++) {
 		struct xfrm_state *x;
 restart:
-		hlist_for_each_entry(x, net->xfrm.state_bydst+i, bydst) {
+		hlist_for_each_entry(x, xfrm_state_deref_prot(net->xfrm.state_bydst, net) + i, bydst) {
 			if (!xfrm_state_kern(x) &&
 			    xfrm_id_proto_match(x->id.proto, proto)) {
 				xfrm_state_hold(x);
@@ -973,7 +973,7 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
 	err = -ESRCH;
 	for (i = 0; i <= net->xfrm.state_hmask; i++) {
 restart:
-		hlist_for_each_entry(x, net->xfrm.state_bydst+i, bydst) {
+		hlist_for_each_entry(x, xfrm_state_deref_prot(net->xfrm.state_bydst, net) + i, bydst) {
 			xso = &x->xso;
 
 			if (!xfrm_state_kern(x) && xso->dev == dev) {
@@ -1652,7 +1652,7 @@ xfrm_stateonly_find(struct net *net, u32 mark, u32 if_id,
 
 	spin_lock_bh(&net->xfrm.xfrm_state_lock);
 	h = xfrm_dst_hash(net, daddr, saddr, reqid, family);
-	hlist_for_each_entry(x, net->xfrm.state_bydst+h, bydst) {
+	hlist_for_each_entry(x, xfrm_state_deref_prot(net->xfrm.state_bydst, net) + h, bydst) {
 		if (x->props.family == family &&
 		    x->props.reqid == reqid &&
 		    (mark & x->mark.m) == x->mark.v &&
@@ -1779,7 +1779,7 @@ static void __xfrm_state_bump_genids(struct xfrm_state *xnew)
 	u32 cpu_id = xnew->pcpu_num;
 
 	h = xfrm_dst_hash(net, &xnew->id.daddr, &xnew->props.saddr, reqid, family);
-	hlist_for_each_entry(x, net->xfrm.state_bydst+h, bydst) {
+	hlist_for_each_entry(x, xfrm_state_deref_prot(net->xfrm.state_bydst, net) + h, bydst) {
 		if (x->props.family	== family &&
 		    x->props.reqid	== reqid &&
 		    x->if_id		== if_id &&
@@ -1815,7 +1815,7 @@ static struct xfrm_state *__find_acq_core(struct net *net,
 	struct xfrm_state *x;
 	u32 mark = m->v & m->m;
 
-	hlist_for_each_entry(x, net->xfrm.state_bydst+h, bydst) {
+	hlist_for_each_entry(x, xfrm_state_deref_prot(net->xfrm.state_bydst, net) + h, bydst) {
 		if (x->props.reqid  != reqid ||
 		    x->props.mode   != mode ||
 		    x->props.family != family ||
@@ -2097,7 +2097,7 @@ struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *n
 	if (m->reqid) {
 		h = xfrm_dst_hash(net, &m->old_daddr, &m->old_saddr,
 				  m->reqid, m->old_family);
-		hlist_for_each_entry(x, net->xfrm.state_bydst+h, bydst) {
+		hlist_for_each_entry(x, xfrm_state_deref_prot(net->xfrm.state_bydst, net) + h, bydst) {
 			if (x->props.mode != m->mode ||
 			    x->id.proto != m->proto)
 				continue;
@@ -2116,7 +2116,7 @@ struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *n
 	} else {
 		h = xfrm_src_hash(net, &m->old_daddr, &m->old_saddr,
 				  m->old_family);
-		hlist_for_each_entry(x, net->xfrm.state_bysrc+h, bysrc) {
+		hlist_for_each_entry(x, xfrm_state_deref_prot(net->xfrm.state_bysrc, net) + h, bysrc) {
 			if (x->props.mode != m->mode ||
 			    x->id.proto != m->proto)
 				continue;
@@ -2318,7 +2318,7 @@ void xfrm_state_update_stats(struct net *net)
 
 	spin_lock_bh(&net->xfrm.xfrm_state_lock);
 	for (i = 0; i <= net->xfrm.state_hmask; i++) {
-		hlist_for_each_entry(x, net->xfrm.state_bydst + i, bydst)
+		hlist_for_each_entry(x, xfrm_state_deref_prot(net->xfrm.state_bydst, net) + i, bydst)
 			xfrm_dev_state_update_stats(x);
 	}
 	spin_unlock_bh(&net->xfrm.xfrm_state_lock);
@@ -2509,7 +2509,7 @@ static struct xfrm_state *__xfrm_find_acq_byseq(struct net *net, u32 mark, u32 s
 	unsigned int h = xfrm_seq_hash(net, seq);
 	struct xfrm_state *x;
 
-	hlist_for_each_entry_rcu(x, net->xfrm.state_byseq + h, byseq) {
+	hlist_for_each_entry(x, xfrm_state_deref_prot(net->xfrm.state_byseq, net) + h, byseq) {
 		if (x->km.seq == seq &&
 		    (mark & x->mark.m) == x->mark.v &&
 		    x->pcpu_num == pcpu_num &&
-- 
2.51.2

[PATCH ipsec-next 05/10] xfrm: remove rcu/state_hold from xfrm_state_lookup_spi_proto

[Sabrina Dubroca]

xfrm_state_lookup_spi_proto is called under xfrm_state_lock by
xfrm_alloc_spi, no need to take a reference on the state and pretend
to be under RCU.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/xfrm/xfrm_state.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index c234431e1967..ff421fb73df5 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1703,18 +1703,12 @@ static struct xfrm_state *xfrm_state_lookup_spi_proto(struct net *net, __be32 sp
 	struct xfrm_state *x;
 	unsigned int i;
 
-	rcu_read_lock();
 	for (i = 0; i <= net->xfrm.state_hmask; i++) {
-		hlist_for_each_entry_rcu(x, &net->xfrm.state_byspi[i], byspi) {
-			if (x->id.spi == spi && x->id.proto == proto) {
-				if (!xfrm_state_hold_rcu(x))
-					continue;
-				rcu_read_unlock();
+		hlist_for_each_entry(x, xfrm_state_deref_prot(net->xfrm.state_byspi, net) + i, byspi) {
+			if (x->id.spi == spi && x->id.proto == proto)
 				return x;
-			}
 		}
 	}
-	rcu_read_unlock();
 	return NULL;
 }
 
@@ -2615,7 +2609,6 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
 			err = 0;
 			goto unlock;
 		}
-		xfrm_state_put(x0);
 		spin_unlock_bh(&net->xfrm.xfrm_state_lock);
 
 next:
-- 
2.51.2

[PATCH ipsec-next 06/10] xfrm: state: silence sparse warnings during netns exit

[Sabrina Dubroca]

Silence sparse warnings in xfrm_state_fini:
net/xfrm/xfrm_state.c:3327:9: warning: incorrect type in argument 1 (different address spaces)
net/xfrm/xfrm_state.c:3327:9:    expected struct hlist_head const *h
net/xfrm/xfrm_state.c:3327:9:    got struct hlist_head [noderef] __rcu *state_byseq

Add xfrm_state_deref_netexit() to wrap those calls. The netns is going
away, we don't have to worry about the state_by* pointers being
changed behind our backs.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/xfrm/xfrm_state.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index ff421fb73df5..5b29d770a3fa 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -3315,6 +3315,8 @@ int __net_init xfrm_state_init(struct net *net)
 	return -ENOMEM;
 }
 
+#define xfrm_state_deref_netexit(table) \
+	rcu_dereference_protected((table), true /* netns is going away */)
 void xfrm_state_fini(struct net *net)
 {
 	unsigned int sz;
@@ -3327,17 +3329,17 @@ void xfrm_state_fini(struct net *net)
 	WARN_ON(!list_empty(&net->xfrm.state_all));
 
 	for (i = 0; i <= net->xfrm.state_hmask; i++) {
-		WARN_ON(!hlist_empty(net->xfrm.state_byseq + i));
-		WARN_ON(!hlist_empty(net->xfrm.state_byspi + i));
-		WARN_ON(!hlist_empty(net->xfrm.state_bysrc + i));
-		WARN_ON(!hlist_empty(net->xfrm.state_bydst + i));
+		WARN_ON(!hlist_empty(xfrm_state_deref_netexit(net->xfrm.state_byseq) + i));
+		WARN_ON(!hlist_empty(xfrm_state_deref_netexit(net->xfrm.state_byspi) + i));
+		WARN_ON(!hlist_empty(xfrm_state_deref_netexit(net->xfrm.state_bysrc) + i));
+		WARN_ON(!hlist_empty(xfrm_state_deref_netexit(net->xfrm.state_bydst) + i));
 	}
 
 	sz = (net->xfrm.state_hmask + 1) * sizeof(struct hlist_head);
-	xfrm_hash_free(net->xfrm.state_byseq, sz);
-	xfrm_hash_free(net->xfrm.state_byspi, sz);
-	xfrm_hash_free(net->xfrm.state_bysrc, sz);
-	xfrm_hash_free(net->xfrm.state_bydst, sz);
+	xfrm_hash_free(xfrm_state_deref_netexit(net->xfrm.state_byseq), sz);
+	xfrm_hash_free(xfrm_state_deref_netexit(net->xfrm.state_byspi), sz);
+	xfrm_hash_free(xfrm_state_deref_netexit(net->xfrm.state_bysrc), sz);
+	xfrm_hash_free(xfrm_state_deref_netexit(net->xfrm.state_bydst), sz);
 	free_percpu(net->xfrm.state_cache_input);
 }
 
-- 
2.51.2

[PATCH ipsec-next 07/10] xfrm: policy: fix sparse warnings in xfrm_policy_{init,fini}

[Sabrina Dubroca]

In xfrm_policy_init:
add rcu_assign_pointer to fix warning:
net/xfrm/xfrm_policy.c:4238:29: warning: incorrect type in assignment (different address spaces)
net/xfrm/xfrm_policy.c:4238:29:    expected struct hlist_head [noderef] __rcu *table
net/xfrm/xfrm_policy.c:4238:29:    got struct hlist_head *

add rcu_dereference_protected to silence warning:
net/xfrm/xfrm_policy.c:4265:36: warning: incorrect type in argument 1 (different address spaces)
net/xfrm/xfrm_policy.c:4265:36:    expected struct hlist_head *n
net/xfrm/xfrm_policy.c:4265:36:    got struct hlist_head [noderef] __rcu *table

The netns is being created, no concurrent access is possible yet.

In xfrm_policy_fini, net is going away, there shouldn't be any
concurrent changes to the hashtables, so we can use
rcu_dereference_protected to silence warnings:
net/xfrm/xfrm_policy.c:4291:17: warning: incorrect type in argument 1 (different address spaces)
net/xfrm/xfrm_policy.c:4291:17:    expected struct hlist_head const *h
net/xfrm/xfrm_policy.c:4291:17:    got struct hlist_head [noderef] __rcu *table
net/xfrm/xfrm_policy.c:4292:36: warning: incorrect type in argument 1 (different address spaces)
net/xfrm/xfrm_policy.c:4292:36:    expected struct hlist_head *n
net/xfrm/xfrm_policy.c:4292:36:    got struct hlist_head [noderef] __rcu *table

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/xfrm/xfrm_policy.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 7bcb6583e84c..9b8d37502de3 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -4235,7 +4235,7 @@ static int __net_init xfrm_policy_init(struct net *net)
 		net->xfrm.policy_count[XFRM_POLICY_MAX + dir] = 0;
 
 		htab = &net->xfrm.policy_bydst[dir];
-		htab->table = xfrm_hash_alloc(sz);
+		rcu_assign_pointer(htab->table, xfrm_hash_alloc(sz));
 		if (!htab->table)
 			goto out_bydst;
 		htab->hmask = hmask;
@@ -4262,7 +4262,7 @@ static int __net_init xfrm_policy_init(struct net *net)
 		struct xfrm_policy_hash *htab;
 
 		htab = &net->xfrm.policy_bydst[dir];
-		xfrm_hash_free(htab->table, sz);
+		xfrm_hash_free(rcu_dereference_protected(htab->table, true), sz);
 	}
 	xfrm_hash_free(net->xfrm.policy_byidx, sz);
 out_byidx:
@@ -4288,8 +4288,8 @@ static void xfrm_policy_fini(struct net *net)
 
 		htab = &net->xfrm.policy_bydst[dir];
 		sz = (htab->hmask + 1) * sizeof(struct hlist_head);
-		WARN_ON(!hlist_empty(htab->table));
-		xfrm_hash_free(htab->table, sz);
+		WARN_ON(!hlist_empty(rcu_dereference_protected(htab->table, true)));
+		xfrm_hash_free(rcu_dereference_protected(htab->table, true), sz);
 	}
 
 	sz = (net->xfrm.policy_idx_hmask + 1) * sizeof(struct hlist_head);
-- 
2.51.2

[PATCH ipsec-next 08/10] xfrm: policy: silence sparse warning in xfrm_policy_unregister_afinfo

[Sabrina Dubroca]

xfrm_policy_afinfo is __rcu, use rcu_access_pointer to silence:

net/xfrm/xfrm_policy.c:4152:43: error: incompatible types in comparison expression (different address spaces):
net/xfrm/xfrm_policy.c:4152:43:    struct xfrm_policy_afinfo const [noderef] __rcu *
net/xfrm/xfrm_policy.c:4152:43:    struct xfrm_policy_afinfo const *

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/xfrm/xfrm_policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 9b8d37502de3..93fb47472390 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -4149,7 +4149,7 @@ void xfrm_policy_unregister_afinfo(const struct xfrm_policy_afinfo *afinfo)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(xfrm_policy_afinfo); i++) {
-		if (xfrm_policy_afinfo[i] != afinfo)
+		if (rcu_access_pointer(xfrm_policy_afinfo[i]) != afinfo)
 			continue;
 		RCU_INIT_POINTER(xfrm_policy_afinfo[i], NULL);
 		break;
-- 
2.51.2

[PATCH ipsec-next 09/10] xfrm: add rcu_access_pointer to silence sparse warning for xfrm_input_afinfo

[Sabrina Dubroca]

xfrm_input_afinfo is __rcu, we should use rcu_access_pointer to avoid
a sparse warning:
net/xfrm/xfrm_input.c:78:21: error: incompatible types in comparison expression (different address spaces):
net/xfrm/xfrm_input.c:78:21:    struct xfrm_input_afinfo const [noderef] __rcu *
net/xfrm/xfrm_input.c:78:21:    struct xfrm_input_afinfo const *

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/xfrm/xfrm_input.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 4ed346e682c7..dc1312ed5a09 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -75,7 +75,10 @@ int xfrm_input_unregister_afinfo(const struct xfrm_input_afinfo *afinfo)
 
 	spin_lock_bh(&xfrm_input_afinfo_lock);
 	if (likely(xfrm_input_afinfo[afinfo->is_ipip][afinfo->family])) {
-		if (unlikely(xfrm_input_afinfo[afinfo->is_ipip][afinfo->family] != afinfo))
+		const struct xfrm_input_afinfo *cur;
+
+		cur = rcu_access_pointer(xfrm_input_afinfo[afinfo->is_ipip][afinfo->family]);
+		if (unlikely(cur != afinfo))
 			err = -EINVAL;
 		else
 			RCU_INIT_POINTER(xfrm_input_afinfo[afinfo->is_ipip][afinfo->family], NULL);
-- 
2.51.2

[PATCH ipsec-next 10/10] xfrm: avoid RCU warnings around the per-netns netlink socket

[Sabrina Dubroca]

net->xfrm.nlsk is used in 2 types of contexts:
 - fully under RCU, with rcu_read_lock + rcu_dereference and a NULL check
 - in the netlink handlers, with requests coming from a userspace socket

In the 2nd case, net->xfrm.nlsk is guaranteed to stay non-NULL and the
object is alive, since we can't enter the netns destruction path while
the user socket holds a reference on the netns.

After adding the __rcu annotation to netns_xfrm.nlsk (which silences
sparse warnings in the RCU users and __net_init code), we need to tell
sparse that the 2nd case is safe. Add a helper for that.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 include/net/netns/xfrm.h |  2 +-
 net/xfrm/xfrm_user.c     | 25 +++++++++++++++++--------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
index 23dd647fe024..b73983a17e08 100644
--- a/include/net/netns/xfrm.h
+++ b/include/net/netns/xfrm.h
@@ -59,7 +59,7 @@ struct netns_xfrm {
 	struct list_head	inexact_bins;
 
 
-	struct sock		*nlsk;
+	struct sock		__rcu *nlsk;
 	struct sock		*nlsk_stash;
 
 	u32			sysctl_aevent_etime;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 403b5ecac2c5..88fdbdc099a3 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -35,6 +35,15 @@
 #endif
 #include <linux/unaligned.h>
 
+static struct sock *xfrm_net_nlsk(const struct net *net, const struct sk_buff *skb)
+{
+	/* get the source of this request, see netlink_unicast_kernel */
+	const struct sock *sk = NETLINK_CB(skb).sk;
+
+	/* sk is refcounted, the netns stays alive and nlsk with it */
+	return rcu_dereference_protected(net->xfrm.nlsk, sk->sk_net_refcnt);
+}
+
 static int verify_one_alg(struct nlattr **attrs, enum xfrm_attr_type_t type,
 			  struct netlink_ext_ack *extack)
 {
@@ -1727,7 +1736,7 @@ static int xfrm_get_spdinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = build_spdinfo(r_skb, net, sportid, seq, *flags);
 	BUG_ON(err < 0);
 
-	return nlmsg_unicast(net->xfrm.nlsk, r_skb, sportid);
+	return nlmsg_unicast(xfrm_net_nlsk(net, skb), r_skb, sportid);
 }
 
 static inline unsigned int xfrm_sadinfo_msgsize(void)
@@ -1787,7 +1796,7 @@ static int xfrm_get_sadinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = build_sadinfo(r_skb, net, sportid, seq, *flags);
 	BUG_ON(err < 0);
 
-	return nlmsg_unicast(net->xfrm.nlsk, r_skb, sportid);
+	return nlmsg_unicast(xfrm_net_nlsk(net, skb), r_skb, sportid);
 }
 
 static int xfrm_get_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
@@ -1807,7 +1816,7 @@ static int xfrm_get_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (IS_ERR(resp_skb)) {
 		err = PTR_ERR(resp_skb);
 	} else {
-		err = nlmsg_unicast(net->xfrm.nlsk, resp_skb, NETLINK_CB(skb).portid);
+		err = nlmsg_unicast(xfrm_net_nlsk(net, skb), resp_skb, NETLINK_CB(skb).portid);
 	}
 	xfrm_state_put(x);
 out_noput:
@@ -1897,7 +1906,7 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 		}
 	}
 
-	err = nlmsg_unicast(net->xfrm.nlsk, resp_skb, NETLINK_CB(skb).portid);
+	err = nlmsg_unicast(xfrm_net_nlsk(net, skb), resp_skb, NETLINK_CB(skb).portid);
 
 out:
 	xfrm_state_put(x);
@@ -2542,7 +2551,7 @@ static int xfrm_get_default(struct sk_buff *skb, struct nlmsghdr *nlh,
 	r_up->out = net->xfrm.policy_default[XFRM_POLICY_OUT];
 	nlmsg_end(r_skb, r_nlh);
 
-	return nlmsg_unicast(net->xfrm.nlsk, r_skb, portid);
+	return nlmsg_unicast(xfrm_net_nlsk(net, skb), r_skb, portid);
 }
 
 static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
@@ -2608,7 +2617,7 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 		if (IS_ERR(resp_skb)) {
 			err = PTR_ERR(resp_skb);
 		} else {
-			err = nlmsg_unicast(net->xfrm.nlsk, resp_skb,
+			err = nlmsg_unicast(xfrm_net_nlsk(net, skb), resp_skb,
 					    NETLINK_CB(skb).portid);
 		}
 	} else {
@@ -2781,7 +2790,7 @@ static int xfrm_get_ae(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = build_aevent(r_skb, x, &c);
 	BUG_ON(err < 0);
 
-	err = nlmsg_unicast(net->xfrm.nlsk, r_skb, NETLINK_CB(skb).portid);
+	err = nlmsg_unicast(xfrm_net_nlsk(net, skb), r_skb, NETLINK_CB(skb).portid);
 	spin_unlock_bh(&x->lock);
 	xfrm_state_put(x);
 	return err;
@@ -3483,7 +3492,7 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 			goto err;
 		}
 
-		err = netlink_dump_start(net->xfrm.nlsk, skb, nlh, &c);
+		err = netlink_dump_start(xfrm_net_nlsk(net, skb), skb, nlh, &c);
 		goto err;
 	}
 
-- 
2.51.2

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

[Leon Romanovsky]

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

    932 static inline void xfrm_state_hold(struct xfrm_state *x)
    933 {
    934         refcount_inc(&x->refcnt);
    935 }

The function signatures differ, of course, but it still seems worth
considering whether they could be unified.

Thanks

> -- 
> 2.51.2
> 
> 

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

[Sabrina Dubroca]

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

Re: [PATCH ipsec-next 00/10] xfrm: fix most sparse warnings

[Simon Horman]

On Mon, Mar 09, 2026 at 11:32:33AM +0100, Sabrina Dubroca wrote:
> This series fixes most of the sparse warnings currently reported about
> RCU pointers for files under net/xfrm. There's no actual bug in the
> current code, we only need to use the correct helpers in each context.

Thanks Sabrina,

I'm really pleased to see this addressed.

Reviewed-by: Simon Horman <horms@kernel.org>

...

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

[Leon Romanovsky]

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

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

[Florian Westphal]

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.

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

[Leon Romanovsky]

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

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

[Florian Westphal]

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.

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

[Leon Romanovsky]

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.

Yes, this is possible scenario and this is what is worth to document.

Thanks

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

[Sabrina Dubroca]

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


We also have __xfrm_state_put, the commit message that introduced it
states:

    We often just do an atomic_dec(&x->refcnt) on an xfrm_state object
    because we know there is more than 1 reference remaining and thus
    we can elide the heavier xfrm_state_put() call.

so we could add:

/* Drop a reference to @x, when we know there is more than 1 reference remaining.
 * In this case, we can avoid refcount_dec_and_test and just decrement refcnt.
 */

but maybe someone has a better suggestion.


-- 
Sabrina

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

[Leon Romanovsky]

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

Thanks

> 
> 
> We also have __xfrm_state_put, the commit message that introduced it
> states:
> 
>     We often just do an atomic_dec(&x->refcnt) on an xfrm_state object
>     because we know there is more than 1 reference remaining and thus
>     we can elide the heavier xfrm_state_put() call.
> 
> so we could add:
> 
> /* Drop a reference to @x, when we know there is more than 1 reference remaining.
>  * In this case, we can avoid refcount_dec_and_test and just decrement refcnt.
>  */
> 
> but maybe someone has a better suggestion.
> 
> 
> -- 
> Sabrina

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

[Steffen Klassert]

On Tue, Mar 10, 2026 at 10:41:46PM +0100, Sabrina Dubroca wrote:
> 
> > 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.
> 
> 
> We also have __xfrm_state_put, the commit message that introduced it
> states:
> 
>     We often just do an atomic_dec(&x->refcnt) on an xfrm_state object
>     because we know there is more than 1 reference remaining and thus
>     we can elide the heavier xfrm_state_put() call.
> 
> so we could add:
> 
> /* Drop a reference to @x, when we know there is more than 1 reference remaining.
>  * In this case, we can avoid refcount_dec_and_test and just decrement refcnt.
>  */
> 
> but maybe someone has a better suggestion.

I plan to take this series as is. If you feel we need a comment,
just add it with a followup patch.

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

[Sabrina Dubroca]

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?

/* 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

Re: [PATCH ipsec-next 00/10] xfrm: fix most sparse warnings

[Steffen Klassert]

On Mon, Mar 09, 2026 at 11:32:33AM +0100, Sabrina Dubroca wrote:
> This series fixes most of the sparse warnings currently reported about
> RCU pointers for files under net/xfrm. There's no actual bug in the
> current code, we only need to use the correct helpers in each context.
> 
> Sabrina Dubroca (10):
>   xfrm: state: fix sparse warnings on xfrm_state_hold_rcu
>   xfrm: state: fix sparse warnings in xfrm_state_init
>   xfrm: state: fix sparse warnings around XFRM_STATE_INSERT
>   xfrm: state: add xfrm_state_deref_prot to state_by* walk under lock
>   xfrm: remove rcu/state_hold from xfrm_state_lookup_spi_proto
>   xfrm: state: silence sparse warnings during netns exit
>   xfrm: policy: fix sparse warnings in xfrm_policy_{init,fini}
>   xfrm: policy: silence sparse warning in xfrm_policy_unregister_afinfo
>   xfrm: add rcu_access_pointer to silence sparse warning for
>     xfrm_input_afinfo
>   xfrm: avoid RCU warnings around the per-netns netlink socket

Series applied, thanks a ot Sabrina!

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

[Leon Romanovsky]

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

Re: [PATCH ipsec-next 00/10] xfrm: fix most sparse warnings

[Steffen Klassert]

On Fri, Mar 13, 2026 at 08:48:17AM +0100, Steffen Klassert wrote:
> On Mon, Mar 09, 2026 at 11:32:33AM +0100, Sabrina Dubroca wrote:
> > This series fixes most of the sparse warnings currently reported about
> > RCU pointers for files under net/xfrm. There's no actual bug in the
> > current code, we only need to use the correct helpers in each context.
> > 
> > Sabrina Dubroca (10):
> >   xfrm: state: fix sparse warnings on xfrm_state_hold_rcu
> >   xfrm: state: fix sparse warnings in xfrm_state_init
> >   xfrm: state: fix sparse warnings around XFRM_STATE_INSERT
> >   xfrm: state: add xfrm_state_deref_prot to state_by* walk under lock
> >   xfrm: remove rcu/state_hold from xfrm_state_lookup_spi_proto
> >   xfrm: state: silence sparse warnings during netns exit
> >   xfrm: policy: fix sparse warnings in xfrm_policy_{init,fini}
> >   xfrm: policy: silence sparse warning in xfrm_policy_unregister_afinfo
> >   xfrm: add rcu_access_pointer to silence sparse warning for
> >     xfrm_input_afinfo
> >   xfrm: avoid RCU warnings around the per-netns netlink socket
> 
> Series applied, thanks a ot Sabrina!

I forgot to mention, I've pulled this into the ipsec tree
as I consider this as fixes to get rid of these warnings
and the regression risk is pretty low.