linux-kernel - Re: [PATCH] bpf: defer printk() inside __bpf_prog

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZnvVQ5cs9F0b7paI@pathway.suse.cz>
Date: Wed, 26 Jun 2024 10:45:55 +0200
From: Petr Mladek <pmladek@...e.com>
To: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>,
	John Ogness <john.ogness@...utronix.de>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Andrii Nakryiko <andrii@...nel.org>,
	Martin KaFai Lau <martin.lau@...ux.dev>,
	Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
	Yonghong Song <yonghong.song@...ux.dev>,
	John Fastabend <john.fastabend@...il.com>,
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>,
	Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	bpf <bpf@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] bpf: defer printk() inside __bpf_prog_run()

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@...ove.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