Re: [PATCH bpf-next 1/2] bpf: Support bpf_rcu_head in hash and LRU hash maps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: preallocated hash map loses per-cpu spare causing update failure
- [Medium] bpf: LRU eviction fails to cancel timers immediately for RCU claimed elements
--

commit 48e7d59b1040d1c559a2c8cba927c93eb44f710e
Author: Puranjay Mohan <puranjay@kernel.org>

bpf: Support bpf_rcu_head in hash and LRU hash maps

This commit introduces bpf_rcu_head support for hash and LRU hash maps,
allowing map elements with RCU callbacks to be handled appropriately on
deletion. Elements are kept alive until the callback completes, with the
dead bit stopping further arming.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index f744a42bb8139..5e2f4cbe9d0f2 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -914,15 +925,24 @@ static bool htab_lru_map_delete_node(void *arg, struct bpf_lru_node *node)
>  	hlist_nulls_for_each_entry_rcu(l, n, head, hash_node)
>  		if (l == tgt_l) {
>  			hlist_nulls_del_rcu(&l->hash_node);
> -			bpf_map_dec_elem_count(&htab->map);
>  			break;
>  		}
>  
>  	htab_unlock_bucket(b, flags);
>  
> -	if (l == tgt_l)
> -		check_and_cancel_fields(htab, l);
> -	return l == tgt_l;
> +	if (l != tgt_l)
> +		return false;
> +
> +	/*
> +	 * An arm can land after the check above. The element is already unlinked
> +	 * by now, so map_release_elem() finishes the handoff to the free list.
> +	 */
> +	if (bpf_rcu_head_claim(&htab->map, htab_elem_value(l, htab->map.key_size)))
> +		return false;

[Severity: Medium]
When an element is claimed by an RCU callback, htab_lru_map_delete_node()
returns early and skips check_and_cancel_fields(). Does this leave timers
and other special fields active after logical deletion?

The commit message mentions that timers and friends in the same value are
still cancelled on delete, but they appear to remain active here and could
fire during the RCU grace period.

> +
> +	bpf_map_dec_elem_count(&htab->map);
> +	check_and_cancel_fields(htab, l);
> +	return true;
>  }

[ ... ]

> @@ -1129,9 +1179,18 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key,
>  			struct pcpu_freelist_node *l;
>  
>  			l = __pcpu_freelist_pop(&htab->freelist);
> -			if (!l)
> -				return ERR_PTR(-E2BIG);
> -			l_new = container_of(l, struct htab_elem, fnode);
> +			if (l) {
> +				l_new = container_of(l, struct htab_elem, fnode);
> +			} else {
> +				/* Spend the spare; freeing old_elem refills the freelist. */
> +				if (!old_elem)
> +					return ERR_PTR(-E2BIG);
> +				pl_new = this_cpu_ptr(htab->extra_elems);
> +				l_new = *pl_new;
> +				if (!l_new)
> +					return ERR_PTR(-E2BIG);
> +				*pl_new = NULL;

[Severity: High]
By setting the per-CPU spare *pl_new = NULL and later pushing the replaced
element into the global freelist, is the per-CPU spare permanently lost?

Once all CPUs lose their spares and the map is full, could updating an
existing element fail with -E2BIG?

> +			}
>  			bpf_map_inc_elem_count(&htab->map);
>  		}
>  	} else {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924162858.2435106-1-puranjay@kernel.org?part=1