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

[Petr Mladek <pmladek@suse.com>]

On Wed 2024-06-26 08:52:44, Tetsuo Handa wrote:
> On 2024/06/26 4:32, Alexei Starovoitov wrote:
> >>>>> On 2024-06-25, Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> >>>>>> syzbot is reporting circular locking dependency inside __bpf_prog_run(),
> >>>>>> for fault injection calls printk() despite rq lock is already held.
> > 
> > If you want to add printk_deferred_enter() it
> > probably should be in should_fail_ex(). Not here.
> > We will not be wrapping all bpf progs this way.
> 
> should_fail_ex() is just an instance.
> Three months ago you said "bpf never calls printk()" at
> https://lkml.kernel.org/r/CAADnVQLmLMt2bF9aAB26dtBCvy2oUFt+AAKDRgTTrc7Xk_zxJQ@mail.gmail.com ,
> but bpf does indirectly call printk() due to debug functionality.
> 
> We will be able to stop wrapping with printk_deferred_enter() after the printk
> rework completes ( https://lkml.kernel.org/r/ZXBCB2Gv1O-1-T6f@alley ). But we
> can't predict how long we need to wait for all console drivers to get converted.
> 
> Until the printk rework completes, it is responsibility of the caller to guard
> whatever possible printk() with rq lock already held.

Honestly, even the current printk rework does not solve the deadlock
with rq lock completely. The console_lock/console_sem will still be needed for
serialization with early consoles. It might need to be used when
printing emergency messages while there is still a boot console.

I am sure that it might be solved but I am not aware of any plan at
the moment.

I have just got a crazy idea. printk() needs to take the rq lock in
console_unlock() only when there is a waiter for the lock. The problem
might be gone if we offloaded the wakeup into an irq_work.

It is just an idea. I haven't thought much of all consequences and
scenarios. It might violate some basic locking rule and might not work.
Anyway, it would require special variant for unlocking semaphore which would
be used in console_unlock().

> If you think that only
> individual function that may call printk() (e.g. should_fail_ex()) should be
> wrapped, just saying "We will not be wrapping all bpf progs this way" does not
> help, for we need to scatter migrate_{disable,enable}() overhead as well as
> printk_deferred_{enter,exit}() to individual function despite majority of callers
> do not call e.g. should_fail_ex() with rq lock already held. Only who needs to
> call e.g. should_fail_ex() with rq lock already held should pay the cost. In this
> case, the one who should pay the cost is tracing hooks that are called with rq
> lock already held. I don't think that it is reasonable to add overhead to all
> users because tracing hooks might not be enabled or bpf program might not call
> e.g. should_fail_ex().
> 
> If you have a method that we can predict whether e.g. should_fail_ex() is called,
> you can wrap only bpf progs that call e.g. should_fail_ex(). But it is your role
> to maintain list of functions that might trigger printk(). I think that you don't
> want such burden (as well as all users don't want burden/overhead of adding
> migrate_{disable,enable}() only for the sake of bpf subsystem).

Yeah, converting printk() into printk_deferred() or using
printk_deferred_enter() around particular code paths is a whac-a-mole
game.

Best Regards,
Petr