Re: [PATCH bpf-next v1] bpf: Use hlist_nulls_replace_rcu() when updating htab elements

[luoxuanqiang]

在 2026/5/27 16:06, bot+bpf-ci@xxxxxxxxxx 写道:
> >      bpf: Use hlist_nulls_replace_rcu() when updating htab elements
> >
> >      When updating an existing element, the current code does
> >      hlist_nulls_add_head_rcu() followed by hlist_nulls_del_rcu(), exposing
> >      a transient state where both old and new elements are visible to
> >      concurrent RCU traversals.
> >
> >      Use hlist_nulls_replace_rcu() to atomically replace the old element in
> >      place, so RCU readers always see a consistent hash chain.
> >
> >      Signed-off-by: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>
> This looks like a bug fix that should include:
>
>    Fixes: 4fe8435909fd ("bpf: convert htab map to hlist_nulls")
>
> The bug was introduced by commit 4fe8435909fd which added the
> hlist_nulls_add_head_rcu() followed by hlist_nulls_del_rcu() pattern,
> exposing a transient state where both old and new elements are visible
> to concurrent RCU traversals.
>
> > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > index 3dd9b4924ae4..b20e82bd253d 100644
> > --- a/kernel/bpf/hashtab.c
> > +++ b/kernel/bpf/hashtab.c
> > @@ -1246,18 +1246,16 @@ static long htab_map_update_elem(struct bpf_map *map, void *key, void *value,
> >   		goto err;
> >   	}
> >
> > -	/* add new element to the head of the list, so that
> > -	 * concurrent search will find it before old elem
> > -	 */
> > -	hlist_nulls_add_head_rcu(&l_new->hash_node, head);
> >   	if (l_old) {
> > -		hlist_nulls_del_rcu(&l_old->hash_node);
> > +		hlist_nulls_replace_rcu(&l_old->hash_node, &l_new->hash_node);
> >
> >   		/* l_old has already been stashed in htab->extra_elems, free
> >   		 * its special fields before it is available for reuse.
> >   		 */
> >   		if (htab_is_prealloc(htab))
> >   			check_and_free_fields(htab, l_old);
> > +	} else {
> > +		hlist_nulls_add_head_rcu(&l_new->hash_node, head);
> >   	}
> Does this atomic replacement approach actually solve the lookup problem?
>
> During review of Hou Tao's earlier v1 patch that used the same approach,
> Alexei Starovoitov raised a fundamental concern:
>
>    "This atomic replacement doesn't really solve anything. lookup will see
>     one element. That element could be deleted by another thread. bucket
>     lock and either two step update or single step don't change anything
>     from the pov of bpf prog doing lookup."
>
> Reference: https://lore.kernel.org/bpf/CAADnVQKD94q-G4N=w9PJU+k6gPhM8GmUYcyfj=33B_mKX6Qbjw@xxxxxxxxxxxxxx/
>
> Hou Tao acknowledged this limitation and abandoned this approach in v2,
> instead implementing atomic updates only for htab of maps. This patch
> appears to be a re-implementation of the abandoned v1 approach without
> addressing the fundamental concerns raised during that review.
>
> Can you clarify how this approach addresses the concerns that led to
> abandonment of the similar change in Hou Tao's earlier patch series?
>
> [ ... ]
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/26496972451
I read the earlier discussion and realized I missed it, and I also missed
Hou Tao's updated patch 2c304172e0319 ("bpf: Support atomic update for htab
of maps") which already addressed this.

Sorry, My patch is outdated, please ignore it.

Thanks.