* [PATCH bpf-next] bpf: use preempt_disable/enable() to protect bpf_bprintf_buffers nesting
@ 2025-11-09 17:36 Sahil Chandna
2025-11-09 18:07 ` bot+bpf-ci
0 siblings, 1 reply; 6+ messages in thread
From: Sahil Chandna @ 2025-11-09 17:36 UTC (permalink / raw)
To: yonghong.song, ast, daniel, andrii, martin.lau, eddyz87, song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, bigeasy, bpf
Cc: Sahil Chandna, syzbot+b0cff308140f79a9c4cb
The bpf_bprintf_prepare() and related helpers (bpf_try_get_buffers() /
bpf_put_buffers()) rely on a per-CPU counter bpf_bprintf_nest_level to
manage nested buffer usage. However, when invoked from different contexts
(process, softirq, NMI), the nesting counter can become inconsistent if
task migration occurs between CPUs during these operations. This can
result in warnings such as:
WARNING: CPU: 1 PID: 6145 at kernel/bpf/helpers.c:781 bpf_try_get_buffers kernel/bpf/helpers.c:781 [inline]
WARNING: CPU: 1 PID: 6145 at kernel/bpf/helpers.c:781 bpf_bprintf_prepare+0x12cf/0x13a0 kernel/bpf/helpers.c:834
Having only migrate_disable is insufficient here to prevent nesting,
hence add preempt_disable()/enable() around buffer acquisition and release.
Reported-by: syzbot+b0cff308140f79a9c4cb@syzkaller.appspotmail.comi
Closes: https://syzkaller.appspot.com/bug?extid=b0cff308140f79a9c4cb
Fixes: 7c33e97a6ef5 ("bpf: Do not disable preemption in bpf_test_run().")
Suggested-by: Yonghong Song <yonghong.song@linux.dev>
Signed-off-by: Sahil Chandna <chandna.sahil@gmail.com>
---
Testing:
Tested using syzkaller reproducers from:
[1] https://syzkaller.appspot.com/bug?extid=1f1fbecb9413cdbfbef8
[2] https://syzkaller.appspot.com/bug?extid=b0cff308140f79a9c4cb
Validation was done on PREEMPT_FULL and PREEMPT_RT configurations.
---
kernel/bpf/helpers.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index eb25e70e0bdc..01dbede0ecdc 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -777,9 +777,11 @@ int bpf_try_get_buffers(struct bpf_bprintf_buffers **bufs)
{
int nest_level;
+ preempt_disable();
nest_level = this_cpu_inc_return(bpf_bprintf_nest_level);
if (WARN_ON_ONCE(nest_level > MAX_BPRINTF_NEST_LEVEL)) {
this_cpu_dec(bpf_bprintf_nest_level);
+ preempt_enable();
return -EBUSY;
}
*bufs = this_cpu_ptr(&bpf_bprintf_bufs[nest_level - 1]);
@@ -789,9 +791,12 @@ int bpf_try_get_buffers(struct bpf_bprintf_buffers **bufs)
void bpf_put_buffers(void)
{
- if (WARN_ON_ONCE(this_cpu_read(bpf_bprintf_nest_level) == 0))
+ if (WARN_ON_ONCE(this_cpu_read(bpf_bprintf_nest_level) == 0)) {
+ preempt_enable();
return;
+ }
this_cpu_dec(bpf_bprintf_nest_level);
+ preempt_enable();
}
void bpf_bprintf_cleanup(struct bpf_bprintf_data *data)
--
2.50.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next] bpf: use preempt_disable/enable() to protect bpf_bprintf_buffers nesting
2025-11-09 17:36 [PATCH bpf-next] bpf: use preempt_disable/enable() to protect bpf_bprintf_buffers nesting Sahil Chandna
@ 2025-11-09 18:07 ` bot+bpf-ci
2025-11-09 19:44 ` Yonghong Song
0 siblings, 1 reply; 6+ messages in thread
From: bot+bpf-ci @ 2025-11-09 18:07 UTC (permalink / raw)
To: chandna.sahil, yonghong.song, ast, daniel, andrii, martin.lau,
eddyz87, song, john.fastabend, kpsingh, sdf, haoluo, jolsa,
bigeasy, bpf
Cc: chandna.sahil, syzbot+b0cff308140f79a9c4cb, ast, andrii, daniel,
martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 865b0dae3..99780b654 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -774,9 +774,11 @@ int bpf_try_get_buffers(struct bpf_bprintf_buffers **bufs)
> {
> int nest_level;
>
> + preempt_disable();
> nest_level = this_cpu_inc_return(bpf_bprintf_nest_level);
> if (WARN_ON_ONCE(nest_level > MAX_BPRINTF_NEST_LEVEL)) {
> this_cpu_dec(bpf_bprintf_nest_level);
> + preempt_enable();
> return -EBUSY;
> }
> *bufs = this_cpu_ptr(&bpf_bprintf_bufs[nest_level - 1]);
> @@ -786,9 +788,12 @@ int bpf_try_get_buffers(struct bpf_bprintf_buffers **bufs)
>
> void bpf_put_buffers(void)
> {
> - if (WARN_ON_ONCE(this_cpu_read(bpf_bprintf_nest_level) == 0))
> + if (WARN_ON_ONCE(this_cpu_read(bpf_bprintf_nest_level) == 0)) {
> + preempt_enable();
^^^^^^^^^^^^^^
Can this cause a preempt count underflow? The WARN_ON_ONCE checks for
nest_level==0, which means bpf_try_get_buffers() was never successfully
called. In that case, preempt_disable() was never called either, so calling
preempt_enable() here would underflow the preempt count.
In the normal calling pattern, bpf_try_get_buffers() either succeeds
(disabling preemption) or fails before reaching bpf_put_buffers(). But this
WARN exists to catch programming bugs where the get/put contract is violated.
If such a bug occurs, adding preempt_enable() here makes it worse by
introducing a preempt count imbalance on top of the original error.
> + return;
> + }
> this_cpu_dec(bpf_bprintf_nest_level);
> + preempt_enable();
> }
---
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/19212189476
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next] bpf: use preempt_disable/enable() to protect bpf_bprintf_buffers nesting
2025-11-09 18:07 ` bot+bpf-ci
@ 2025-11-09 19:44 ` Yonghong Song
2025-11-10 13:25 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2025-11-09 19:44 UTC (permalink / raw)
To: bot+bpf-ci, chandna.sahil, ast, daniel, andrii, martin.lau,
eddyz87, song, john.fastabend, kpsingh, sdf, haoluo, jolsa,
bigeasy, bpf
Cc: syzbot+b0cff308140f79a9c4cb, martin.lau, clm, ihor.solodrai
On 11/9/25 10:07 AM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>> index 865b0dae3..99780b654 100644
>> --- a/kernel/bpf/helpers.c
>> +++ b/kernel/bpf/helpers.c
>> @@ -774,9 +774,11 @@ int bpf_try_get_buffers(struct bpf_bprintf_buffers **bufs)
>> {
>> int nest_level;
>>
>> + preempt_disable();
>> nest_level = this_cpu_inc_return(bpf_bprintf_nest_level);
>> if (WARN_ON_ONCE(nest_level > MAX_BPRINTF_NEST_LEVEL)) {
>> this_cpu_dec(bpf_bprintf_nest_level);
>> + preempt_enable();
>> return -EBUSY;
>> }
>> *bufs = this_cpu_ptr(&bpf_bprintf_bufs[nest_level - 1]);
>> @@ -786,9 +788,12 @@ int bpf_try_get_buffers(struct bpf_bprintf_buffers **bufs)
>>
>> void bpf_put_buffers(void)
>> {
>> - if (WARN_ON_ONCE(this_cpu_read(bpf_bprintf_nest_level) == 0))
>> + if (WARN_ON_ONCE(this_cpu_read(bpf_bprintf_nest_level) == 0)) {
>> + preempt_enable();
> ^^^^^^^^^^^^^^
>
> Can this cause a preempt count underflow? The WARN_ON_ONCE checks for
> nest_level==0, which means bpf_try_get_buffers() was never successfully
> called. In that case, preempt_disable() was never called either, so calling
> preempt_enable() here would underflow the preempt count.
>
> In the normal calling pattern, bpf_try_get_buffers() either succeeds
> (disabling preemption) or fails before reaching bpf_put_buffers(). But this
> WARN exists to catch programming bugs where the get/put contract is violated.
> If such a bug occurs, adding preempt_enable() here makes it worse by
> introducing a preempt count imbalance on top of the original error.
Sorry, Sahil. I made a wrong suggestion earlier and the above AI analysis is correct.
We should not have preempt_enable() in the above. Please make the
change and submit v2.
>
>> + return;
>> + }
>> this_cpu_dec(bpf_bprintf_nest_level);
>> + preempt_enable();
>> }
>
> ---
> 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/19212189476
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next] bpf: use preempt_disable/enable() to protect bpf_bprintf_buffers nesting
2025-11-09 19:44 ` Yonghong Song
@ 2025-11-10 13:25 ` Sebastian Andrzej Siewior
2025-11-10 17:44 ` Yonghong Song
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2025-11-10 13:25 UTC (permalink / raw)
To: Yonghong Song
Cc: bot+bpf-ci, chandna.sahil, ast, daniel, andrii, martin.lau,
eddyz87, song, john.fastabend, kpsingh, sdf, haoluo, jolsa, bpf,
syzbot+b0cff308140f79a9c4cb, martin.lau, clm, ihor.solodrai
On 2025-11-09 11:44:48 [-0800], Yonghong Song wrote:
Could we do this instead?
There is __bpf_stream_push_str() => bpf_stream_page_reserve_elem() =>
bpf_stream_page_replace() => alloc_pages_nolock().
diff --git a/include/linux/sched.h b/include/linux/sched.h
index b469878de25c8..5a4965724c374 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1598,6 +1598,7 @@ struct task_struct {
void *security;
#endif
#ifdef CONFIG_BPF_SYSCALL
+ s8 bpf_bprintf_idx;
/* Used by BPF task local storage */
struct bpf_local_storage __rcu *bpf_storage;
/* Used for BPF run context */
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index eb25e70e0bdc0..62e37c845ec5a 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -770,28 +770,39 @@ static int bpf_trace_copy_string(char *buf, void *unsafe_ptr, char fmt_ptype,
/* Support executing three nested bprintf helper calls on a given CPU */
#define MAX_BPRINTF_NEST_LEVEL 3
-static DEFINE_PER_CPU(struct bpf_bprintf_buffers[MAX_BPRINTF_NEST_LEVEL], bpf_bprintf_bufs);
-static DEFINE_PER_CPU(int, bpf_bprintf_nest_level);
+struct bpf_cpu_buffer {
+ struct bpf_bprintf_buffers bufs[MAX_BPRINTF_NEST_LEVEL];
+ local_lock_t lock[MAX_BPRINTF_NEST_LEVEL];
+};
+
+static DEFINE_PER_CPU(struct bpf_cpu_buffer, bpf_cpu_bprintf) = {
+ .lock = { [0 ... MAX_BPRINTF_NEST_LEVEL - 1] = INIT_LOCAL_LOCK(bpf_cpu_bprintf.lock) },
+};
int bpf_try_get_buffers(struct bpf_bprintf_buffers **bufs)
{
- int nest_level;
+ s8 nest_level;
- nest_level = this_cpu_inc_return(bpf_bprintf_nest_level);
- if (WARN_ON_ONCE(nest_level > MAX_BPRINTF_NEST_LEVEL)) {
- this_cpu_dec(bpf_bprintf_nest_level);
+ nest_level = current->bpf_bprintf_idx++;
+ if (WARN_ON_ONCE(nest_level >= MAX_BPRINTF_NEST_LEVEL)) {
+ current->bpf_bprintf_idx--;
return -EBUSY;
}
- *bufs = this_cpu_ptr(&bpf_bprintf_bufs[nest_level - 1]);
+ local_lock(&bpf_cpu_bprintf.lock[nest_level]);
+ *bufs = this_cpu_ptr(&bpf_cpu_bprintf.bufs[nest_level]);
return 0;
}
void bpf_put_buffers(void)
{
- if (WARN_ON_ONCE(this_cpu_read(bpf_bprintf_nest_level) == 0))
+ s8 nest_level;
+
+ nest_level = current->bpf_bprintf_idx;
+ if (WARN_ON_ONCE(nest_level == 0))
return;
- this_cpu_dec(bpf_bprintf_nest_level);
+ local_unlock(&bpf_cpu_bprintf.lock[nest_level - 1]);
+ current->bpf_bprintf_idx--;
}
void bpf_bprintf_cleanup(struct bpf_bprintf_data *data)
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next] bpf: use preempt_disable/enable() to protect bpf_bprintf_buffers nesting
2025-11-10 13:25 ` Sebastian Andrzej Siewior
@ 2025-11-10 17:44 ` Yonghong Song
2025-11-11 10:37 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2025-11-10 17:44 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: bot+bpf-ci, chandna.sahil, ast, daniel, andrii, martin.lau,
eddyz87, song, john.fastabend, kpsingh, sdf, haoluo, jolsa, bpf,
syzbot+b0cff308140f79a9c4cb, martin.lau, clm, ihor.solodrai
On 11/10/25 5:25 AM, Sebastian Andrzej Siewior wrote:
> On 2025-11-09 11:44:48 [-0800], Yonghong Song wrote:
>
> Could we do this instead?
> There is __bpf_stream_push_str() => bpf_stream_page_reserve_elem() =>
> bpf_stream_page_replace() => alloc_pages_nolock().
I would suggest to stick to preempt_disable/enable().
In the bpf-next (newer change), for function
bpf_stream_elem_alloc(), kmalloc_nolock() is used
and no local_lock usage any more.
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index b469878de25c8..5a4965724c374 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1598,6 +1598,7 @@ struct task_struct {
> void *security;
> #endif
> #ifdef CONFIG_BPF_SYSCALL
> + s8 bpf_bprintf_idx;
> /* Used by BPF task local storage */
> struct bpf_local_storage __rcu *bpf_storage;
> /* Used for BPF run context */
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index eb25e70e0bdc0..62e37c845ec5a 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -770,28 +770,39 @@ static int bpf_trace_copy_string(char *buf, void *unsafe_ptr, char fmt_ptype,
> /* Support executing three nested bprintf helper calls on a given CPU */
> #define MAX_BPRINTF_NEST_LEVEL 3
>
> -static DEFINE_PER_CPU(struct bpf_bprintf_buffers[MAX_BPRINTF_NEST_LEVEL], bpf_bprintf_bufs);
> -static DEFINE_PER_CPU(int, bpf_bprintf_nest_level);
> +struct bpf_cpu_buffer {
> + struct bpf_bprintf_buffers bufs[MAX_BPRINTF_NEST_LEVEL];
> + local_lock_t lock[MAX_BPRINTF_NEST_LEVEL];
> +};
> +
> +static DEFINE_PER_CPU(struct bpf_cpu_buffer, bpf_cpu_bprintf) = {
> + .lock = { [0 ... MAX_BPRINTF_NEST_LEVEL - 1] = INIT_LOCAL_LOCK(bpf_cpu_bprintf.lock) },
> +};
>
> int bpf_try_get_buffers(struct bpf_bprintf_buffers **bufs)
> {
> - int nest_level;
> + s8 nest_level;
>
> - nest_level = this_cpu_inc_return(bpf_bprintf_nest_level);
> - if (WARN_ON_ONCE(nest_level > MAX_BPRINTF_NEST_LEVEL)) {
> - this_cpu_dec(bpf_bprintf_nest_level);
> + nest_level = current->bpf_bprintf_idx++;
> + if (WARN_ON_ONCE(nest_level >= MAX_BPRINTF_NEST_LEVEL)) {
> + current->bpf_bprintf_idx--;
> return -EBUSY;
> }
> - *bufs = this_cpu_ptr(&bpf_bprintf_bufs[nest_level - 1]);
>
> + local_lock(&bpf_cpu_bprintf.lock[nest_level]);
> + *bufs = this_cpu_ptr(&bpf_cpu_bprintf.bufs[nest_level]);
> return 0;
> }
>
> void bpf_put_buffers(void)
> {
> - if (WARN_ON_ONCE(this_cpu_read(bpf_bprintf_nest_level) == 0))
> + s8 nest_level;
> +
> + nest_level = current->bpf_bprintf_idx;
> + if (WARN_ON_ONCE(nest_level == 0))
> return;
> - this_cpu_dec(bpf_bprintf_nest_level);
> + local_unlock(&bpf_cpu_bprintf.lock[nest_level - 1]);
> + current->bpf_bprintf_idx--;
> }
>
> void bpf_bprintf_cleanup(struct bpf_bprintf_data *data)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next] bpf: use preempt_disable/enable() to protect bpf_bprintf_buffers nesting
2025-11-10 17:44 ` Yonghong Song
@ 2025-11-11 10:37 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2025-11-11 10:37 UTC (permalink / raw)
To: Yonghong Song
Cc: bot+bpf-ci, chandna.sahil, ast, daniel, andrii, martin.lau,
eddyz87, song, john.fastabend, kpsingh, sdf, haoluo, jolsa, bpf,
syzbot+b0cff308140f79a9c4cb, martin.lau, clm, ihor.solodrai
On 2025-11-10 09:44:55 [-0800], Yonghong Song wrote:
>
>
> On 11/10/25 5:25 AM, Sebastian Andrzej Siewior wrote:
> > On 2025-11-09 11:44:48 [-0800], Yonghong Song wrote:
> >
> > Could we do this instead?
> > There is __bpf_stream_push_str() => bpf_stream_page_reserve_elem() =>
> > bpf_stream_page_replace() => alloc_pages_nolock().
>
> I would suggest to stick to preempt_disable/enable().
> In the bpf-next (newer change), for function
> bpf_stream_elem_alloc(), kmalloc_nolock() is used
> and no local_lock usage any more.
Okay. This is then basically a revert of the removal of
preempt_disable(). If it doesn't break anything, fine then. I don't have
a strong argument against other than it would impose limitations as
mentioned previously (but this is no longer the case).
Unless it breaks anything, fine by me :)
The correct tags would be:
Reported-by: syzbot+b0cff308140f79a9c4cb@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/68f6a4c8.050a0220.1be48.0011.GAE@google.com/
Fixes: 4223bf833c849 ("bpf: Remove preempt_disable in bpf_try_get_buffers")
Sebastian
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-11-11 10:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-09 17:36 [PATCH bpf-next] bpf: use preempt_disable/enable() to protect bpf_bprintf_buffers nesting Sahil Chandna
2025-11-09 18:07 ` bot+bpf-ci
2025-11-09 19:44 ` Yonghong Song
2025-11-10 13:25 ` Sebastian Andrzej Siewior
2025-11-10 17:44 ` Yonghong Song
2025-11-11 10:37 ` Sebastian Andrzej Siewior
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox