[v2] panic: Taint kernel if fault injection has been used

  From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Since the function error injection framework in the fault injection
subsystem can change the function code flow forcibly, it may cause
unexpected behavior (and that is the purpose of this feature) even
if it is applied to the ALLOW_ERROR_INJECTION functions.
So this feature must be used only for debugging or testing purpose.

To identify this in the kernel oops message, add a new taint flag
for the fault injection. This taint flag will be set by either
function error injection is used or the BPF use the kprobe_override
on error injectable functions (identified by ALLOW_ERROR_INJECTION).

Link: https://lore.kernel.org/all/20221121104403.1545f9b5@gandalf.local.home/T/#u

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Changes in v2:
  - Update kernel-chktaint.
---
 Documentation/admin-guide/tainted-kernels.rst |    5 +++++
 include/linux/panic.h                         |    3 ++-
 kernel/fail_function.c                        |    2 ++
 kernel/panic.c                                |    1 +
 kernel/trace/bpf_trace.c                      |    2 ++
 tools/debugging/kernel-chktaint               |    8 ++++++++
 6 files changed, 20 insertions(+), 1 deletion(-)
  

On Mon, Dec 05, 2022 at 07:22:44AM +0900, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Since the function error injection framework in the fault injection
> subsystem can change the function code flow forcibly, it may cause
> unexpected behavior (and that is the purpose of this feature) even
> if it is applied to the ALLOW_ERROR_INJECTION functions.
> So this feature must be used only for debugging or testing purpose.

The whole idea of tainting for kernel debugging is questionable.
There are many other *inject* kconfigs and other debug flags
for link lists, RCU, sleeping, etc.
None of them taint the kernel.

> To identify this in the kernel oops message, add a new taint flag

Have you ever seen a single oops message because of this particular
error injection?

> for the fault injection. This taint flag will be set by either
> function error injection is used or the BPF use the kprobe_override
> on error injectable functions (identified by ALLOW_ERROR_INJECTION).

...

>  	/* set the new array to event->tp_event and set event->prog */
> +	if (prog->kprobe_override)
> +		add_taint(TAINT_FAULT_INJECTED, LOCKDEP_NOW_UNRELIABLE);

Nack for bpf bits.
  

On Sun, 4 Dec 2022 14:30:01 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Mon, Dec 05, 2022 at 07:22:44AM +0900, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Since the function error injection framework in the fault injection
> > subsystem can change the function code flow forcibly, it may cause
> > unexpected behavior (and that is the purpose of this feature) even
> > if it is applied to the ALLOW_ERROR_INJECTION functions.
> > So this feature must be used only for debugging or testing purpose.
> 
> The whole idea of tainting for kernel debugging is questionable.
> There are many other *inject* kconfigs and other debug flags
> for link lists, RCU, sleeping, etc.
> None of them taint the kernel.
> 
> > To identify this in the kernel oops message, add a new taint flag
> 
> Have you ever seen a single oops message because of this particular
> error injection?

No, but there is no guarantee that the FEI doesn't cause any issue
in the future too. If it happens, we need to know the precise
information about what FEI/bpf does.
FEI is a kind of temporal Livepatch for testing. If Livepatch taints
the kernel, why doesn't the FEI taint it too?

> 
> > for the fault injection. This taint flag will be set by either
> > function error injection is used or the BPF use the kprobe_override
> > on error injectable functions (identified by ALLOW_ERROR_INJECTION).
> 
> ...
> 
> >  	/* set the new array to event->tp_event and set event->prog */
> > +	if (prog->kprobe_override)
> > +		add_taint(TAINT_FAULT_INJECTED, LOCKDEP_NOW_UNRELIABLE);
> 
> Nack for bpf bits.

I think this is needed especially for bpf bits. If we see this flag,
we can ask reporters to share the bpf programs which they used.

Thank you,
  

On Mon, Dec 05, 2022 at 07:59:21AM +0900, Masami Hiramatsu wrote:
> On Sun, 4 Dec 2022 14:30:01 -0800
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> > On Mon, Dec 05, 2022 at 07:22:44AM +0900, Masami Hiramatsu (Google) wrote:
> > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > 
> > > Since the function error injection framework in the fault injection
> > > subsystem can change the function code flow forcibly, it may cause
> > > unexpected behavior (and that is the purpose of this feature) even
> > > if it is applied to the ALLOW_ERROR_INJECTION functions.
> > > So this feature must be used only for debugging or testing purpose.
> > 
> > The whole idea of tainting for kernel debugging is questionable.
> > There are many other *inject* kconfigs and other debug flags
> > for link lists, RCU, sleeping, etc.
> > None of them taint the kernel.
> > 
> > > To identify this in the kernel oops message, add a new taint flag
> > 
> > Have you ever seen a single oops message because of this particular
> > error injection?
> 
> No, but there is no guarantee that the FEI doesn't cause any issue
> in the future too. If it happens, we need to know the precise
> information about what FEI/bpf does.
> FEI is a kind of temporal Livepatch for testing. If Livepatch taints
> the kernel, why doesn't the FEI taint it too?

Live patching can replace an arbitrary function and the kernel has
no visibility into what KLP module is doing.
While 'bpf error injection' is predictable.
The functions marked with [BPF_]ALLOW_ERROR_INJECTION can return errors
in the normal execution. So the callers of these functions have to deal with errors.

If kernel panics on such injected error it potentially would have paniced
on it anyway. At this point crash dump might be necessary to debug.
Whether oops happened because of bpf, kprobe or normal execution
doesn't matter much. The bug is in the caller that wasn't prepared
to deal with that error.

One can still walk all bpf progs from crash dump with tool "drgn"
(it has nice scripts to examine the dumps) or "crash" or other tools.

