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

[Steffen Klassert <steffen.klassert@secunet.com>]

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?