Message-Id: <20221211115218.2e6e289bb85f8cf53c11aa97@kernel.org>
Date:   Sun, 11 Dec 2022 11:52:18 +0900
From:   Masami Hiramatsu (Google) <mhiramat@...nel.org>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Borislav Petkov <bp@...en8.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Kees Cook <keescook@...omium.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        KP Singh <kpsingh@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Florent Revest <revest@...omium.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Christoph Hellwig <hch@...radead.org>,
        Chris Mason <clm@...a.com>
Subject: Re: [PATCH v2] panic: Taint kernel if fault injection has been used

Hi Alexei,

On Wed, 7 Dec 2022 20:36:28 -0800
Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:

> Yet for 2 days this 'taint' arguing is preventing people from looking at the bug.
> And that happens all the time on lkml. Somebody reports a bug and kernel devs
> jump on the poor person:
> "Can you repro without taint?",
> "Can you repro with upstream kernel?"
> This is discouraging.
> The 'taint' concept makes it easier for kernel devs to ignore bug reports
> and push back on the reporter.
> Do it few times and people stop reporting bugs.

That seems off topic for me. You seems complained against the taint flag
itself.

> Say, this particular bug in rethook was found by one of our BPF CI developers.
> They're not very familiar with the kernel, but they can see plenty of 'rethook'
> references in the stack trace, lookup MAINTAINER file and ping Massami,
> but to the question "can you repro without taint?" they can only say NO,
> because this is how our CI works. So they will keep silence and the bug will be lost.

BTW, this sounds like the BPF CI system design issue. If user is NOT easily
identifying what test caused the issue (e.g. what tests ran on the system
until the bug was found), the CI system is totally useless, because after
finding a problem, it must be investigated to solve the problem.

Without investigation, how would you usually fix the bug??

> That's not the only reason why I'm against generalizing 'taint'.
> Tainting because HW is misbehaving makes sense, but tainting because
> of OoO module or because of live-patching does not.
> It becomes an excuse that people abuse.

yeah, it is possible to be abused. but that is the problem who
abuse it.

> Right now syzbot is finding all sorts of bugs. Most of the time syzbot
> turns error injection on to find those allocation issues.
> If syzbot reports will start coming as tainted there will be even less
> attention to them. That will not be good.

Hmm, what kind of error injection does syzbot do? I would like to know
how it is used. For example, does that use only a specify set of
injection points, or use all existing points?

If the latter, I feel safer because syzbot ensures the current all
ALLOW_ERROR_INJECTION() functions will work with error injection. If not,
we need to consider removing the ALLOW_ERROR_INJECTION() from the
function which is not tested well (or add this taint flag.)

Documentation/fault-injection/fault-injection.rst has no explanation
about ALLOW_ERROR_INJECTION(), but obviously the ALLOW_ERROR_INJECTION()
marked functions and its caller MUST be designed safely against the
error injection. e.g.

- It must return an error code. (so EI_ETYPE_NONE must be removed)
- Caller must check the return value always.
  (but I thought this was the reason why we need this test framework...)
- It should not run any 'effective' code before checking an error.
  For example, increment counter, call other functions etc.
  (this means it can return without any side-effect)

Anything else?

[...]
> All these years we've been working on improving bpf introspection and
> debuggability. Today crash dumps look like this:
>   bpf_trace_printk+0xd3/0x170 kernel/trace/bpf_trace.c:377
>   bpf_prog_cf2ac6d483d8499b_trace_bpf_trace_printk+0x2b/0x37
>   bpf_dispatcher_nop_func include/linux/bpf.h:1082 [inline]
>   __bpf_prog_run include/linux/filter.h:600 [inline]
>   bpf_prog_run include/linux/filter.h:607 [inline]
> 
> The 2nd from the top is a bpf prog. The rest are kernel functions.
> bpf_prog_cf2ac6d483d8499b_trace_bpf_trace_printk
>          ^^ is a prog tag   ^^ name of bpf prog
> 
> If you do 'bpftool prog show' you can see both tag and name. 
> 'bpftool prog dump jited'
> dumps x86 code mixed with source line text.
> Often enough +0x2b offset will have some C code right next to it.

This is good, but this only works when the vmcore is dumped and
on the stack. My concern about the function error injection is
that makes some side effects, which can cause a problem afterwards
(this means after unloading the bpf prog)

> 
> One can monitor all prog load/unload via perf or via audit.

Ah, audit is helpful :), because we can dig the log what was loaded
before crash.


Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists