[PATCH net] xfrm: retry inexact policy lookup after node reinsertion

[PATCH net] xfrm: retry inexact policy lookup after node reinsertion

[Chengfeng Ye]

When a newly inserted inexact policy covers multiple existing nodes,
xfrm_policy_inexact_node_merge() removes each policy from the old node with
hlist_del_rcu(). xfrm_policy_inexact_list_reinsert() then immediately adds
the same bydst node to another hlist. hlist_del_rcu() leaves ->next intact
for readers already at the removed entry, but the add overwrites it before
an RCU grace period.

This permits the following interleaving:

  CPU 0                                  CPU 1
  -----                                  -----
  reach policy P in the old hlist
                                         delete P from the old hlist
                                         add P to the new hlist
  follow P->next into the new hlist

The lookup can then skip a matching policy and make an incorrect IPsec
decision.

During concurrent policy insertion and route lookup, KCSAN reported:

  BUG: KCSAN: data-race in __xfrm_policy_link / xfrm_lookup_with_ifid
  write to ... by task 92 on cpu 0:
    __xfrm_policy_link
    xfrm_policy_insert
    xfrm_add_policy
  read to ... by task 91 on cpu 1:
    xfrm_lookup_with_ifid
    xfrm_lookup_route
    ip_route_output_flow

The per-bin sequence counter already brackets inexact tree changes,
including node merges. Snapshot it before candidate discovery and retry
after evaluating all candidate lists if it changed. This rejects results
from any traversal overlapping reinsertion while leaving stable lookup
order and locking unchanged.

Fixes: 9cf545ebd591 ("xfrm: policy: store inexact policies in a tree ordered by destination address")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 net/xfrm/xfrm_policy.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 932a313b9460..5d4e863863df 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2157,6 +2157,7 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
 	struct xfrm_pol_inexact_bin *bin;
 	struct xfrm_policy *pol, *ret;
 	struct hlist_head *chain;
+	unsigned int inexact_sequence;
 	unsigned int sequence;
 	int err;
 
@@ -2191,12 +2192,18 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
 		goto skip_inexact;
 
 	bin = xfrm_policy_inexact_lookup_rcu(net, type, family, dir, if_id);
-	if (!bin || !xfrm_policy_find_inexact_candidates(&cand, bin, saddr,
-							 daddr))
+	if (!bin)
+		goto skip_inexact;
+
+	inexact_sequence = read_seqcount_begin(&bin->count);
+	if (!xfrm_policy_find_inexact_candidates(&cand, bin, saddr, daddr))
 		goto skip_inexact;
 
 	pol = xfrm_policy_eval_candidates(&cand, ret, fl, type,
 					  family, if_id);