> > 
> > > for the fault injection. This taint flag will be set by either
> > > function error injection is used or the BPF use the kprobe_override
> > > on error injectable functions (identified by ALLOW_ERROR_INJECTION).
> > 
> > ...
> > 
> > >  	/* set the new array to event->tp_event and set event->prog */
> > > +	if (prog->kprobe_override)
> > > +		add_taint(TAINT_FAULT_INJECTED, LOCKDEP_NOW_UNRELIABLE);
> > 
> > Nack for bpf bits.
> 
> I think this is needed especially for bpf bits. If we see this flag,
> we can ask reporters to share the bpf programs which they used.

You can ask reporters to share bpf progs, but you can repro
the oops just as well without bpf. It's not bpf to blame, but the
bug in the caller that you should worry about.
  

On Mon, 5 Dec 2022 18:17:00 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Mon, Dec 05, 2022 at 07:59:21AM +0900, Masami Hiramatsu wrote:
> > On Sun, 4 Dec 2022 14:30:01 -0800
> > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > 
> > > On Mon, Dec 05, 2022 at 07:22:44AM +0900, Masami Hiramatsu (Google) wrote:
> > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > > 
> > > > Since the function error injection framework in the fault injection
> > > > subsystem can change the function code flow forcibly, it may cause
> > > > unexpected behavior (and that is the purpose of this feature) even
> > > > if it is applied to the ALLOW_ERROR_INJECTION functions.
> > > > So this feature must be used only for debugging or testing purpose.
> > > 
> > > The whole idea of tainting for kernel debugging is questionable.
> > > There are many other *inject* kconfigs and other debug flags
> > > for link lists, RCU, sleeping, etc.
> > > None of them taint the kernel.
> > > 
> > > > To identify this in the kernel oops message, add a new taint flag
> > > 
> > > Have you ever seen a single oops message because of this particular
> > > error injection?
> > 
> > No, but there is no guarantee that the FEI doesn't cause any issue
> > in the future too. If it happens, we need to know the precise
> > information about what FEI/bpf does.
> > FEI is a kind of temporal Livepatch for testing. If Livepatch taints
> > the kernel, why doesn't the FEI taint it too?
> 
> Live patching can replace an arbitrary function and the kernel has
> no visibility into what KLP module is doing.
> While 'bpf error injection' is predictable.

No, not much predictable because the kernel code can be changed.

> The functions marked with [BPF_]ALLOW_ERROR_INJECTION can return errors
> in the normal execution. So the callers of these functions have to deal with errors.

Right, but it might change something before checking the input, and
if it rejects the sane input, the caller may go into unexpected
status (e.g. the caller already checked input value, and does not
expect the call is fail). Such behaviors are buggy, yes. And the
FEI is designed for finding such buggy behavior.
(e.g. injecting error, but the caller passed successfully, it
means the caller code has some issue.)

> If kernel panics on such injected error it potentially would have paniced
> on it anyway.

Yes, but that doesn't cover all cases. If the function doesn't have
any internal state but returns an error according to the input,
FEI can make it return an error even if the input is correct.
And if it cause a kernel panic, that is a panic that must not
happen without FEI.

Thus, the ALLOW_ERROR_INJECTION should only be applied to the
function which has so-called 'side-effect', e.g. memory allocation,
external data (except for input data) read, etc. that could cause
an error regardless of the input value. Then the caller must
handle such errors.

> At this point crash dump might be necessary to debug.

Yes. So the TAINT flag can help. Please consider that the TAINT flag
doesn't mean you are guilty, but this is just a hint for debugging.
(good for the first triage)

> Whether oops happened because of bpf, kprobe or normal execution
> doesn't matter much. The bug is in the caller that wasn't prepared
> to deal with that error.
> 
> One can still walk all bpf progs from crash dump with tool "drgn"
> (it has nice scripts to examine the dumps) or "crash" or other tools.
> 
> > > 
> > > > for the fault injection. This taint flag will be set by either
> > > > function error injection is used or the BPF use the kprobe_override
> > > > on error injectable functions (identified by ALLOW_ERROR_INJECTION).
> > > 
> > > ...
> > > 
> > > >  	/* set the new array to event->tp_event and set event->prog */
> > > > +	if (prog->kprobe_override)
> > > > +		add_taint(TAINT_FAULT_INJECTED, LOCKDEP_NOW_UNRELIABLE);
> > > 
> > > Nack for bpf bits.
> > 
> > I think this is needed especially for bpf bits. If we see this flag,
> > we can ask reporters to share the bpf programs which they used.
> 
> You can ask reporters to share bpf progs, but you can repro
> the oops just as well without bpf. It's not bpf to blame, but the
> bug in the caller that you should worry about.

I don't blame the bpf, but just it points that undesigned behavior has
been injected. So we have to take it into account.

Thank you,
  

On Tue, Dec 06, 2022 at 04:20:35PM +0900, Masami Hiramatsu wrote:
> On Mon, 5 Dec 2022 18:17:00 -0800
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> > On Mon, Dec 05, 2022 at 07:59:21AM +0900, Masami Hiramatsu wrote:
> > > On Sun, 4 Dec 2022 14:30:01 -0800
> > > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > > 
> > > > On Mon, Dec 05, 2022 at 07:22:44AM +0900, Masami Hiramatsu (Google) wrote:
> > > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > > > 
> > > > > Since the function error injection framework in the fault injection
> > > > > subsystem can change the function code flow forcibly, it may cause
> > > > > unexpected behavior (and that is the purpose of this feature) even
> > > > > if it is applied to the ALLOW_ERROR_INJECTION functions.
> > > > > So this feature must be used only for debugging or testing purpose.
> > > > 
> > > > The whole idea of tainting for kernel debugging is questionable.
> > > > There are many other *inject* kconfigs and other debug flags
> > > > for link lists, RCU, sleeping, etc.
> > > > None of them taint the kernel.
> > > > 
> > > > > To identify this in the kernel oops message, add a new taint flag
> > > > 
> > > > Have you ever seen a single oops message because of this particular
> > > > error injection?
> > > 
> > > No, but there is no guarantee that the FEI doesn't cause any issue
> > > in the future too. If it happens, we need to know the precise
> > > information about what FEI/bpf does.
> > > FEI is a kind of temporal Livepatch for testing. If Livepatch taints
> > > the kernel, why doesn't the FEI taint it too?
> > 
> > Live patching can replace an arbitrary function and the kernel has
> > no visibility into what KLP module is doing.
> > While 'bpf error injection' is predictable.
> 
> No, not much predictable because the kernel code can be changed.
> 
> > The functions marked with [BPF_]ALLOW_ERROR_INJECTION can return errors
> > in the normal execution. So the callers of these functions have to deal with errors.
> 
> Right, but it might change something before checking the input, and
> if it rejects the sane input, the caller may go into unexpected
> status (e.g. the caller already checked input value, and does not
> expect the call is fail). Such behaviors are buggy, yes. And the
> FEI is designed for finding such buggy behavior.
> (e.g. injecting error, but the caller passed successfully, it
> means the caller code has some issue.)
> 
> > If kernel panics on such injected error it potentially would have paniced
> > on it anyway.
> 
> Yes, but that doesn't cover all cases. If the function doesn't have
> any internal state but returns an error according to the input,
> FEI can make it return an error even if the input is correct.
> And if it cause a kernel panic, that is a panic that must not
> happen without FEI.
> 
> Thus, the ALLOW_ERROR_INJECTION should only be applied to the
> function which has so-called 'side-effect', e.g. memory allocation,
> external data (except for input data) read, etc. that could cause
> an error regardless of the input value. Then the caller must
> handle such errors.

Not quite. I think you're confusing functions with 'side effect'
with 'pure' functions.
Your point about 'checking args before the call' applies to pure functions.
We have some too: git grep __pure.
Most of the time the compiler can identify the functions that return
the same value with the same args, but sometimes it needs help
and we mark such functions.
Clearly such functions should never be marked as 'error inject'
because changing return value of such function might lead to
wrong code and _it will not be a kernel bug_.
The compiler optimizations rely on the function being pure.
Live kernel patching should be very careful with __pure functions too.
No idea whether they do this now or not.

Of course, there is a category of functions (with or without side effects)
which return values should not be changed by error injecton mechanism.
That's a given. Applying ALLOW_ERROR_INJECTION should not to be taken lightly.

> 
> > At this point crash dump might be necessary to debug.
> 
> Yes. So the TAINT flag can help. Please consider that the TAINT flag
> doesn't mean you are guilty, but this is just a hint for debugging.
> (good for the first triage)

I think you misunderstand the reason behind 'tainted' flags.
It's 'hint for debugging' only on the surface.
See Documentation/admin-guide/tainted-kernels.rst
"... That's why bug reports
from tainted kernels will often be ignored by developers, hence try to reproduce
problems with an untainted kernel."

When 'error injection' finds a kernel bug the kernel developers need to
look into it regardless whether it's syzbot error injection
or whatever other mechanism.

To change the topic to something ... else...

We've just hit this panic using rethook.
[   49.235708] ==================================================================
[   49.236243] BUG: KASAN: use-after-free in rethook_try_get+0x7e/0x380
[   49.236693] Read of size 8 at addr ffff888102e62c88 by task test_progs/1688
[   49.240398]  kasan_report+0x90/0x190
[   49.240934]  rethook_try_get+0x7e/0x380
[   49.244885]  fprobe_handler.part.1+0x119/0x1f0
[   49.245505]  arch_ftrace_ops_list_func+0x17d/0x1d0
[   49.246544]  ftrace_regs_call+0x5/0x52
[   49.247411]  bpf_fentry_test1+0x5/0x10

[   49.262578] Allocated by task 1692:
[   49.262804]  kasan_save_stack+0x1c/0x40
[   49.263059]  kasan_set_track+0x21/0x30
[   49.263335]  __kasan_kmalloc+0x7a/0x90
[   49.263624]  rethook_alloc+0x2c/0xa0
[   49.263879]  fprobe_init_rethook+0x6d/0x170
[   49.264154]  register_fprobe_ips+0xae/0x130

[   49.265938] Freed by task 0:
[   49.266153]  kasan_save_stack+0x1c/0x40
[   49.266440]  kasan_set_track+0x21/0x30
[   49.266705]  kasan_save_free_info+0x26/0x40
[   49.266995]  __kasan_slab_free+0x103/0x190
[   49.267282]  __kmem_cache_free+0x1b7/0x3a0
[   49.267559]  rcu_core+0x4d8/0xd50

[   49.268181] Last potentially related work creation:
[   49.268565]  kasan_save_stack+0x1c/0x40
[   49.268898]  __kasan_record_aux_stack+0xa1/0xb0
[   49.269260]  call_rcu+0x47/0x360
[   49.269526]  unregister_fprobe+0x47/0x80

