Re: [PATCH] bpf: defer printk() inside __bpf_prog_run()

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 27 Jun 2024 07:15:25 +0900
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> On 2024/06/27 1:27, Steven Rostedt wrote:
> > On Wed, 26 Jun 2024 09:02:22 +0900
> > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> >   
> >> On 2024/06/26 8:56, Alexei Starovoitov wrote:  
> >>> You are missing the point. The bug has nothing to do with bpf.    
> >>
> >> The bug is caused by calling tracing hooks with rq lock held.
> >> If tracing hooks do not exist, this bug does not exist.  
> > 
> > Could you expand on this. What tracing hooks are called with rq lock
> > held? You mean the scheduling events?  
> 
> Yes, trace_sched_switch().
> __schedule() calls trace_sched_switch() hook with rq lock held.
> 
>  #2: ffff8880b943e798 (&rq->__lock){-.-.}-{2:2}, at: raw_spin_rq_lock_nested+0x2a/0x140 kernel/sched/core.c:559
> 
>  __bpf_prog_run include/linux/filter.h:691 [inline]
>  bpf_prog_run include/linux/filter.h:698 [inline]
>  __bpf_trace_run kernel/trace/bpf_trace.c:2403 [inline]
>  bpf_trace_run4+0x334/0x590 kernel/trace/bpf_trace.c:2446
>  __traceiter_sched_switch+0x98/0xd0 include/trace/events/sched.h:222
>  trace_sched_switch include/trace/events/sched.h:222 [inline]
>  __schedule+0x2587/0x4a20 kernel/sched/core.c:6742
>  preempt_schedule_notrace+0x100/0x140 kernel/sched/core.c:7017

So you are saying that because a BPF hook can attach to a tracepoint
that is called with rq locks held, it should always disable preemption
and call printk_deferred_enter(), because it *might* hit an error path
that will call printk?? In other words, how the BPF hook is used
determines if the rq lock is held or not when it is called.

I can use that same argument for should_fail_ex(). Because how it is
used determines if the rq lock is held or not when it is called. And it
is the function that actually calls printk().

Sorry, but it makes no sense to put the burden of the
printk_deferred_enter() on the BPF hook logic. It should sit solely
with the code that actually calls printk().

-- Steve