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

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

在 2026/5/20 06:56, Eduard Zingerman 写道:
> On Mon, 2026-05-18 at 11:02 +0800, Kaitao Cheng wrote:
> 
> [...]
> 
>>>>> The patch does have a bug, however. To fix the issues we are seeing now,
>>>>> I propose the additional changes below and would appreciate feedback.
>>>>>
>>>>> --- a/kernel/bpf/helpers.c
>>>>> +++ b/kernel/bpf/helpers.c
>>>>> @@ -2263,8 +2263,10 @@ void bpf_list_head_free(const struct btf_field *field, void *list_head,
>>>>>         if (!head->next || list_empty(head))
>>>>>                 goto unlock;
>>>>>         list_for_each_safe(pos, n, head) {
>>>>> -               WRITE_ONCE(container_of(pos,
>>>>> -                       struct bpf_list_node_kern, list_head)->owner, NULL);
>>>>> +               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);
>>>>>         }
>>>>>  unlock:
>>>>> @@ -2272,8 +2274,12 @@ void bpf_list_head_free(const struct btf_field *field, void *list_head,
>>>>>         __bpf_spin_unlock_irqrestore(spin_lock);
>>>>>
>>>>>         while (!list_empty(&drain)) {
>>>>> +               struct bpf_list_node_kern *node;
>>>>> +
>>>>>                 pos = drain.next;
>>>>> +               node = container_of(pos, struct bpf_list_node_kern, list_head);
>>>>>                 list_del_init(pos);
>>>>> +               WRITE_ONCE(node->owner, NULL);
> 
> Is CPU allowed to reorder the stores in list_del_init() and WRITE_ONCE()?
> If it is, I think there is a race here.

Thanks for taking a close look at this. You are right that there is an
ordering issue here, but I don't think the specific sequence illustrated
by the example below is problematic.

> Thread #1:
>   enter bpf_list_head_free()
>   acquire H1 lock
>   list_move_tail(pos, &drain);             // reordered
>   <-- ip here -->
>   WRITE_ONCE(node->owner, BPF_PTR_POISON); // reordered
> 
> Thread #2:
> 
>   acquire H1 lock
>   n = bpf_refcount_acquire()
>   release H1 lock
>   acquire H2 lock
>   enter __bpf_list_add()
>   <-- ip here -->
>   cmpxchg(&node->owner, NULL, BPF_PTR_POISON)

Even if the stores from list_move_tail(pos, &drain) become visible before
WRITE_ONCE(node->owner, BPF_PTR_POISON), node->owner is not NULL in that
window. Before the WRITE_ONCE(), it still points to H1. After the WRITE_ONCE(),
it is BPF_PTR_POISON. In both cases, __bpf_list_add() will fail:

	cmpxchg(&node->owner, NULL, BPF_PTR_POISON)

because the old value is neither NULL nor expected to become NULL from this
part of bpf_list_head_free().


However, I agree that your original concern about the ordering between
list_del_init() and WRITE_ONCE(node->owner, NULL) is valid for the later
drain loop:

	list_del_init(pos);
	WRITE_ONCE(node->owner, NULL);

Here owner == NULL is the signal that the node can be inserted into another
list. Since WRITE_ONCE() does not provide release ordering, another CPU may
observe owner == NULL and successfully acquire the node in __bpf_list_add()
before the list_del_init() stores are visible. In that case __bpf_list_add()
can link the node into H2, and the delayed stores from list_del_init() may
then overwrite the node's list pointers and corrupt the H2 list.

So the fix should be to publish owner == NULL with release ordering after the
node has been fully unlinked, for example:

```
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2279,7 +2279,8 @@ void bpf_list_head_free(const struct btf_field *field, void *list_head,
                pos = drain.next;
                node = container_of(pos, struct bpf_list_node_kern, list_head);
                list_del_init(pos);
-               WRITE_ONCE(node->owner, NULL);
+               /* 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.
                 */
@@ -2607,7 +2608,8 @@ static struct bpf_list_node *__bpf_list_del(struct bpf_list_head *head,
                return NULL;

        list_del_init(n);
-       WRITE_ONCE(node->owner, NULL);
+       /* Ensure __bpf_list_add() sees the node as unlinked. */
+       smp_store_release(&node->owner, NULL);
        return (struct bpf_list_node *)n;
 }
```

The existing cmpxchg() in __bpf_list_add() is a successful RMW with return
value, so it is fully ordered and is sufficient on the acquire side.

-- 
Thanks
Kaitao Cheng