Re: [PATCH bpf-next v2] bpf: use preempt_disable/enable() to protect bpf_bprintf_buffers nesting

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2025-11-11 22:36:28 [+0530], Sahil Chandna wrote:
> 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:

Not to be a nitpick but different contexts as describe above is fine. If
a preemptible task is preempted by softirq followed by NMI nothing bad
will happen, it will work as intended. That preempt_disable() has no
affect on not getting preempted by a softirq, hardirq or NMI.

The problem is that a task can be preempted by another task. The BPF
program is limited to a single CPU (so counter will always be
incremented on the same CPU) but you can still have multiple tasks
on the same CPU asking/ returning a buffer.
The first task is using slot 0, the second one slot 1. Still works (by
some definition of works since 1 can return the buffer before 2 does and
ask for a new one which will be slot 2 which is still in use).
However if you add two additional tasks to the mix then you will exceed
the available slots which leads to the warning below.

> 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.

I prepared to following while adding the local locks which we agreed not
do:

    bpf_try_get_buffers() returns one of multiple per-CPU buffers. The
    "nesting" level is counted based on invocation to decide which buffer to
    return. This relies on disabled preemption in order to not get preempted
    while a buffer is returned and to have the buffers returned in the
    same order as they were requested. Without disabled preemption,
    multiple tasks can be preempted while a buffer is acquired,
    eventually exceeding the limit (MAX_BPRINTF_NEST_LEVEL) and
    triggering the warning.

    Multiple buffer can be acquired by the same context due users in
    different execution context (task and IRQ). It can also happen for a
    single context if after a buffer has been returned and another
    buffer needs to be returned due to an event (such as invoking
    spin_lock()).


you can take it or leave it. Here is a
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

since the change itself is okay and does not lead to any problems as far
as I am aware. I just think the description is not accurate and does not
describe the problem the syzkaller reproducer triggered. I leave to the
maintainer here…

Sebastian