Re: [PATCH bpf-next v12 2/6] bpf: Add bpf_prog_run_array_sleepable()

Mykyta Yatsenko
2026-04-22T16:36:25Z

On 4/22/26 5:06 PM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -3079,6 +3079,56 @@ void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr);
>> void bpf_dynptr_set_rdonly(struct bpf_dynptr_kern *ptr);
>> void bpf_prog_report_arena_violation(bool write, unsigned long addr, unsigned long fault_ip);
>>
>> +static __always_inline u32
>> +bpf_prog_run_array_sleepable(const struct bpf_prog_array *array,
>> + const void *ctx, bpf_prog_run_fn run_prog)
>> +{
>> + const struct bpf_prog_array_item *item;
>> + struct bpf_prog *prog;
>> + struct bpf_run_ctx *old_run_ctx;
>> + struct bpf_trace_run_ctx run_ctx;
>> + u32 ret = 1;
>> +
>> + if (unlikely(!array))
>> + return ret;
>> +
>> + migrate_disable();
>> +
>> + run_ctx.is_uprobe = false;
>> +
>> + old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
>> + item = &array->items[0];
>> + while ((prog = READ_ONCE(item->prog))) {
>> + /* Skip dummy_bpf_prog placeholder (len == 0) */
>> + if (unlikely(!prog->len)) {
>> + item++;
>> + continue;
>> + }
>> +
>> + if (unlikely(!bpf_prog_get_recursion_context(prog))) {
>> + bpf_prog_inc_misses_counter(prog);
>> + bpf_prog_put_recursion_context(prog);
>> + item++;
>> + continue;
>> + }
>> +
>> + run_ctx.bpf_cookie = item->bpf_cookie;
>> +
>> + if (prog->sleepable) {
>> + guard(rcu)();
>> + ret &= run_prog(prog, ctx);
>> + } else {
>> + ret &= run_prog(prog, ctx);
>> + }
>

Yep, this one should be inverted.
I'll send v13, if Alexei does not mind.

> The sleepable/non-sleepable branches appear inverted relative to
> bpf_prog_run_array_uprobe(). In the uprobe sibling at
> include/linux/bpf.h:2491-2526, classic RCU is acquired only for
> non-sleepable programs:
>
> while ((prog = READ_ONCE(item->prog))) {
> if (!prog->sleepable)
> rcu_read_lock();
> run_ctx.bpf_cookie = item->bpf_cookie;
> ret &= run_prog(prog, ctx);
> item++;
> if (!prog->sleepable)
> rcu_read_unlock();
> }
>
> Here, the condition takes classic RCU (guard(rcu)() expands to
> rcu_read_lock()/rcu_read_unlock() via DEFINE_LOCK_GUARD_0 in
> include/linux/rcupdate.h:1181) around sleepable programs while taking no
> RCU read-side lock for non-sleepable ones.
>
> Sleepable BPF programs are designed to fault and sleep. Executing them
> inside a classic RCU read-side critical section violates the RCU
> requirement that classic readers must not block. On CONFIG_PREEMPT_RCU=n
> kernels this can cause RCU stalls.
>
> Meanwhile, non-sleepable BPF programs often rely on classic-RCU-protected
> structures (like classic RCU-protected map entries). Invoking them with no
> classic RCU read-side critical section weakens protection. The caller added
> in the subsequent commit 864dee5d55d1 holds only guard(rcu_tasks_trace)(),
> so no outer classic RCU protects these accesses.
>
> Should the condition be inverted to match the uprobe pattern?
>
> This was raised by Alexei Starovoitov in v11 review:
> https://lore.kernel.org/bpf/DHZ4I3RWVVCU.16XRZX1HPFI8K@gmail.com/
>
> Alexei suggested in his April 21st comment:
>
> "Why grab rcu_read_lock() and undo it?
> imo it would be cleaner and faster to do
> bpf_prog_get_recursion_context() here ...
>
> ... and then here:
> if (!prog->sleepable) {
> guard(rcu)();
> ret &= run_prog(prog, ctx);
> } else {
> ret &= run_prog(prog, ctx);
> }"
>
> Was this feedback addressed in v12?
>
>> +
>> + bpf_prog_put_recursion_context(prog);
>> + item++;
>> + }
>> + bpf_reset_run_ctx(old_run_ctx);
>> + migrate_enable();
>> + return ret;
>> +}
>> +
>> #else /* !CONFIG_BPF_SYSCALL */
>> static inline struct bpf_prog *bpf_prog_get(u32 ufd)
>> {
>
>
> ---
> 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/24787752194