Re: [PATCH bpf-next] bpf: Don't use percpu_counter for the htab element count in NMI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: 32-bit atomic_t htab->count overflow with 64-bit percpu_counter
--

commit 9bfb61b87bddde697a9661d8440f11f7f1dcd019
Author: Puranjay Mohan <puranjay@kernel.org>

bpf: Don't use percpu_counter for the htab element count in NMI

This patch avoids deadlocking on the percpu_counter spinlock in NMI context
by routing NMI element count updates for non-preallocated hash maps into
htab->count instead of htab->pcount, and combining the two counters in
is_map_full() and htab_map_mem_usage().

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index bf78e09f4d518..7a27479c46dc5 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -95,7 +95,8 @@ struct bpf_htab {
>  	};
>  	struct htab_elem *__percpu *extra_elems;
>  	/* number of elements in non-preallocated hashtable are kept
> -	 * in either pcount or count
> +	 * in either pcount or count; with pcount, those added or removed
> +	 * in NMI are in count, see is_map_full()
>  	 */
>  	struct percpu_counter pcount;
>  	atomic_t count;
> @@ -1160,11 +1161,23 @@ static void htab_put_fd_value(struct bpf_htab *htab, struct htab_elem *l)
>  	}
>  }
>  
> +/*
> + * With use_percpu_counter the elements added or removed in NMI are counted in
> + * @count, as percpu_counter takes a spinlock that the NMI could have
> + * interrupted the holder of.
> + */
>  static bool is_map_full(struct bpf_htab *htab)
>  {
> -	if (htab->use_percpu_counter)
> -		return __percpu_counter_compare(&htab->pcount, htab->map.max_entries,
> +	if (htab->use_percpu_counter) {
> +		/* percpu_counter_sum() takes that lock, so only read it in NMI */
> +		if (in_nmi())
> +			return percpu_counter_read(&htab->pcount) +
> +			       atomic_read(&htab->count) >= htab->map.max_entries;
> +		return __percpu_counter_compare(&htab->pcount,
> +						(s64)htab->map.max_entries -
> +						atomic_read(&htab->count),
>  						PERCPU_COUNTER_BATCH) >= 0;
> +	}
>  	return atomic_read(&htab->count) >= htab->map.max_entries;
>  }
>  
> @@ -1172,7 +1185,7 @@ static void inc_elem_count(struct bpf_htab *htab)
>  {
>  	bpf_map_inc_elem_count(&htab->map);
>  
> -	if (htab->use_percpu_counter)
> +	if (htab->use_percpu_counter && !in_nmi())
>  		percpu_counter_add_batch(&htab->pcount, 1, PERCPU_COUNTER_BATCH);
>  	else
>  		atomic_inc(&htab->count);
> @@ -1182,7 +1195,7 @@ static void dec_elem_count(struct bpf_htab *htab)
>  {
>  	bpf_map_dec_elem_count(&htab->map);
>  
> -	if (htab->use_percpu_counter)
> +	if (htab->use_percpu_counter && !in_nmi())
>  		percpu_counter_add_batch(&htab->pcount, -1, PERCPU_COUNTER_BATCH);
>  	else
>  		atomic_dec(&htab->count);
> @@ -2578,7 +2591,8 @@ static u64 htab_map_mem_usage(const struct bpf_map *map)
>  #define LLIST_NODE_SZ sizeof(struct llist_node)
>  
>  		num_entries = htab->use_percpu_counter ?
> -					  percpu_counter_sum(&htab->pcount) :
> +					  percpu_counter_sum(&htab->pcount) +
> +					  atomic_read(&htab->count) :
>  					  atomic_read(&htab->count);
>  		usage += (htab->elem_size + LLIST_NODE_SZ) * num_entries;
>  		if (percpu) {

[Severity: High]
Can htab->count overflow or underflow when elements are inserted in NMI
context and deleted outside NMI context (or vice versa)?

When htab->use_percpu_counter is enabled, inc_elem_count() and
dec_elem_count() choose between htab->pcount and htab->count based on
in_nmi() at the time of each operation, and htab->count is never folded
into htab->pcount.

If a perf_event BPF program running in NMI inserts elements via
htab_map_update_elem() -> alloc_htab_elem() -> inc_elem_count() (which
calls atomic_inc(&htab->count)) and userspace or a non-NMI BPF program
later deletes those elements via free_htab_elem() -> dec_elem_count()
(which calls percpu_counter_add_batch(&htab->pcount, -1, ...)), the two
counters diverge without bound over the lifetime of the map.

Because htab->pcount is a 64-bit signed counter (s64) while htab->count
is a 32-bit signed atomic_t, after 2^31 net cross-context insert/delete
cycles, htab->count wraps between INT_MAX and INT_MIN while htab->pcount
continues counting in 64-bit arithmetic.

When atomic_read(&htab->count) is sign-extended to s64 and combined with
htab->pcount in is_map_full() and htab_map_mem_usage(), wrapping
htab->count shifts the computed occupancy by +/- 2^32. That either
disables the max_entries limit in is_map_full() and underflows
num_entries in htab_map_mem_usage(), or causes is_map_full() to
permanently return true (-E2BIG) even when the map is empty.

Should htab->count be atomic64_t (or folded into htab->pcount in the
non-NMI paths via atomic_xchg(&htab->count, 0)) so the two counters
cannot wrap at different widths?

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