Re: [PATCH bpf-next v3 04/16] bpf: Convert bpf_selem_unlink to failable

[Martin KaFai Lau <martin.lau@linux.dev>]

On 12/18/25 9:56 AM, Amery Hung wrote:
> To prepare changing both bpf_local_storage_map_bucket::lock and
> bpf_local_storage::lock to rqspinlock, convert bpf_selem_unlink() to
> failable. It still always succeeds and returns 0 until the change
> happens. No functional change.
> 
> For bpf_local_storage_map_free(), WARN_ON() for now as no real error
> will happen until we switch to rqspinlock.
> 
> __must_check is added to the function declaration locally to make sure
> all callers are accounted for during the conversion.

I don't see __must_check. The same for patch 2.

> 
> Signed-off-by: Amery Hung <ameryhung@gmail.com>
> ---
>   include/linux/bpf_local_storage.h | 2 +-
>   kernel/bpf/bpf_cgrp_storage.c     | 3 +--
>   kernel/bpf/bpf_inode_storage.c    | 4 +---
>   kernel/bpf/bpf_local_storage.c    | 8 +++++---
>   kernel/bpf/bpf_task_storage.c     | 4 +---
>   net/core/bpf_sk_storage.c         | 4 +---
>   6 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/bpf_local_storage.h b/include/linux/bpf_local_storage.h
> index 6cabf5154cf6..a94e12ddd83d 100644
> --- a/include/linux/bpf_local_storage.h
> +++ b/include/linux/bpf_local_storage.h
> @@ -176,7 +176,7 @@ int bpf_local_storage_map_check_btf(const struct bpf_map *map,
>   void bpf_selem_link_storage_nolock(struct bpf_local_storage *local_storage,
>   				   struct bpf_local_storage_elem *selem);
>   
> -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);
>   
>   int bpf_selem_link_map(struct bpf_local_storage_map *smap,
>   		       struct bpf_local_storage_elem *selem);
> diff --git a/kernel/bpf/bpf_cgrp_storage.c b/kernel/bpf/bpf_cgrp_storage.c
> index 0687a760974a..8fef24fcac68 100644
> --- a/kernel/bpf/bpf_cgrp_storage.c
> +++ b/kernel/bpf/bpf_cgrp_storage.c
> @@ -118,8 +118,7 @@ static int cgroup_storage_delete(struct cgroup *cgroup, struct bpf_map *map)
>   	if (!sdata)
>   		return -ENOENT;
>   
> -	bpf_selem_unlink(SELEM(sdata), false);
> -	return 0;
> +	return bpf_selem_unlink(SELEM(sdata), false);
>   }
>   
>   static long bpf_cgrp_storage_delete_elem(struct bpf_map *map, void *key)
> diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> index e54cce2b9175..cedc99184dad 100644
> --- a/kernel/bpf/bpf_inode_storage.c
> +++ b/kernel/bpf/bpf_inode_storage.c
> @@ -110,9 +110,7 @@ static int inode_storage_delete(struct inode *inode, struct bpf_map *map)
>   	if (!sdata)
>   		return -ENOENT;
>   
> -	bpf_selem_unlink(SELEM(sdata), false);
> -
> -	return 0;
> +	return bpf_selem_unlink(SELEM(sdata), false);
>   }
>   
>   static long bpf_fd_inode_storage_delete_elem(struct bpf_map *map, void *key)
> 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;
> @@ -377,7 +377,7 @@ void bpf_selem_unlink(struct bpf_local_storage_elem *selem, bool reuse_now)
>   
>   	if (unlikely(!selem_linked_to_storage_lockless(selem)))
>   		/* selem has already been unlinked from sk */
> -		return;
> +		return 0;
>   
>   	local_storage = rcu_dereference_check(selem->local_storage,
>   					      bpf_rcu_lock_held());
> @@ -402,6 +402,8 @@ void bpf_selem_unlink(struct bpf_local_storage_elem *selem, bool reuse_now)
>   
>   	if (free_local_storage)
>   		bpf_local_storage_free(local_storage, reuse_now);
> +
> +	return err;

err is not used in patch 3 and then becomes useful in patch 4. The 
ai-review discovered issue on err also. Squash patch 4 into patch 3. It 
will be easier to read.

>   }
>   
>   void __bpf_local_storage_insert_cache(struct bpf_local_storage *local_storage,
> @@ -837,7 +839,7 @@ void bpf_local_storage_map_free(struct bpf_map *map,
>   				struct bpf_local_storage_elem, map_node))) {
>   			if (busy_counter)
>   				this_cpu_inc(*busy_counter);
> -			bpf_selem_unlink(selem, true);
> +			WARN_ON(bpf_selem_unlink(selem, true));

nit. I would add __must_check to the needed functions in a single patch 
when everything is ready instead of having an intermediate WARN_ON here 
and then removed it later.