[   49.281382] general protection fault, probably for non-canonical address 0x57e006e00000000: 0000 [#1] PREEMPT SMP KASAN
[   49.282226] CPU: 6 PID: 1688 Comm: test_progs Tainted: G    B      O       6.1.0-rc7-01508-gf0c5a2d9f234 #4343
[   49.283751] RIP: 0010:rethook_trampoline_handler+0xff/0x1d0
[   49.289900] Call Trace:
[   49.290083]  <TASK>
[   49.290248]  arch_rethook_trampoline_callback+0x6c/0xa0
[   49.290631]  arch_rethook_trampoline+0x2c/0x50
[   49.290964]  ? lock_release+0xad/0x3f0
[   49.291245]  ? bpf_prog_test_run_tracing+0x235/0x380
[   49.291609]  trace_clock_x86_tsc+0x10/0x10

This is just running bpf selftests in parallel mode on 16-cpu VM on bpf-next.
Notice 'Tained' flags.
Please take a look.

Thanks!
  

On Tue, 6 Dec 2022 20:01:46 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:


> >   
> > > At this point crash dump might be necessary to debug.  
> > 
> > Yes. So the TAINT flag can help. Please consider that the TAINT flag
> > doesn't mean you are guilty, but this is just a hint for debugging.
> > (good for the first triage)  
> 
> I think you misunderstand the reason behind 'tainted' flags.
> It's 'hint for debugging' only on the surface.
> See Documentation/admin-guide/tainted-kernels.rst
> "... That's why bug reports
> from tainted kernels will often be ignored by developers, hence try to reproduce
> problems with an untainted kernel."

You conveniently left out the first part of that paragraph. Showing just a
portion of a statement can be very misleading. Let me add the whole
paragraph here:

   The kernel will mark itself as 'tainted' when something occurs that
   might be relevant later when investigating problems. Don't worry too much about this,
   most of the time it's not a problem to run a tainted kernel; the information is
   mainly of interest once someone wants to investigate some problem, as its real
   cause might be the event that got the kernel tainted. That's why bug reports
   from tainted kernels will often be ignored by developers, hence try to reproduce
   problems with an untainted kernel.

Let me stress the very first sentence above:

   The kernel will mark itself as 'tainted' when something occurs that
   might be relevant later when investigating problems.

I think you are the one that is misunderstanding what a taint is. It most
definitely is about giving hints for debugging. That's why the very first
sentence of that paragraph, as well as the entire document, explicitly
states "might be relevant later when investigating problems".


> 
> When 'error injection' finds a kernel bug the kernel developers need to
> look into it regardless whether it's syzbot error injection
> or whatever other mechanism.
> 

And this is a very useful taint. Just like:

  2  _/S       4  kernel running on an out of specification system
  5  _/B      32  bad page referenced or some unexpected page flags
  7  _/D     128  kernel died recently, i.e. there was an OOPS or BUG
 10  _/C    1024  staging driver was loaded
 11  _/I    2048  workaround for bug in platform firmware applied
 14  _/L   16384  soft lockup occurred 
 17  _/T  131072  kernel was built with the struct randomization plugin

Any of the above should not be ignored by developers, but they are useful
hints for debugging the issue.


> To change the topic to something ... else...
> 
> We've just hit this panic using rethook.
> [   49.235708] ==================================================================
> [   49.236243] BUG: KASAN: use-after-free in rethook_try_get+0x7e/0x380
> [   49.236693] Read of size 8 at addr ffff888102e62c88 by task test_progs/1688
> [   49.240398]  kasan_report+0x90/0x190
> [   49.240934]  rethook_try_get+0x7e/0x380
> [   49.244885]  fprobe_handler.part.1+0x119/0x1f0
> [   49.245505]  arch_ftrace_ops_list_func+0x17d/0x1d0
> [   49.246544]  ftrace_regs_call+0x5/0x52
> [   49.247411]  bpf_fentry_test1+0x5/0x10
> 
> [   49.262578] Allocated by task 1692:
> [   49.262804]  kasan_save_stack+0x1c/0x40
> [   49.263059]  kasan_set_track+0x21/0x30
> [   49.263335]  __kasan_kmalloc+0x7a/0x90
> [   49.263624]  rethook_alloc+0x2c/0xa0
> [   49.263879]  fprobe_init_rethook+0x6d/0x170
> [   49.264154]  register_fprobe_ips+0xae/0x130
> 
> [   49.265938] Freed by task 0:
> [   49.266153]  kasan_save_stack+0x1c/0x40
> [   49.266440]  kasan_set_track+0x21/0x30
> [   49.266705]  kasan_save_free_info+0x26/0x40
> [   49.266995]  __kasan_slab_free+0x103/0x190
> [   49.267282]  __kmem_cache_free+0x1b7/0x3a0
> [   49.267559]  rcu_core+0x4d8/0xd50
> 
> [   49.268181] Last potentially related work creation:
> [   49.268565]  kasan_save_stack+0x1c/0x40
> [   49.268898]  __kasan_record_aux_stack+0xa1/0xb0
> [   49.269260]  call_rcu+0x47/0x360
> [   49.269526]  unregister_fprobe+0x47/0x80
> 
> [   49.281382] general protection fault, probably for non-canonical address 0x57e006e00000000: 0000 [#1] PREEMPT SMP KASAN
> [   49.282226] CPU: 6 PID: 1688 Comm: test_progs Tainted: G    B      O       6.1.0-rc7-01508-gf0c5a2d9f234 #4343
> [   49.283751] RIP: 0010:rethook_trampoline_handler+0xff/0x1d0
> [   49.289900] Call Trace:
> [   49.290083]  <TASK>
> [   49.290248]  arch_rethook_trampoline_callback+0x6c/0xa0
> [   49.290631]  arch_rethook_trampoline+0x2c/0x50
> [   49.290964]  ? lock_release+0xad/0x3f0
> [   49.291245]  ? bpf_prog_test_run_tracing+0x235/0x380
> [   49.291609]  trace_clock_x86_tsc+0x10/0x10
> 
> This is just running bpf selftests in parallel mode on 16-cpu VM on bpf-next.
> Notice 'Tained' flags.
> Please take a look.
> 

"G - Proprietary module" - "O - out of tree module"

Can you reproduce this without those taints?

-- Steve
  

On Tue, 6 Dec 2022 23:39:47 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> "G - Proprietary module" - "O - out of tree module"

Sorry, "P" is proprietary, "G" is still GPL. But it is an out of tree
module still.  ;-)

-- Steve
  

On Tue, Dec 6, 2022 at 8:39 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > [   49.281382] general protection fault, probably for non-canonical address 0x57e006e00000000: 0000 [#1] PREEMPT SMP KASAN
> > [   49.282226] CPU: 6 PID: 1688 Comm: test_progs Tainted: G    B      O       6.1.0-rc7-01508-gf0c5a2d9f234 #4343
> > [   49.283751] RIP: 0010:rethook_trampoline_handler+0xff/0x1d0
> > [   49.289900] Call Trace:
> > [   49.290083]  <TASK>
> > [   49.290248]  arch_rethook_trampoline_callback+0x6c/0xa0
> > [   49.290631]  arch_rethook_trampoline+0x2c/0x50
> > [   49.290964]  ? lock_release+0xad/0x3f0
> > [   49.291245]  ? bpf_prog_test_run_tracing+0x235/0x380
> > [   49.291609]  trace_clock_x86_tsc+0x10/0x10
> >
> > This is just running bpf selftests in parallel mode on 16-cpu VM on bpf-next.
> > Notice 'Tained' flags.
> > Please take a look.
> >
>
> "G - Proprietary module" - "O - out of tree module"
>
> Can you reproduce this without those taints?

Lol. That question is exactly the reason why my Nack stands.
  

On December 6, 2022 11:45:17 PM EST, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>On Tue, Dec 6, 2022 at 8:39 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>> >
>> > [   49.281382] general protection fault, probably for non-canonical address 0x57e006e00000000: 0000 [#1] PREEMPT SMP KASAN
>> > [   49.282226] CPU: 6 PID: 1688 Comm: test_progs Tainted: G    B      O       6.1.0-rc7-01508-gf0c5a2d9f234 #4343
>> > [   49.283751] RIP: 0010:rethook_trampoline_handler+0xff/0x1d0
>> > [   49.289900] Call Trace:
>> > [   49.290083]  <TASK>
>> > [   49.290248]  arch_rethook_trampoline_callback+0x6c/0xa0
>> > [   49.290631]  arch_rethook_trampoline+0x2c/0x50
>> > [   49.290964]  ? lock_release+0xad/0x3f0
>> > [   49.291245]  ? bpf_prog_test_run_tracing+0x235/0x380
>> > [   49.291609]  trace_clock_x86_tsc+0x10/0x10
>> >
>> > This is just running bpf selftests in parallel mode on 16-cpu VM on bpf-next.
>> > Notice 'Tained' flags.
>> > Please take a look.
>> >
>>
>> "G - Proprietary module" - "O - out of tree module"
>>
>> Can you reproduce this without those taints?
>
>Lol. That question is exactly the reason why my Nack stands.

I only said the above *because* of your comment ;-) 

-- Steve
  

On Tue, 6 Dec 2022 20:45:17 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> > "G - Proprietary module" - "O - out of tree module"
> >
> > Can you reproduce this without those taints?  
> 
> Lol. That question is exactly the reason why my Nack stands.

First, that's a BS reason for a NACK.

But in all seriousness, what I would actually ask (and what I'll ask now)
is, what module did you use that is out of tree, and was it relevant to
this test?

That's a reasonable question to ask, and one that only gets asked with a
taint.


If there's a BPF injection taint, one can ask that same question, as the
bug may happen sometime after the injection but be caused by that injection,
and not be in the backtrace. Not all kernel developers have the access to
debugging utilities that backend production servers have. A lot of bugs that
kernel developers debug are from someone's laptop. Where all they have is
that backtrace. If a tool or root kit, added function error injection, it
would be extremely useful information to debug what happened.

I don't understand the push back here.

-- Steve
  

On Wed, Dec 07, 2022 at 07:48:06AM -0500, Steven Rostedt wrote:
> On Tue, 6 Dec 2022 20:45:17 -0800
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> > > "G - Proprietary module" - "O - out of tree module"
> > >
> > > Can you reproduce this without those taints?  
> > 
> > Lol. That question is exactly the reason why my Nack stands.
> 
> First, that's a BS reason for a NACK.
> 
> But in all seriousness, what I would actually ask (and what I'll ask now)
> is, what module did you use that is out of tree, and was it relevant to
> this test?
> 
> That's a reasonable question to ask, and one that only gets asked with a
> taint.

When we receive bug reports in bpf@vger the only question we ask is:
"How to reproduce this bug?"
We ignoring taint completely.
tbh I didn't even know that our BPF CI causes taint until that email.

In the previous email I said that the bug report comes from bpf selftest on bpf-next.
Clearly there is no ambiguity that this is as upstream as it can get.
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.
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.
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.

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.

> If there's a BPF injection taint, one can ask that same question, as the
> bug may happen sometime after the injection but be caused by that injection,
> and not be in the backtrace. Not all kernel developers have the access to
> debugging utilities that backend production servers have. A lot of bugs that
> kernel developers debug are from someone's laptop.

I would have to disagree.
We see few human reported bugs and prioritize them higher,
but majority are coming from the production fleet, test tiers,
syzbot, CIs, and automated things.

> Where all they have is
> that backtrace. 

Even laptops typically leave vmcore after crash. distro support packages have
clear rules what scripts to run to collect all the necessary data in case
of the crash from vmcore.
These tools support extracting everything bpf related too.
For example see:
Documentation/bpf/drgn.rst
It works on vmcore and on the running kernel.

> If a tool or root kit, added function error injection, it
> would be extremely useful information to debug what happened.
> 
> I don't understand the push back here.

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.

One can monitor all prog load/unload via perf or via audit.
'perf record' collects all bpf progs that were loaded before
the start and even thouse that were loaded and unloaded quickly
while 'perf record' was running.
So 'perf report' has all the data including annotation and source code.

Currently we're working on adding 'file.c:lineno' to dump_stack()
for bpf progs.

If you have ideas how we can improve debuggability we are all ears.
  

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

> > But in all seriousness, what I would actually ask (and what I'll ask now)
> > is, what module did you use that is out of tree, and was it relevant to
> > this test?
> > 
> > That's a reasonable question to ask, and one that only gets asked with a
> > taint.  
> 
> When we receive bug reports in bpf@vger the only question we ask is:
> "How to reproduce this bug?"
> We ignoring taint completely.

Um, I'm guessing that bug reports that go to bpf@vger are focused on issues
with BPF and not core kernel infrastructure. No, I do not consider BPF (nor
tracing for that matter) as core infrastructure. The bug reports I'm
talking about is for the scheduler, exception handling, interrupts, VFS,
MM, and networking. When one of these have a bug report, you most
definitely do look at taints. There's been several times where a bug report
was caused by an out of tree module, or a machine check exception.

> tbh I didn't even know that our BPF CI causes taint until that email.

That's because BPF development probably never gets hit by things that cause
taints. That doesn't go for the rest of the kernel.

> 
> In the previous email I said that the bug report comes from bpf selftest on bpf-next.
> Clearly there is no ambiguity that this is as upstream as it can get.
> Yet for 2 days this 'taint' arguing is preventing people from looking at the bug.

Who isn't looking at the bug because of the 'taint' argument? I'm not
looking at it right now, because it isn't top of my priority list.


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

So is spending 5 days debugging something and then finding out that what
caused the taint *was* the culprit!

Sorry, but that has happened to me too many times. Which is why you get
grumpy kernel developers pushing back on this.

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

Again, I've been burned by OoO modules more than once. If you send 5 days
debugging something to find out that the cause was from an OoO module,
you'd change your tune too.

> 
> 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.
> 
> > If there's a BPF injection taint, one can ask that same question, as the
> > bug may happen sometime after the injection but be caused by that injection,
> > and not be in the backtrace. Not all kernel developers have the access to
> > debugging utilities that backend production servers have. A lot of bugs that
> > kernel developers debug are from someone's laptop.  
> 
> I would have to disagree.
> We see few human reported bugs and prioritize them higher,
> but majority are coming from the production fleet, test tiers,
> syzbot, CIs, and automated things.
> 
> > Where all they have is
> > that backtrace.   
> 
> Even laptops typically leave vmcore after crash. distro support packages have
> clear rules what scripts to run to collect all the necessary data in case
> of the crash from vmcore.

They are not as common as you think.

> These tools support extracting everything bpf related too.
> For example see:
> Documentation/bpf/drgn.rst
> It works on vmcore and on the running kernel.
> 
> > If a tool or root kit, added function error injection, it
> > would be extremely useful information to debug what happened.
> > 
> > I don't understand the push back here.  
> 
> 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.
> 
> One can monitor all prog load/unload via perf or via audit.
> 'perf record' collects all bpf progs that were loaded before
> the start and even thouse that were loaded and unloaded quickly
> while 'perf record' was running.
> So 'perf report' has all the data including annotation and source code.
> 
> Currently we're working on adding 'file.c:lineno' to dump_stack()
> for bpf progs.
> 
> If you have ideas how we can improve debuggability we are all ears.

I talked with KP on IRC, and he said he's going to work on the same thing
as above. That is, showing what BPF programs are loaded, and if they modify
the return of any function, as well as what those functions are. And have
that reported in the oops message.

I'm not hell bent on BPF triggering a taint, as long as the oops message
explicitly defines what BPF is doing. I think that's much more informative
than a taint, and I would greatly welcome that. The main issue I'm raising
is that I want the oops message to have most the information to gather what
went wrong, and not rely on other tooling like kernel core dumps and drgn
for debugging the problem.

-- Steve
  

Hi Alexei,

On Wed, 7 Dec 2022 20:36:28 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.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,
  

On Sun, Dec 11, 2022 at 3:52 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi Alexei,
>
> On Wed, 7 Dec 2022 20:36:28 -0800
> Alexei Starovoitov <alexei.starovoitov@gmail.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.

The series is about adding a taint for, so discussing the user
experience, when someone reports a "tainted crash" seems reasonable to
me and not off topic.

>
> > 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??

Masami, this seems accusational and counter productive, it was never
said that issues can be solved without investigation.

The BPF CI does find issues, the BPF reviewers and maintainers
regularly fix bugs using it. Alexei's point here is that a taint does
not help in solving the problem, rather deter some people from even
looking at it. (not BPF people, but other maintainers [distro, kernel]
who would ask for a reproduction without a taint).

Let's take a step back and focus on solving debuggability and
introspection as we clearly have some perception issues about taints
in the community. (distro maintainers, users) before we go and add
more taints.

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

I am sorry, but it's our responsibility as developers to design
features so that users don't face arduous pushbacks.

> > 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)

This is already the case with BPF, the modify return trampolines
further limits the error injection to functions that return errors.

> - 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)

This is the case with modify_return trampolines in BPF which avoid
side effects and limit the attachment surface further and avoiding
side effects is a design goal. If we missed anything, let's fix that.

https://lwn.net/Articles/813724/https://lwn.net/Articles/813724/

>
> 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)

I think careful choices need to be made on when error injection is
allowed so that these situations don't occur. (as you mentioned in
your comment). [1]. If a BPF program is unloaded, there is no error
injection any more, let's ensure that we design the error injection
allow list and the BPF logic to ensure this cannot happen.


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

I would like us to focus on debuggability as it helps both the
maintainers and the user. And I see a few things that need to be done:

1. Revisit what is allowed for error injection in the kernel and if
they can cause any subtle issues. My initial take is that functions
that are directly called from syscall path should generally be okay.
But let's check them for the patterns you mentioned.
2. If it helps, add the list of BPF modify return programs to stack
traces. Although this is really needed if we don't do [1] properly.
3. Check if anything needs to be improved in the verification logic
for modify return trampolines.

- KP

>
> Ah, audit is helpful :), because we can dig the log what was loaded
> before crash.
>
>
> Thank you,
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
  

On Sun, 11 Dec 2022 08:49:01 +0100
KP Singh <kpsingh@kernel.org> wrote:

> On Sun, Dec 11, 2022 at 3:52 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Hi Alexei,
> >
> > On Wed, 7 Dec 2022 20:36:28 -0800
> > Alexei Starovoitov <alexei.starovoitov@gmail.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.
> 
> The series is about adding a taint for, so discussing the user
> experience, when someone reports a "tainted crash" seems reasonable to
> me and not off topic.
> 
> >
> > > 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??
> 
> Masami, this seems accusational and counter productive, it was never
> said that issues can be solved without investigation.

Let me apologies about my misunderstanding.  

> 
> The BPF CI does find issues, the BPF reviewers and maintainers
> regularly fix bugs using it. Alexei's point here is that a taint does
> not help in solving the problem, rather deter some people from even
> looking at it. (not BPF people, but other maintainers [distro, kernel]
> who would ask for a reproduction without a taint).

Hmm, that is a problem. Some taint flag should be useful hints
for finding the error patterns.

> Let's take a step back and focus on solving debuggability and
> introspection as we clearly have some perception issues about taints
> in the community. (distro maintainers, users) before we go and add
> more taints.

Agreed.

> > > 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.
> 
> I am sorry, but it's our responsibility as developers to design
> features so that users don't face arduous pushbacks.

