* Re: [PATCH bpf-next] bpf: Don't use percpu_counter for the htab element count in NMI
2026-10-20 13:15 [PATCH bpf-next] bpf: Don't use percpu_counter for the htab element count in NMI Puranjay Mohan
@ 2026-10-20 13:28 ` sashiko-bot
2026-10-20 13:53 ` Mykyta Yatsenko
2026-10-20 14:53 ` Alexei Starovoitov
2 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-10-20 13:28 UTC (permalink / raw)
To: Puranjay Mohan, bpf
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next] bpf: Don't use percpu_counter for the htab element count in NMI
2026-10-20 13:15 [PATCH bpf-next] bpf: Don't use percpu_counter for the htab element count in NMI Puranjay Mohan
2026-10-20 13:28 ` sashiko-bot
@ 2026-10-20 13:53 ` Mykyta Yatsenko
2026-10-20 14:53 ` Alexei Starovoitov
2 siblings, 0 replies; 4+ messages in thread
From: Mykyta Yatsenko @ 2026-10-20 13:53 UTC (permalink / raw)
To: Puranjay Mohan, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song
On 10/20/26 2:15 PM, Puranjay Mohan wrote:
> BPF programs attached to perf events run in NMI and can update a
> non-preallocated hash map. For large maps the element count is a
> percpu_counter, whose slow path takes fbc->lock with
> raw_spin_lock_irqsave(). That does not exclude NMIs, so an NMI that
> interrupts a holder of the lock and then updates the same map spins on it
> forever:
>
> WARNING: inconsistent lock state
> inconsistent {INITIAL USE} -> {IN-NMI} usage.
> kstress/261 [HC2[2]:SC0[0]:HE0:SE1] takes:
> (key#5){....}-{2:2}, at: _raw_spin_lock_irqsave+0x18/0x30
> {INITIAL USE} state was registered at:
> percpu_counter_add_batch+0xd8/0x160
> alloc_htab_elem+0x408/0x480
> htab_map_update_elem+0x1e8/0x6a0
>
> is_map_full() is affected too, as __percpu_counter_compare() falls back
> to percpu_counter_sum(), which takes the same lock.
>
> Count the elements added or removed in NMI in htab->count, which is
> otherwise unused for such maps, and add it to the percpu_counter where
> that is read. In NMI is_map_full() only reads the percpu_counter, so the
> limit is approximate by up to PERCPU_COUNTER_BATCH * num_online_cpus()
> there.
>
> Fixes: 86fe28f7692d ("bpf: Optimize element count in non-preallocated hash map.")
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> ---
> kernel/bpf/hashtab.c | 26 ++++++++++++++++++++------
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> 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,
To protect bucket locks in rhashtable from re-entrance from NMI we use
bpf_disable_instrumentation()/bpf_enable_instrumentation(), will it work here?
It looks like a more common solution for hashtab.
> + 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) {
>
> base-commit: b71326388a8a24674846c1d8d3b6dd831d9843a9
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next] bpf: Don't use percpu_counter for the htab element count in NMI
2026-10-20 13:15 [PATCH bpf-next] bpf: Don't use percpu_counter for the htab element count in NMI Puranjay Mohan
2026-10-20 13:28 ` sashiko-bot
2026-10-20 13:53 ` Mykyta Yatsenko
@ 2026-10-20 14:53 ` Alexei Starovoitov
2 siblings, 0 replies; 4+ messages in thread
From: Alexei Starovoitov @ 2026-10-20 14:53 UTC (permalink / raw)
To: Puranjay Mohan, bpf
Cc: Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau,
Eduard Zingerman, Kumar Kartikeya Dwivedi, Song Liu,
Yonghong Song
On Tue, Oct 20, 2026 at 06:15 AM Puranjay Mohan <puranjay@kernel.org> wrote:
> Count the elements added or removed in NMI in htab->count, which is
> otherwise unused for such maps, and add it to the percpu_counter where
> that is read. In NMI is_map_full() only reads the percpu_counter, so the
> limit is approximate by up to PERCPU_COUNTER_BATCH * num_online_cpus()
> there.
Not only NMI. queued_spin_lock_slowpath() calls trace_contention_end()
with the lock already held. When percpu_counter_add_batch() contends on
fbc->lock, a tp_btf/contention_end prog that updates the same map sees
the same this_cpu counter, goes to the slow path too and spins on
fbc->lock that its own cpu holds. in_nmi() is false there.
can we drop percpu_counter from htab instead ?
map.elem_count is already a lockless per-cpu count of the same elements.
htab->count can follow it in steps of the batch and is_map_full() can
sum elem_count when it gets close to max_entries. That is exact in NMI
as well. Then no need for pcount, in_nmi() checks and adding up two
counters.
Something like this (compile tested only):
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 7750df9..b298b89 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3041,6 +3041,8 @@ static inline void bpf_map_dec_elem_count(struct bpf_map *map)
this_cpu_dec(*map->elem_count);
}
+s64 bpf_map_sum_elem_count(const struct bpf_map *map);
+
extern int sysctl_unprivileged_bpf_disabled;
bool bpf_token_capable(const struct bpf_token *token, int cap);
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index bf78e09..6e21154 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -94,10 +94,10 @@ struct bpf_htab {
struct bpf_lru lru;
};
struct htab_elem *__percpu *extra_elems;
- /* number of elements in non-preallocated hashtable are kept
- * in either pcount or count
+ /* number of elements in non-preallocated hashtable. With
+ * use_percpu_counter it only follows map.elem_count in steps of
+ * PERCPU_COUNTER_BATCH, see is_map_full()
*/
- struct percpu_counter pcount;
atomic_t count;
bool use_percpu_counter;
bool has_hash;
@@ -739,29 +739,20 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr)
htab_init_buckets(htab);
-/* compute_batch_value() computes batch value as num_online_cpus() * 2
- * and __percpu_counter_compare() needs
- * htab->max_entries - cur_number_of_elems to be more than batch * num_online_cpus()
- * for percpu_counter to be faster than atomic_t. In practice the average bpf
- * hash map size is 10k, which means that a system with 64 cpus will fill
- * hashmap to 20% of 10k before percpu_counter becomes ineffective. Therefore
- * define our own batch count as 32 then 10k hash map can be filled up to 80%:
+/* is_map_full() needs htab->max_entries - cur_number_of_elems to be more than
+ * batch * nr_cpu_ids for the percpu counter to be faster than atomic_t. In
+ * practice the average bpf hash map size is 10k. With batch count of 32 a 10k
+ * hash map on a system with 64 cpus can be filled up to 80%:
* 10k - 8k > 32 _batch_ * 64 _cpus_
- * and __percpu_counter_compare() will still be fast. At that point hash map
+ * and is_map_full() will still be fast. At that point hash map
* collisions will dominate its performance anyway. Assume that hash map filled
* to 50+% isn't going to be O(1) and use the following formula to choose
- * between percpu_counter and atomic_t.
+ * between the percpu counter and atomic_t. The batch has to be a power of 2.
*/
#define PERCPU_COUNTER_BATCH 32
- if (attr->max_entries / 2 > num_online_cpus() * PERCPU_COUNTER_BATCH)
+ if (attr->max_entries / 2 > nr_cpu_ids * PERCPU_COUNTER_BATCH)
htab->use_percpu_counter = true;
- if (htab->use_percpu_counter) {
- err = percpu_counter_init(&htab->pcount, 0, GFP_KERNEL);
- if (err)
- goto free_map_locked;
- }
-
if (prealloc) {
err = prealloc_init(htab);
if (err)
@@ -789,8 +780,6 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr)
free_prealloc:
prealloc_destroy(htab);
free_map_locked:
- if (htab->use_percpu_counter)
- percpu_counter_destroy(&htab->pcount);
bpf_map_area_free(htab->buckets);
bpf_mem_alloc_destroy(&htab->pcpu_ma);
bpf_mem_alloc_destroy(&htab->ma);
@@ -1162,30 +1151,33 @@ static void htab_put_fd_value(struct bpf_htab *htab, struct htab_elem *l)
static bool is_map_full(struct bpf_htab *htab)
{
- if (htab->use_percpu_counter)
- return __percpu_counter_compare(&htab->pcount, htab->map.max_entries,
- PERCPU_COUNTER_BATCH) >= 0;
- return atomic_read(&htab->count) >= htab->map.max_entries;
+ s64 count = atomic_read(&htab->count);
+
+ /* each cpu's elem_count is ahead of count by less than the batch */
+ if (htab->use_percpu_counter &&
+ count + nr_cpu_ids * PERCPU_COUNTER_BATCH >= htab->map.max_entries)
+ count = bpf_map_sum_elem_count(&htab->map);
+ return count >= htab->map.max_entries;
}
static void inc_elem_count(struct bpf_htab *htab)
{
- bpf_map_inc_elem_count(&htab->map);
+ s64 cnt = this_cpu_inc_return(*htab->map.elem_count);
- if (htab->use_percpu_counter)
- percpu_counter_add_batch(&htab->pcount, 1, PERCPU_COUNTER_BATCH);
- else
+ if (!htab->use_percpu_counter)
atomic_inc(&htab->count);
+ else if (!(cnt & (PERCPU_COUNTER_BATCH - 1)))
+ atomic_add(PERCPU_COUNTER_BATCH, &htab->count);
}
static void dec_elem_count(struct bpf_htab *htab)
{
- bpf_map_dec_elem_count(&htab->map);
+ s64 cnt = this_cpu_dec_return(*htab->map.elem_count);
- if (htab->use_percpu_counter)
- percpu_counter_add_batch(&htab->pcount, -1, PERCPU_COUNTER_BATCH);
- else
+ if (!htab->use_percpu_counter)
atomic_dec(&htab->count);
+ else if (!((cnt + 1) & (PERCPU_COUNTER_BATCH - 1)))
+ atomic_sub(PERCPU_COUNTER_BATCH, &htab->count);
}
static void free_htab_elem(struct bpf_htab *htab, struct htab_elem *l)
@@ -1866,8 +1858,6 @@ static void htab_map_free(struct bpf_map *map)
bpf_map_area_free(htab->buckets);
bpf_mem_alloc_destroy(&htab->pcpu_ma);
bpf_mem_alloc_destroy(&htab->ma);
- if (htab->use_percpu_counter)
- percpu_counter_destroy(&htab->pcount);
bpf_map_area_free(htab);
}
@@ -2577,9 +2567,7 @@ static u64 htab_map_mem_usage(const struct bpf_map *map)
} else {
#define LLIST_NODE_SZ sizeof(struct llist_node)
- num_entries = htab->use_percpu_counter ?
- percpu_counter_sum(&htab->pcount) :
- atomic_read(&htab->count);
+ num_entries = bpf_map_sum_elem_count(map);
usage += (htab->elem_size + LLIST_NODE_SZ) * num_entries;
if (percpu) {
usage += (LLIST_NODE_SZ + sizeof(void *)) * num_entries;
pw-bot: cr
^ permalink raw reply related [flat|nested] 4+ messages in thread