netdev - Re: [PATCH bpf-next v3 04/16] bpf: Convert bpf_selem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <cd6f7bf79d96dff7196dfc0b33d72d91280bbb2de5042cb9d817918ebf4c7209@mail.kernel.org>
Date: Thu, 18 Dec 2025 18:27:55 +0000 (UTC)
From: bot+bpf-ci@...nel.org
To: ameryhung@...il.com,bpf@...r.kernel.org
Cc: netdev@...r.kernel.org,alexei.starovoitov@...il.com,andrii@...nel.org,daniel@...earbox.net,memxor@...il.com,martin.lau@...nel.org,kpsingh@...nel.org,yonghong.song@...ux.dev,song@...nel.org,haoluo@...gle.com,ameryhung@...il.com,kernel-team@...a.com,ast@...nel.org,andrii@...nel.org,daniel@...earbox.net,martin.lau@...nel.org,eddyz87@...il.com,yonghong.song@...ux.dev,clm@...a.com,ihor.solodrai@...ux.dev
Subject: Re: [PATCH bpf-next v3 04/16] bpf: Convert bpf_selem_unlink to failable

> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> index 0e3fa5fbaaf3..fa629a180e9e 100644
> --- a/kernel/bpf/bpf_local_storage.c
> +++ b/kernel/bpf/bpf_local_storage.c
> @@ -367,7 +367,7 @@ static void bpf_selem_link_map_nolock(struct bpf_local_storage_map *smap,
>  	hlist_add_head_rcu(&selem->map_node, &b->list);
>  }
>
> -void bpf_selem_unlink(struct bpf_local_storage_elem *selem, bool reuse_now)
> +int bpf_selem_unlink(struct bpf_local_storage_elem *selem, bool reuse_now)
>  {
>  	struct bpf_local_storage *local_storage;
>  	bool free_local_storage = false;
>  	HLIST_HEAD(selem_free_list);
>  	unsigned long flags;
>  	int err;

The err variable is declared here but not initialized.

>  	if (unlikely(!selem_linked_to_storage_lockless(selem)))
>  		/* selem has already been unlinked from sk */
> -		return;
> +		return 0;

[ ... ]

>  	raw_spin_lock_irqsave(&local_storage->lock, flags);
>  	if (likely(selem_linked_to_storage(selem))) {

If this condition is false, the if-block is skipped and err is never
assigned. There is a race window between the lockless check above and
this locked check where another CPU could unlink the selem.

>  		err = bpf_selem_unlink_map(selem);
>  		if (err)
>  			goto out;
>
>  		free_local_storage = bpf_selem_unlink_storage_nolock(
>  			local_storage, selem, &selem_free_list);
>  	}

[ ... ]

> +	return err;
>  }

Could this return an uninitialized value when selem_linked_to_storage()
returns false? The commit message says "always succeeds and returns 0"
but err is not initialized to 0.

I note that this appears to be addressed in a later commit in this
series (615477732ee8 "bpf: Change local_storage->lock and b->lock to
rqspinlock") where the lock function returns into err before the
condition check. Should err be initialized to 0 here for correctness
in the intermediate state?


---
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/20346662777