Sorry if I confuse you. I meant that taint flag abusing. :(

> 
> > > 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)
> 
> This is already the case with BPF, the modify return trampolines
> further limits the error injection to functions that return errors.

OK, so I also should remove it from FEI.

> 
> > - 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)
> 
> This is the case with modify_return trampolines in BPF which avoid
> side effects and limit the attachment surface further and avoiding
> side effects is a design goal. If we missed anything, let's fix that.
> 
> https://lwn.net/Articles/813724/

Yeah, if BPF tests already tested all ALLOW_ERROR_INJECTION() functions,
it may be checked already. I think we just need adding the above
explanation on the document, so that the people who will add additional
ALLOW_ERROR_INJECTION() on a function, can understand the limitation.

> 
> >
> > 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)
> 
> I think careful choices need to be made on when error injection is
> allowed so that these situations don't occur. (as you mentioned in
> your comment). [1]. If a BPF program is unloaded, there is no error
> injection any more, let's ensure that we design the error injection
> allow list and the BPF logic to ensure this cannot happen.

OK. Actually, I trust the BPF logic itself will be handle this
correctly. I just concerned that some people who don't know much
(because it is not carefully documented) might add ALLOW_ERROR_INJECTION()
to a function which is not injectable by design. Thus I thought the
taint flag can help.
But if those are always Cc'd to bpf@vger and it will be tested by BPF
CI, I'm OK for that.

