Re: [PATCH] bpf: Clear rb node linkage when freeing bpf_rb_root

[Yonghong Song]

On 5/31/26 10:58 PM, Kaitao Cheng wrote:
> From: Kaitao Cheng <chengkaitao@xxxxxxxxxx>
>
> bpf_rb_root_free() detaches the root by copying the current rb_root_cached
> and then replacing the live root with RB_ROOT_CACHED. It then walks the
> copied root and drops each object contained in the tree.
>
> This leaves the rb node state intact while dropping the object. If the
> object is refcounted and survives the drop, its bpf_rb_node_kern still
> contains an owner pointer to the freed root and stale rb tree linkage. If
> a later bpf_rb_root allocation reuses the same address, bpf_rbtree_remove()
> can incorrectly pass the owner check and call rb_erase_cached() on a node
> whose rb pointers belong to the old tree.
>
> Mirror the list draining behavior by marking nodes as busy while the root
> is being detached, then clear the rb node and release the owner before
> dropping the containing object. This makes surviving nodes unowned and
> safe to reject from remove or accept for a later add.
>
> Fixes: 9c395c1b99bd ("bpf: Add basic bpf_rb_{root,node} support")
> Signed-off-by: Kaitao Cheng <chengkaitao@xxxxxxxxxx>

Please use [PATCH bpf] tag so CI can test it. Do we need a selftest?

LGTM with a few nits below.

Acked-by: Yonghong Song <yonghong.song@xxxxxxxxx>

> ---
>   kernel/bpf/helpers.c | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 9ca195104667..46e8eada463b 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2307,22 +2307,30 @@ void bpf_rb_root_free(const struct btf_field *field, void *rb_root,
>   {
>   	struct rb_root_cached orig_root, *root = rb_root;
>   	struct rb_node *pos, *n;
> -	void *obj;
>   
>   	BUILD_BUG_ON(sizeof(struct rb_root_cached) > sizeof(struct bpf_rb_root));
>   	BUILD_BUG_ON(__alignof__(struct rb_root_cached) > __alignof__(struct bpf_rb_root));
>   
>   	__bpf_spin_lock_irqsave(spin_lock);
>   	orig_root = *root;
> +	bpf_rbtree_postorder_for_each_entry_safe(pos, n, &orig_root.rb_root) {
> +		struct bpf_rb_node_kern *node;

Move 'struct bpf_rb_node_kern *node;' and the below to the top function declaration.
This will make code simpler.

> +
> +		node = rb_entry(pos, struct bpf_rb_node_kern, rb_node);
> +		WRITE_ONCE(node->owner, BPF_PTR_POISON);
> +	}
>   	*root = RB_ROOT_CACHED;
>   	__bpf_spin_unlock_irqrestore(spin_lock);
>   
>   	bpf_rbtree_postorder_for_each_entry_safe(pos, n, &orig_root.rb_root) {
> -		obj = pos;
> -		obj -= field->graph_root.node_offset;

We can keep this two ...

> +		struct bpf_rb_node_kern *node;
>   
> -
> -		__bpf_obj_drop_impl(obj, field->graph_root.value_rec, false);
> +		node = rb_entry(pos, struct bpf_rb_node_kern, rb_node);
> +		RB_CLEAR_NODE(pos);
> +		/* Ensure __bpf_rbtree_add() sees the node as unlinked. */
> +		smp_store_release(&node->owner, NULL);
> +		__bpf_obj_drop_impl((char *)pos - field->graph_root.node_offset,
> +				    field->graph_root.value_rec, false);

and then __bpf_obj_drop_impl(...) will not change.

>   	}
>   }
>