Re: [PATCH bpf-next v10 1/8] bpf: refactor __bpf_list_del to take list node pointer

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

在 2026/4/28 02:43, bot+bpf-ci@kernel.org 写道:
>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>> --- a/kernel/bpf/helpers.c
>> +++ b/kernel/bpf/helpers.c
> 
> [ ... ]
> 
>> @@ -2575,12 +2577,16 @@ static struct bpf_list_node *__bpf_list_del(struct bpf_list_head *head,
>>
>>  __bpf_kfunc struct bpf_list_node *bpf_list_pop_front(struct bpf_list_head *head)
>>  {
>> -	return __bpf_list_del(head, false);
>> +	struct list_head *h = (void *)head;
>> +
>> +	return __bpf_list_del(head, h->next);
>>  }
> 
> This addresses an unaddressed review comment from Emil Tsalapatis in v8.
> The code reads h->next before __bpf_list_del checks if the list is
> initialized. If the list is uninitialized (h->next is NULL), a NULL pointer
> is passed to __bpf_list_del.
> 
> While the function will eventually initialize the list and return NULL without
> dereferencing the NULL pointer:
> 
> kernel/bpf/helpers.c:__bpf_list_del() {
> 	if (unlikely(!h->next)) {
> 		INIT_LIST_HEAD(h);
> 		return NULL;
> 	}
> 	...
> }
> 
> this is counterintuitive - passing a potentially garbage value that is later
> ignored.
> 
> Emil suggested moving the init check logic into the top-level kfuncs
> (bpf_list_pop_front/bpf_list_pop_back) to ensure the list_head passed to
> __bpf_list_del is always valid. You can also just init the list and return
> NULL in that case - we know it's empty.
> 
> The same issue applies to bpf_list_pop_back() which reads h->prev before
> the initialization check.
> 
> Original comment from Emil Tsalapatis (Thu, 19 Mar 2026 12:17:50 -0400):
> https://lore.kernel.org/bpf/DH6W7K2SLPYB.PCO7FQ6CR1YZ@etsalapatis.com/
> 
> "The new code reads n _before_ we check if the list is initialized. So the n
> we are passing from the caller may well be NULL. However, __bpf_list_del()
> will in that case now a) call INIT_LIST_HEAD(() to properly set up prev/next,
> b) call list_empty() on the newly initialized list and exit without ever
> reading the NULL passed by the caller.
> 
> This is kind of counterintuitive: We are passing essentially a garbage value
> to __bpf_list_del that we thankfully end upi ignoring. Can you move the init
> check logic into the top-level kfuncs to make sure the list_head we're passing
> to __bpf_list_del is always valid? You can also just init the list and return
> NULL in that case - we know it's empty."
> 
> This issue remains unaddressed in v10.
> 
> 
> ---
> 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/25009536772

The '__bpf_list_del' function is a static function, so its scope and
associated risk are both very limited. I think its purpose is code
reuse and avoiding duplication.

If we move 'unlikely(!h->next)' to the outer callers, I believe it
brings no benefit other than making the code more redundant. Also,
I think validating a function's parameters before execution is not
counterintuitive; on the contrary, for global functions it is almost
a hard requirement.

-- 
Thanks
Kaitao Cheng