Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Dec 2022 17:41:29 -0800
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Chris Mason <clm@meta.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Borislav Petkov <bp@alien8.de>,
        LKML <linux-kernel@vger.kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Kees Cook <keescook@chromium.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        KP Singh <kpsingh@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Florent Revest <revest@chromium.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Christoph Hellwig <hch@infradead.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>
Subject: Re: [PATCH] error-injection: Add prompt for function error injection
Message-ID: <20221202014129.n5lmvzsy436ebo4b@macbook-pro-6.dhcp.thefacebook.com>
References: <20221121104403.1545f9b5@gandalf.local.home>
 <Y3vSQo85ofkfD/L8@zn.tnic>
 <CAADnVQLJFnu6gARdZ7ckgxeGaSv70jHBiQo+W=zDKO0-ELFGcQ@mail.gmail.com>
 <Y3ymzAYRyH5IWEe/@zn.tnic>
 <3fa8ec60-dd96-c41f-ea46-8856bf855949@meta.com>
 <20221122132905.12a8d5ad@gandalf.local.home>
 <a53e8c6d-b0e5-72bf-1aba-07609c9cf1bf@meta.com>
 <20221130143719.07e36277d1471b83e9a1b627@linux-foundation.org>
 <CAADnVQ+KLXPhowdBZq0PvGOq5tv4ovNtNCvGBXHQBkVbz4UVkg@mail.gmail.com>
 <CAHk-=wjeq1m=9mU17WzfRQ1W6N0SgKHY-e2J35SpppWwUUBFbQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wjeq1m=9mU17WzfRQ1W6N0SgKHY-e2J35SpppWwUUBFbQ@mail.gmail.com>
Precedence: bulk

On Thu, Dec 01, 2022 at 01:13:35PM -0800, Linus Torvalds wrote:
> 
> I'm going to apply Steven's patch, because honestly, we need to fix
> this disgusting mess *before* it gets to mainline, rather than say
> "oh, we already have broken users in next, so let's bend over
> backwards for that".
> 
> The code is called "error injection", not "random bpf extension"

Right, ALLOW_ERROR_INJECTION doesn't fit hid-bpf.
As Benjamin clarified for hid-bpf we don't want non-bpf attach to
those 3 functions. Injecting errors there is not useful.
We'll come with some clean mechanism to express "attach bpf here".

But let's examine other places of "error injection".

The following are clearly falling into kernel debugging category:
mm/filemap.c:ALLOW_ERROR_INJECTION(__filemap_add_folio, ERRNO);
mm/page_alloc.c:ALLOW_ERROR_INJECTION(should_fail_alloc_page, TRUE);
mm/slab_common.c:ALLOW_ERROR_INJECTION(should_failslab, ERRNO);

The distros might disable such hooks while data centers might still
enable them. Consider chaosmonkey and other frameworks that stress
user space. They are used on non-production user space while
running on production kernel.
The cloud providers wouldn't want to spin another kernel flavor
with fault injection enabled just to satisfy this set of users.
So the FUNCTION_ERROR_INJECTION kconfig is absolutely necessary.
Whether it defaults to N or Y, doesn't matter much.

But where would you draw the line for:
include/linux/syscalls.h: ALLOW_ERROR_INJECTION(sys_##sname, ERRNO);

Right now people can add to their .bashrc:

cd /sys/kernel/debug/fail_function/
echo __x64_sys_bpf > inject
echo 0xffffFFFF > times
echo 100 > probability
echo 0 > verbose

and stop their favorite syscall ever happening on their laptops after boot.

The fault injection framework disables individual syscall with zero performance
overhead comparing to LSM and seccomp mechanisms.
BPF is not involved here. It's a kprobe in one spot.
All other syscalls don't notice it.
It's an attractive way to improve security.

A BPF prog over syscall can filter by user, cgroup, task and give fine grain
control over security surface.
tbh I'm not aware of folks doing "syscall disabling" through command line like
above (I've only seen it through bpf), but it doesn't mean that somebody will
not start complaining that their script broke, because distro disabled fault
injection.

So should we split FUNCTION_ERROR_INJECTION kconfig into two ?
And do default N for things like should_failslab() and
default Y for syscalls?

In the other thread TAINT_FAULT_INJECTED was proposed.
I think it's fine to taint when injecting errors into should_failslab(),
but tainting when syscall is disabled feels wrong.

One can argue that ALLOW_ERROR_INJECTION(sys_##sname, ERRNO); was an abuse of
fault injection, but let's keep it aside and focus on moving forward from here.