Re: [PATCH RESEND bpf-next v10 2/8] bpf: clear list node owner and unlink before drop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Race condition causing list corruption, infinite loop, and Use-After-Free in `bpf_list_head_free`.
- [Critical] Incomplete fix leaving `bpf_rb_root_free` vulnerable to Use-After-Free via dangling `owner` pointers.
- [High] Unbounded O(N) latency under IRQ-disabled spinlock in `bpf_list_head_free`.
--

commit 1bc93a76541e332d30d80430964ef29571e93654
Author: Kaitao Cheng <chengkaitao@kylinos.cn>

bpf: clear list node owner and unlink before drop

This commit updates bpf_list_head_free to clear the owner pointer of each
node and use a local drain list before dropping the objects, aiming to
prevent concurrent readers from seeing an invalid next node.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c

[ ... ]

> @@ -2261,20 +2262,23 @@ void bpf_list_head_free(const struct btf_field *field, void *list_head,
>  	__bpf_spin_lock_irqsave(spin_lock);
>  	if (!head->next || list_empty(head))
>  		goto unlock;
> -	head = head->next;
> +	list_for_each_safe(pos, n, head) {
> +		WRITE_ONCE(container_of(pos,
> +			struct bpf_list_node_kern, list_head)->owner, NULL);
> +		list_move_tail(pos, &drain);
> +	}
>  unlock:

Does this introduce an unbounded delay with interrupts disabled?

The original code detached the head in O(1) time. Now there is an O(N)
loop inside __bpf_spin_lock_irqsave(). Since BPF lists can grow quite large,
could this cause soft lockups? 

This also appears to contradict the comment just above the lock which
explicitly mentions doing the actual list draining outside the lock to not
hold it for too long.

> -	INIT_LIST_HEAD(orig_head);
> +	INIT_LIST_HEAD(head);
>  	__bpf_spin_unlock_irqrestore(spin_lock);
>  
> -	while (head != orig_head) {
> -		void *obj = head;
> -
> -		obj -= field->graph_root.node_offset;
> -		head = head->next;
> +	while (!list_empty(&drain)) {
> +		pos = drain.next;
> +		list_del_init(pos);

Can this lead to list corruption and an infinite loop?

By setting the node's owner to NULL in the previous loop under the lock,
the node appears unlinked to the BPF subsystem. Once the lock is released
at the unlock label, a concurrent BPF program holding a reference to this
node might successfully pass it to bpf_list_push_front() or similar,
since the ownership check will now succeed.

If that happens, list_add() will overwrite the node's next and prev
pointers. However, the local drain list header isn't updated. The
drain.next pointer will still point to this hijacked node, and
list_empty(&drain) will remain false forever, causing an infinite loop
that repeatedly unlinks and drops the exact same node.

[ ... ]

On a related note, does bpf_rb_root_free() suffer from a similar issue
regarding dangling owner pointers?

While not introduced by this patch, if a surviving RB node's original
bpf_rb_root memory is freed and then reallocated as another bpf_rb_root,
the node's owner pointer might falsely match the reallocated root. 

A concurrent BPF program could then pass this node to bpf_rbtree_remove(),
bypassing the ownership check and accessing freed memory during
rb_erase_cached().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512055919.95716-1-kaitao.cheng@linux.dev?part=2