+	if (read_seqcount_retry(&bin->count, inexact_sequence))
+		goto retry;
+
 	if (pol) {
 		ret = pol;
 		if (IS_ERR(pol))
-- 
2.43.0

Re: [PATCH net] xfrm: retry inexact policy lookup after node reinsertion

[Steffen Klassert]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [1]: [PATCH net] xfrm: retry inexact policy lookup after node reinsertion ---
commit 55045c9b49d306c47b0ee8e568d224e0bca94a3c
Author: Chengfeng Ye <nicoyip.dev@gmail.com>

xfrm: retry inexact policy lookup after node reinsertion

This adds a snapshot of the per-bin sequence counter around inexact
candidate discovery and evaluation in xfrm_policy_lookup_bytype(), and
retries the whole lookup if the counter changed.  The stated goal is to
reject lookups that overlap the bydst hlist hop done during node merge and
reinsertion.

No Link: tags are present in the commit header.

> During concurrent policy insertion and route lookup, KCSAN reported:
>
>   BUG: KCSAN: data-race in __xfrm_policy_link / xfrm_lookup_with_ifid
>   write to ... by task 92 on cpu 0:
>     __xfrm_policy_link
>     xfrm_policy_insert
>     xfrm_add_policy
>   read to ... by task 91 on cpu 1:
>     xfrm_lookup_with_ifid
>     xfrm_lookup_route
>     ip_route_output_flow

Discovered by: opus-5 · Missed by: sonnet-5, gpt-5-6-sol, sashiko-gemini · Confirmed by: gpt-5-6-sol
Can this KCSAN report come from the code the patch changes?

__xfrm_policy_link() in net/xfrm/xfrm_policy.c only does this:

	switch (dir) {
	case XFRM_POLICY_IN:
	case XFRM_POLICY_FWD:
	case XFRM_POLICY_OUT:
		pol->pos = xfrm_gen_pos(net);
		break;
	}

	list_add(&pol->walk.all, &net->xfrm.policy_all);
	WRITE_ONCE(net->xfrm.policy_count[dir], net->xfrm.policy_count[dir] + 1);
	xfrm_pol_hold(pol);

The only one of those locations that xfrm_lookup_with_ifid() touches is
policy_count, and it reads it marked:

	if (!if_id && ((dst_orig->flags & DST_NOXFRM) ||
		       !READ_ONCE(net->xfrm.policy_count[XFRM_POLICY_OUT])))
		goto nopol;

Documentation/dev-tools/kcsan.rst states that if all concurrent accesses to
a variable are properly marked, KCSAN will never trigger a watchpoint and
therefore never report the accesses.  Both sides here are marked, so how
was this pair reported?

The write side the changelog describes in prose is a different one: the
bydst hop in xfrm_policy_inexact_node_merge():

	hlist_for_each_entry(tmp, &v->hhead, bydst) {
		tmp->bydst_reinsert = true;
		hlist_del_rcu(&tmp->bydst);
	}

	xfrm_policy_inexact_list_reinsert(net, n, family);

That path is reached via xfrm_policy_inexact_insert_node(), not via
__xfrm_policy_link(), and it also uses only marked writes
(hlist_del_rcu(), hlist_add_head_rcu(), hlist_add_behind_rcu()) against a
reader using hlist_for_each_entry_rcu().  So that pair would not be
reported by KCSAN either, which suggests the hazard being fixed is a
logical list-hop ordering problem between correctly marked RCU accesses
rather than a data race.

Could the changelog be reworded to describe the ordering problem directly,
and either drop the KCSAN splat or replace it with a report that actually
corresponds to the accesses this patch guards?  With Cc: stable in place,
the quoted evidence is what backporters will use to judge the fix.

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 932a313b9460a..5d4e863863dfe 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c

[ ... ]

> @@ -2191,12 +2192,18 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
>  		goto skip_inexact;
>
>  	bin = xfrm_policy_inexact_lookup_rcu(net, type, family, dir, if_id);
> -	if (!bin || !xfrm_policy_find_inexact_candidates(&cand, bin, saddr,
> -							 daddr))
> +	if (!bin)
> +		goto skip_inexact;
> +
> +	inexact_sequence = read_seqcount_begin(&bin->count);
> +	if (!xfrm_policy_find_inexact_candidates(&cand, bin, saddr, daddr))
>  		goto skip_inexact;
>
>  	pol = xfrm_policy_eval_candidates(&cand, ret, fl, type,
>  					  family, if_id);
> +	if (read_seqcount_retry(&bin->count, inexact_sequence))
> +		goto retry;
> +

Discovered by: opus-5 · Missed by: sonnet-5, gpt-5-6-sol, sashiko-gemini · Confirmed by: gpt-5-6-sol
Related to the above: this retry can only observe mutations that are
bracketed by write_seqcount_begin(&bin->count).  The chain updates done by
xfrm_policy_insert_list() and __xfrm_policy_unlink(), and the counter
update in __xfrm_policy_link() named in the quoted splat, do not run inside
any bin->count write section.

So even taken at face value, would this new read_seqcount_retry() have any
effect on the __xfrm_policy_link() versus xfrm_lookup_with_ifid() pair
quoted in the changelog?

Re: [PATCH net] xfrm: retry inexact policy lookup after node reinsertion

[Chengfeng Ye]

On Thu, Sep 3, 2026 at 3:32 PM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> --- Patch [1]: [PATCH net] xfrm: retry inexact policy lookup after node reinsertion ---
> commit 55045c9b49d306c47b0ee8e568d224e0bca94a3c
> Author: Chengfeng Ye <nicoyip.dev@gmail.com>
>
> xfrm: retry inexact policy lookup after node reinsertion
>
> This adds a snapshot of the per-bin sequence counter around inexact
> candidate discovery and evaluation in xfrm_policy_lookup_bytype(), and
> retries the whole lookup if the counter changed.  The stated goal is to
> reject lookups that overlap the bydst hlist hop done during node merge and
> reinsertion.
>
> No Link: tags are present in the commit header.
>
> > During concurrent policy insertion and route lookup, KCSAN reported:
> >
> >   BUG: KCSAN: data-race in __xfrm_policy_link / xfrm_lookup_with_ifid
> >   write to ... by task 92 on cpu 0:
> >     __xfrm_policy_link
> >     xfrm_policy_insert
> >     xfrm_add_policy
> >   read to ... by task 91 on cpu 1:
> >     xfrm_lookup_with_ifid
> >     xfrm_lookup_route
> >     ip_route_output_flow
>
> Discovered by: opus-5 · Missed by: sonnet-5, gpt-5-6-sol, sashiko-gemini · Confirmed by: gpt-5-6-sol
> Can this KCSAN report come from the code the patch changes?
>
> __xfrm_policy_link() in net/xfrm/xfrm_policy.c only does this:
>
>         switch (dir) {
>         case XFRM_POLICY_IN:
>         case XFRM_POLICY_FWD:
>         case XFRM_POLICY_OUT:
>                 pol->pos = xfrm_gen_pos(net);
>                 break;
>         }
>
>         list_add(&pol->walk.all, &net->xfrm.policy_all);
>         WRITE_ONCE(net->xfrm.policy_count[dir], net->xfrm.policy_count[dir] + 1);
>         xfrm_pol_hold(pol);
>
> The only one of those locations that xfrm_lookup_with_ifid() touches is
> policy_count, and it reads it marked:
>
>         if (!if_id && ((dst_orig->flags & DST_NOXFRM) ||
>                        !READ_ONCE(net->xfrm.policy_count[XFRM_POLICY_OUT])))
>                 goto nopol;
>
> Documentation/dev-tools/kcsan.rst states that if all concurrent accesses to
> a variable are properly marked, KCSAN will never trigger a watchpoint and
> therefore never report the accesses.  Both sides here are marked, so how
> was this pair reported?
>
> The write side the changelog describes in prose is a different one: the
> bydst hop in xfrm_policy_inexact_node_merge():
>
>         hlist_for_each_entry(tmp, &v->hhead, bydst) {
>                 tmp->bydst_reinsert = true;
>                 hlist_del_rcu(&tmp->bydst);
>         }
>
>         xfrm_policy_inexact_list_reinsert(net, n, family);
>
> That path is reached via xfrm_policy_inexact_insert_node(), not via
> __xfrm_policy_link(), and it also uses only marked writes
> (hlist_del_rcu(), hlist_add_head_rcu(), hlist_add_behind_rcu()) against a
> reader using hlist_for_each_entry_rcu().  So that pair would not be
> reported by KCSAN either, which suggests the hazard being fixed is a
> logical list-hop ordering problem between correctly marked RCU accesses
> rather than a data race.
>
> Could the changelog be reworded to describe the ordering problem directly,
> and either drop the KCSAN splat or replace it with a report that actually
> corresponds to the accesses this patch guards?  With Cc: stable in place,
> the quoted evidence is what backporters will use to judge the fix.
>
> > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > index 932a313b9460a..5d4e863863dfe 100644
> > --- a/net/xfrm/xfrm_policy.c
> > +++ b/net/xfrm/xfrm_policy.c
>
> [ ... ]
>
> > @@ -2191,12 +2192,18 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
> >               goto skip_inexact;
> >
> >       bin = xfrm_policy_inexact_lookup_rcu(net, type, family, dir, if_id);
> > -     if (!bin || !xfrm_policy_find_inexact_candidates(&cand, bin, saddr,
> > -                                                      daddr))
> > +     if (!bin)
> > +             goto skip_inexact;
> > +
> > +     inexact_sequence = read_seqcount_begin(&bin->count);
> > +     if (!xfrm_policy_find_inexact_candidates(&cand, bin, saddr, daddr))
> >               goto skip_inexact;
> >
> >       pol = xfrm_policy_eval_candidates(&cand, ret, fl, type,
> >                                         family, if_id);
> > +     if (read_seqcount_retry(&bin->count, inexact_sequence))
> > +             goto retry;
> > +
>
> Discovered by: opus-5 · Missed by: sonnet-5, gpt-5-6-sol, sashiko-gemini · Confirmed by: gpt-5-6-sol
> Related to the above: this retry can only observe mutations that are
> bracketed by write_seqcount_begin(&bin->count).  The chain updates done by
> xfrm_policy_insert_list() and __xfrm_policy_unlink(), and the counter
> update in __xfrm_policy_link() named in the quoted splat, do not run inside
> any bin->count write section.
>
> So even taken at face value, would this new read_seqcount_retry() have any
> effect on the __xfrm_policy_link() versus xfrm_lookup_with_ifid() pair
> quoted in the changelog?


Hi Steffen,

The review is correct. While generating the changelog I mistakenly
pointed the agent to the KCSAN report triggered for the data race
already fixed by 66817a979426, and I'm sorry for the mistake. I will
send a v2 to correct the changelog.

Best regards,
Chengfeng