Re: [PATCH bpf-next v3 1/4] bpf: Add sleepable execution path for raw tracepoint programs

[Puranjay Mohan <puranjay@kernel.org>]

Mykyta Yatsenko <mykyta.yatsenko5@gmail.com> writes:

> From: Mykyta Yatsenko <yatsenko@meta.com>
>
> Modify __bpf_trace_run() to support both sleepable and non-sleepable
> BPF programs. When the program is sleepable:
>
> - Call might_fault() to annotate the faultable context
> - Use migrate_disable()/migrate_enable() instead of
>   rcu_read_lock()/rcu_read_unlock() to allow sleeping while
>   still protecting percpu data access
> - The outer rcu_tasks_trace lock is already held by the faultable
>   tracepoint callback (__DECLARE_TRACE_SYSCALL), providing lifetime
>   protection for the BPF program
>
> For non-sleepable programs, rcu_read_lock_dont_migrate() is replaced
> with explicit migrate_disable()/rcu_read_lock() pairing.
>
> Remove preempt_disable_notrace()/preempt_enable_notrace() from
> __BPF_DECLARE_TRACE_SYSCALL. Per-CPU protection and RCU locking are
> now managed per-program inside __bpf_trace_run().
>
> Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>

Acked-by: Puranjay Mohan <puranjay@kernel.org>

> ---
>  include/trace/bpf_probe.h |  2 --
>  kernel/trace/bpf_trace.c  | 13 ++++++++++---
>  2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
> index 9391d54d3f12..d1de8f9aa07f 100644
> --- a/include/trace/bpf_probe.h
> +++ b/include/trace/bpf_probe.h
> @@ -58,9 +58,7 @@ static notrace void							\
>  __bpf_trace_##call(void *__data, proto)					\
>  {									\
>  	might_fault();							\
> -	preempt_disable_notrace();					\
>  	CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data, CAST_TO_U64(args));	\
> -	preempt_enable_notrace();					\
>  }
>  
>  #undef DECLARE_EVENT_SYSCALL_CLASS
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 0b040a417442..3688a7e115d1 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2076,7 +2076,7 @@ void __bpf_trace_run(struct bpf_raw_tp_link *link, u64 *args)
>  	struct bpf_run_ctx *old_run_ctx;
>  	struct bpf_trace_run_ctx run_ctx;
>  
> -	rcu_read_lock_dont_migrate();
> +	migrate_disable();
>  	if (unlikely(!bpf_prog_get_recursion_context(prog))) {
>  		bpf_prog_inc_misses_counter(prog);
>  		goto out;
> @@ -2085,12 +2085,19 @@ void __bpf_trace_run(struct bpf_raw_tp_link *link, u64 *args)
>  	run_ctx.bpf_cookie = link->cookie;
>  	old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
>  
> -	(void) bpf_prog_run(prog, args);
> +	if (prog->sleepable) {
> +		might_fault();
> +		(void)bpf_prog_run(prog, args);
> +	} else {
> +		rcu_read_lock();
> +		(void)bpf_prog_run(prog, args);
> +		rcu_read_unlock();
> +	}
>  
>  	bpf_reset_run_ctx(old_run_ctx);
>  out:
>  	bpf_prog_put_recursion_context(prog);
> -	rcu_read_unlock_migrate();
> +	migrate_enable();
>  }
>  
>  #define UNPACK(...)			__VA_ARGS__
>
> -- 
> 2.52.0