> > > One can monitor all prog load/unload via perf or via audit.
> 
> I would like us to focus on debuggability as it helps both the
> maintainers and the user. And I see a few things that need to be done:
> 
> 1. Revisit what is allowed for error injection in the kernel and if
> they can cause any subtle issues. My initial take is that functions
> that are directly called from syscall path should generally be okay.
> But let's check them for the patterns you mentioned.

Yeah, I agree that syscall entries should be safe.

> 2. If it helps, add the list of BPF modify return programs to stack
> traces. Although this is really needed if we don't do [1] properly.

Would you mean a list of enabled BPF programs in Oops code? If so,
I also want to add enabled FEI list on it.

> 3. Check if anything needs to be improved in the verification logic
> for modify return trampolines.

I think BPF logic itself is safe. But the targeted function itself
or the caller may not be designed for such error injection.
I think this is my fault that I have not documented about
ALLOW_ERROR_INJECTION() well. Sorry about that.

Thank you,
  

On Sun, 11 Dec 2022 08:49:01 +0100
KP Singh <kpsingh@kernel.org> wrote:

> Let's take a step back and focus on solving debuggability and
> introspection as we clearly have some perception issues about taints
> in the community. (distro maintainers, users) before we go and add
> more taints.

Note, you will likely get the same push back if the dump includes bpf
programs known to change the return of a function that may be involved
with the bug report. That is, if a crash is reported to code I
maintain, and I see that the bug report includes a list of BPF programs
that can modify the return of a function, and one of those functions
could affect the place that crashed, I'd push back and ask if the crash
could be done without that BPF program loaded, regardless of taints.

I agree that a taint is just a hint and it can include something that
caused the bug or it may not. I would like to see more details in how
the crashed kernel was configured. That includes loaded BPF programs
(just like we include loaded modules). And if any BPF program modifies
a core function (outside of syscall returns) I'd be a bit suspect of
what happened.

I also agree that if a function that checks error paths fails, it
should be fixed, but knowing that the error path was caused by fault
injection will prevent the wasted effort that most developers will go
through to find out why the error path was hit in the first place.

-- Steve
  

Hi,

On Sun, 11 Dec 2022 08:49:01 +0100
KP Singh <kpsingh@kernel.org> wrote:

> 1. Revisit what is allowed for error injection in the kernel and if
> they can cause any subtle issues. My initial take is that functions
> that are directly called from syscall path should generally be okay.
> But let's check them for the patterns you mentioned.
> 2. If it helps, add the list of BPF modify return programs to stack
> traces. Although this is really needed if we don't do [1] properly.
> 3. Check if anything needs to be improved in the verification logic
> for modify return trampolines.

Hmm, I found that bpf might not check the acceptable error type of
each ALLOW_ERROR_INJECTION().

Except for EI_ETYPE_NONE, we have 4 types of the error.

        EI_ETYPE_NULL,          /* Return NULL if failure */
        EI_ETYPE_ERRNO,         /* Return -ERRNO if failure */
        EI_ETYPE_ERRNO_NULL,    /* Return -ERRNO or NULL if failure */
        EI_ETYPE_TRUE,          /* Return true if failure */

These specifies that what return value will be treated as an error
by the caller.

If bpf trampoline only expect that the function will return -errno
in error cases, bpf should check the error type as below.

etype = get_injectable_error_type(addr);
if (etype != EI_ETYPE_ERRNO && etype != EI_ETYPE_ERRNO_NULL)
	/* reject it */

If bpf can handle any case, it still need to verify that the user
bpf prog specifies correct return value for each type.
See adjust_error_retval()@kernel/fail_function.c for the available
return values.

Thank you,
  

