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

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 4/21/26 9:42 PM, Alexei Starovoitov wrote:
> On Tue Apr 21, 2026 at 10:14 AM PDT, Mykyta Yatsenko wrote:
>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>
>> Add bpf_prog_run_array_sleepable() for running BPF program arrays
>> on faultable tracepoints. Unlike bpf_prog_run_array_uprobe(), it
>> includes per-program recursion checking for private stack safety
>> and hardcodes is_uprobe to false.
>>
>> Skip dummy_bpf_prog at the top of the loop. When
>> bpf_prog_array_delete_safe() replaces a detached program with
>> dummy_bpf_prog on allocation failure, the dummy is statically
>> allocated and has NULL active, stats, and aux fields. Identify
>> it by prog->len == 0, since every real program has at least one
>> instruction.
>>
>> Keep bpf_prog_run_array_uprobe() unchanged for uprobe callers.
>>
>> Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>> ---
>>   include/linux/bpf.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 3cb6b9e70080..b6e96c939846 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -3079,6 +3079,61 @@ 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;
>> +
>> +	might_fault();
>> +	RCU_LOCKDEP_WARN(!rcu_read_lock_trace_held(), "no rcu lock held");
> 
> The only caller of this function is in the next patch trace_call_bpf_faultable()
> that does
> +	might_fault();
> +	guard(rcu_tasks_trace)();
> 
> imo above two lines are redunant.
> We can defensive programming when another caller appears.
> 
>> +
>> +	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 (!prog->sleepable)
>> +			rcu_read_lock();
>> +
>> +		if (unlikely(!bpf_prog_get_recursion_context(prog))) {
>> +			bpf_prog_inc_misses_counter(prog);
>> +			bpf_prog_put_recursion_context(prog);
>> +			if (!prog->sleepable)
>> +				rcu_read_unlock();
> 
> Why grab rcu_read_lock() and undo it?
> imo it would be cleaner and faster to do
> bpf_prog_get_recursion_context() here ...
> 
>> +			item++;
>> +			continue;
>> +		}
>> +
>> +		run_ctx.bpf_cookie = item->bpf_cookie;
>> +		ret &= run_prog(prog, ctx);
> 
> ... and then here:
> if (!prog->sleepable) {
>    guard(rcu)();
>    ret &= run_prog(prog, ctx);
> } else {
>    ret &= run_prog(prog, ctx);
> }
> 

Good point, postponing rcu lock makes the code cleaner. I'll send v12, 
thanks.