On Tue, 10 Mar 2026 at 21:10, Kumar Kartikeya Dwivedi <[email protected]> wrote: > > On Mon, 9 Mar 2026 at 07:34, Leon Hwang <[email protected]> wrote: > > > > On 8/3/26 21:46, Chengkaitao wrote: > > > From: Kaitao Cheng <[email protected]> > > > > > > If a user holds ownership of a node in the middle of a list, they > > > can directly remove it from the list without strictly adhering to > > > deletion rules from the head or tail. > > > > > > We have added an additional parameter bpf_list_head *head to > > > bpf_list_del, as the verifier requires the head parameter to > > > check whether the lock is being held. > > > > > > This is typically paired with bpf_refcount. After calling > > > bpf_list_del, it is generally necessary to drop the reference to > > > the list node twice to prevent reference count leaks. > > > > > > Signed-off-by: Kaitao Cheng <[email protected]> > > > --- > > > kernel/bpf/helpers.c | 30 +++++++++++++++++++++++------- > > > kernel/bpf/verifier.c | 6 +++++- > > > 2 files changed, 28 insertions(+), 8 deletions(-) > > > > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > > > index 6eb6c82ed2ee..01b74c4ac00d 100644 > > > --- a/kernel/bpf/helpers.c > > > +++ b/kernel/bpf/helpers.c > > > @@ -2426,20 +2426,23 @@ __bpf_kfunc int bpf_list_push_back_impl(struct > > > bpf_list_head *head, > > > return __bpf_list_add(n, head, true, meta ? meta->record : NULL, > > > off); > > > } > > > > > > -static struct bpf_list_node *__bpf_list_del(struct bpf_list_head *head, > > > bool tail) > > > +static struct bpf_list_node *__bpf_list_del(struct bpf_list_head *head, > > > + struct list_head *n) > > > { > > > - struct list_head *n, *h = (void *)head; > > > + struct list_head *h = (void *)head; > > > struct bpf_list_node_kern *node; > > > > > > /* If list_head was 0-initialized by map, bpf_obj_init_field wasn't > > > * called on its fields, so init here > > > */ > > > - if (unlikely(!h->next)) > > > + if (unlikely(!h->next)) { > > > INIT_LIST_HEAD(h); > > > - if (list_empty(h)) > > > + return NULL; > > > + } > > > + > > > + if (n == h) > > > return NULL; > > Couldn't you keep the list_empty(h) check after INIT_LIST_HEAD(h) as before? > And we don't need n == h either. You could add a comment that n will > never match h. > The verifier should ensure it, since it distinguishes the head and node types. > > Let's say the head is uninit. Then list_empty(h) will catch that case. > Otherwise n might be NULL. > > After list_empty(h) says false, we definitely have non-null n. > We just need to check list membership using the owner check, and then > we're good. > It will be a less noisy diff. > > > > > > > - n = tail ? h->prev : h->next; > > > node = container_of(n, struct bpf_list_node_kern, list_head); > > > if (WARN_ON_ONCE(READ_ONCE(node->owner) != head)) > > > return NULL; > > > > This refactoring is worth, because the "struct list_head *n" seems > > better than "bool tail". > > > > But, such refactoring should be a preparatory patch. Importantly, > > refactoring should not introduce functional changes. > > > > I think it's fine. Let's address this and avoid too many respins now. > It isn't a lot of code anyway. You could mention in the commit log > that you chose to refactor __bpf_list_del though. >
Just to make it clearer, since I feel the language above might be a bit confusing: Let's not add more churn and just fix the issues in the existing set, and try to move towards landing this. We are quite close, and it's been 7 iterations already. The bit about the non-owning refs pointed out by the AI bot, you can do it as a follow up on top after this is accepted. > [...]