On Sun, Dec 11, 2022 at 4:14 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Sun, 11 Dec 2022 08:49:01 +0100
> KP Singh <kpsingh@kernel.org> wrote:
>
> > On Sun, Dec 11, 2022 at 3:52 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > Hi Alexei,
> > >
> > > On Wed, 7 Dec 2022 20:36:28 -0800
> > > Alexei Starovoitov <alexei.starovoitov@gmail.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.
> >
> > The series is about adding a taint for, so discussing the user
> > experience, when someone reports a "tainted crash" seems reasonable to
> > me and not off topic.
> >
> > >
> > > > 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??
> >
> > Masami, this seems accusational and counter productive, it was never
> > said that issues can be solved without investigation.
>
> Let me apologies about my misunderstanding.

e-mail is hard, I am glad this is already progressing constructively
and we are making things better.

>
> >
> > The BPF CI does find issues, the BPF reviewers and maintainers
> > regularly fix bugs using it. Alexei's point here is that a taint does
> > not help in solving the problem, rather deter some people from even
> > looking at it. (not BPF people, but other maintainers [distro, kernel]
> > who would ask for a reproduction without a taint).
>
> Hmm, that is a problem. Some taint flag should be useful hints
> for finding the error patterns.

the boolean information contained in taints is not helpful, stuff like
audit logs / crash dumps / bpftool reports are much more helpful.
>
> > Let's take a step back and focus on solving debuggability and
> > introspection as we clearly have some perception issues about taints
> > in the community. (distro maintainers, users) before we go and add
> > more taints.
>
> Agreed.
>
> > > > 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.
> >
> > I am sorry, but it's our responsibility as developers to design
> > features so that users don't face arduous pushbacks.
>
> Sorry if I confuse you. I meant that taint flag abusing. :(

Again no worries, but we need to make sure that the taint flag is not
abused, and if it is being abused, limit the damage somewhere.

>
> >
> > > > 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)
> >
> > This is already the case with BPF, the modify return trampolines
> > further limits the error injection to functions that return errors.
>
> OK, so I also should remove it from FEI.
>
> >
> > > - 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)
> >
> > This is the case with modify_return trampolines in BPF which avoid
> > side effects and limit the attachment surface further and avoiding
> > side effects is a design goal. If we missed anything, let's fix that.
> >
> > https://lwn.net/Articles/813724/
>
> Yeah, if BPF tests already tested all ALLOW_ERROR_INJECTION() functions,
> it may be checked already. I think we just need adding the above
> explanation on the document, so that the people who will add additional
> ALLOW_ERROR_INJECTION() on a function, can understand the limitation.
>
> >
> > >
> > > 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)
> >
> > I think careful choices need to be made on when error injection is
> > allowed so that these situations don't occur. (as you mentioned in
> > your comment). [1]. If a BPF program is unloaded, there is no error
> > injection any more, let's ensure that we design the error injection
> > allow list and the BPF logic to ensure this cannot happen.
>
> OK. Actually, I trust the BPF logic itself will be handle this
> correctly. I just concerned that some people who don't know much
> (because it is not carefully documented) might add ALLOW_ERROR_INJECTION()
> to a function which is not injectable by design. Thus I thought the
> taint flag can help.

We should definitely carefully review any new ALLOW_ERROR_INJECTION
functions, that's the real value add.

> But if those are always Cc'd to bpf@vger and it will be tested by BPF
> CI, I'm OK for that.
>
> > > > One can monitor all prog load/unload via perf or via audit.
> >
> > I would like us to focus on debuggability as it helps both the
> > maintainers and the user. And I see a few things that need to be done:
> >
> > 1. Revisit what is allowed for error injection in the kernel and if
> > they can cause any subtle issues. My initial take is that functions
> > that are directly called from syscall path should generally be okay.
> > But let's check them for the patterns you mentioned.
>
> Yeah, I agree that syscall entries should be safe.
>
> > 2. If it helps, add the list of BPF modify return programs to stack
> > traces. Although this is really needed if we don't do [1] properly.
>
> Would you mean a list of enabled BPF programs in Oops code? If so,
> I also want to add enabled FEI list on it.
>
> > 3. Check if anything needs to be improved in the verification logic
> > for modify return trampolines.
>
> I think BPF logic itself is safe. But the targeted function itself
> or the caller may not be designed for such error injection.
> I think this is my fault that I have not documented about
> ALLOW_ERROR_INJECTION() well. Sorry about that.

We aim to be blameless and constructive :)

Thanks again for sending the patches!

Cheers,
- KP

>
> Thank you,
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
  

On Sun, Dec 11, 2022 at 6:02 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Sun, 11 Dec 2022 08:49:01 +0100
> KP Singh <kpsingh@kernel.org> wrote:
>
> > Let's take a step back and focus on solving debuggability and
> > introspection as we clearly have some perception issues about taints
> > in the community. (distro maintainers, users) before we go and add
> > more taints.
>
> Note, you will likely get the same push back if the dump includes bpf
> programs known to change the return of a function that may be involved
> with the bug report. That is, if a crash is reported to code I
> maintain, and I see that the bug report includes a list of BPF programs
> that can modify the return of a function, and one of those functions
> could affect the place that crashed, I'd push back and ask if the crash
> could be done without that BPF program loaded, regardless of taints.
>

I think this is already better as it gives the recipient of the stack
trace more information to ask more questions and see if the BPF
programs are relevant to the stack trace and engage further.

> I agree that a taint is just a hint and it can include something that
> caused the bug or it may not. I would like to see more details in how
> the crashed kernel was configured. That includes loaded BPF programs
> (just like we include loaded modules). And if any BPF program modifies
> a core function (outside of syscall returns) I'd be a bit suspect of
> what happened.

Agreed.

>
> I also agree that if a function that checks error paths fails, it
> should be fixed, but knowing that the error path was caused by fault
> injection will prevent the wasted effort that most developers will go
> through to find out why the error path was hit in the first place.

Agreed.

>
> -- Steve