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

[Kaitao Cheng <kaitao.cheng@linux.dev>]

在 2026/5/21 12:30, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Potentially unbounded O(N) list traversal with interrupts disabled in `bpf_list_head_free()`.
> 
> Pre-existing issues:
> - [High] The patch fixes the stale `node->owner` issue for `bpf_list_head_free()` but fails to apply the equivalent fix for `bpf_rb_root_free()`, leaving `bpf_rbtree_remove()` vulnerable to the same memory corruption.
> --
> 
> commit 59439e3ee3f8d57af8635b521393d245d26bbdf4
> Author: Kaitao Cheng <chengkaitao@kylinos.cn>
> 
> bpf: clear list node owner and unlink before drop
> 
> This commit fixes an issue where refcounted BPF list nodes can outlive their
> list head and retain a stale owner pointer. To prevent memory corruption if a
> new list is allocated at the same address, it moves nodes to a private drain
> list under the spinlock, poisons their owner pointers, and unlinks them
> before dropping the objects.
> 
>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>> index 094457c3e6d37..59855b434f0b2 100644
>> --- a/kernel/bpf/helpers.c
>> +++ b/kernel/bpf/helpers.c
> 
> [ ... ]
> 
>> @@ -2261,20 +2262,30 @@ 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) {
>> +		struct bpf_list_node_kern *node;
>> +
>> +		node = container_of(pos, struct bpf_list_node_kern, list_head);
>> +		WRITE_ONCE(node->owner, BPF_PTR_POISON);
>> +		list_move_tail(pos, &drain);
>> +	}
> 
> [Severity: High]
> Could this new loop potentially cause a long CPU stall?
> 
> Since __bpf_spin_lock_irqsave() disables local interrupts, this O(N) list
> traversal executes with interrupts disabled.
> 
> BPF lists do not have a hard size limit, so if a program accumulates a
> very large number of elements in a single list, could iterating over them
> here trigger the NMI watchdog or cause severe latency spikes?

This is merely address-taking and assignment, so it is almost negligible
in cost, and the likelihood of causing a soft lockup is very low.

>>  unlock:
>> -	INIT_LIST_HEAD(orig_head);
>> +	INIT_LIST_HEAD(head);
>>  	__bpf_spin_unlock_irqrestore(spin_lock);
>>  
>> -	while (head != orig_head) {
>> -		void *obj = head;
>> +	while (!list_empty(&drain)) {
>> +		struct bpf_list_node_kern *node;
>>  
>> -		obj -= field->graph_root.node_offset;
>> -		head = head->next;
>> +		pos = drain.next;
>> +		node = container_of(pos, struct bpf_list_node_kern, list_head);
>> +		list_del_init(pos);
>> +		/* Ensure __bpf_list_add() sees the node as unlinked. */
>> +		smp_store_release(&node->owner, NULL);
>>  		/* The contained type can also have resources, including a
>>  		 * bpf_list_head which needs to be freed.
>>  		 */
>> -		__bpf_obj_drop_impl(obj, field->graph_root.value_rec, false);
>> +		__bpf_obj_drop_impl((char *)pos - field->graph_root.node_offset,
>> +				    field->graph_root.value_rec, false);
>>  	}
>>  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does bpf_rb_root_free() need
> the exact same fix?
> 
> When draining an rbtree, bpf_rb_root_free() calls __bpf_obj_drop_impl() on
> each node without clearing the node_internal->owner or unlinking the nodes.
> 
> If a surviving refcounted rb-node retains a pointer to the freed rb_root,
> and a new bpf_rb_root is allocated at the old address, wouldn't a call to
> bpf_rbtree_remove(new_root, stale_node) erroneously bypass the ownership
> check and invoke rb_erase_cached() with stale tree pointers?

I will probably send a separate fix patch later; this issue does not need
to be addressed in this patch series.

-- 
Thanks
Kaitao Cheng