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: <7edb0e39-a62e-4aac-a292-3cf7ae26ccbd@I-love.SAKURA.ne.jp>
Date: Wed, 26 Jun 2024 08:52:44 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: 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>,
        Petr Mladek <pmladek@...e.com>, 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 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. 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).