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@xxxxxxxxxxx> 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@xxxxxxxxx>
>
> 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