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;
MIME-Version: 1.0
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>
In-Reply-To: <CAADnVQ+KLXPhowdBZq0PvGOq5tv4ovNtNCvGBXHQBkVbz4UVkg@mail.gmail.com>
From:   Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date:   Thu, 1 Dec 2022 18:39:29 +0100
Message-ID: <CAO-hwJ+PyqBPAO-3MmR+WFKiRzX-FPd41uNR5tX9sgtgdyf2Og@mail.gmail.com>
Subject: Re: [PATCH] error-injection: Add prompt for function error injection
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
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>,
        Linus Torvalds <torvalds@linux-foundation.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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Dec 1, 2022 at 5:59 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Nov 30, 2022 at 2:37 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Tue, 22 Nov 2022 14:51:08 -0500 Chris Mason <clm@meta.com> wrote:
> >
> > > On 11/22/22 1:29 PM, Steven Rostedt wrote:
> > > > On Tue, 22 Nov 2022 12:42:33 -0500
> > > > Chris Mason <clm@meta.com> wrote:
> > > >
> > > >> On 11/22/22 5:39 AM, Borislav Petkov wrote:
> > > >>> On Mon, Nov 21, 2022 at 03:36:08PM -0800, Alexei Starovoitov wrote:
> > > >>>> The commit log is bogus and the lack of understanding what
> >
> > Why am I not understanding the controversy here?  With this patch
> > applied, people who want function error injection enable
> > CONFIG_FUNCTION_ERROR_INJECTION and people who don't want it don't do
> > that.
> >
> > Alexei, can you please suggest a less bogus changelog for this?
>
> People are using ALLOW_ERROR_INJECTION to allowlist kernel functions
> where bpf progs can attach.
> For example in the linux-next:
> git grep ALLOW_ERROR_INJECTION
> drivers/hid/bpf/hid_bpf_dispatch.c:ALLOW_ERROR_INJECTION(hid_bpf_device_event,
> ERRNO);
> drivers/hid/bpf/hid_bpf_dispatch.c:ALLOW_ERROR_INJECTION(hid_bpf_rdesc_fixup,
> ERRNO);
> drivers/hid/bpf/hid_bpf_jmp_table.c:ALLOW_ERROR_INJECTION(__hid_bpf_tail_call,
> ERRNO);

To be completely honest, I mark hid_bpf_device_event and
hid_bpf_rdesc_fixup as ALLOW_ERROR_INJECTION only to have a dedicated
function to create a SEC("fmod_ret"), but I am actually only using
__hid_bpf_tail_call() to call the bpf programs.

So technically, I should be able to not use ALLOW_ERROR_INJECTION but
that would mean manually calling the BPF programs from
__hid_bpf_tail_call() instead of relying on the magic of libbpf, but
also would force me to have an other way of telling these 2 other
functions are fmod_ret capable, which would be definitely not clean.

>
> The hid-bpf framework depends on it.
> iirc Benjamin mentioned that chromeos is one of the future users
> of hid-bpf. They need it to deal with a variety of quirks in hid
> devices in production.
>
> Either hid-bpf or bpf core can add
> "depends on FUNCTION_ERROR_INJECTION"
> to its kconfig.
> Like:
> diff --git a/kernel/bpf/Kconfig b/kernel/bpf/Kconfig
> index 2dfe1079f772..281e5263f3d1 100644
> --- a/kernel/bpf/Kconfig
> +++ b/kernel/bpf/Kconfig
> @@ -32,6 +32,7 @@ config BPF_SYSCALL
>         select BINARY_PRINTF
>         select NET_SOCK_MSG if NET
>         select PAGE_POOL if NET
> +       depends on FUNCTION_ERROR_INJECTION
>         default n

FWIW, this is what I'm going to apply in hid.git for the time being
[0]. But I'd rather have a BPF_HAVE_FMOD_RET as suggested in [1].

>
> but the better option for now would be to drop this patch.
> For the next next merge window we can come up with alternative way
> (instead of ALLOW_ERROR_INJECTION) to mark kernel functions
> purely on the bpf side.
> I don't think we have time to add this marking infrastructure
> for the upcoming merge window.
>

Outside of the "should we add ALLOW_ERROR_INJECTION in production
environments", all I care about is that I want to be able to attach
SEC("fmod_ret/...") on a specific set of functions that I control. So
for me, I don't need to full "let's randomly add errors in any
functions" (which I doubt you can do with ALLOW_ERROR_INJECTION
anyway), I just need to be able to say "I want a bpf program to be
able to change the return code of this dedicated function".

So I agree with Alexei here. The situation has been to enable this
parameter for quite some time without any complaints, and this patch
prevents HID-BPF to be enabled on systems where sysadmins would think
this is unsafe. Postponing this patch to the next merge window will
give enough time for the BPF folks to change their implementation.

Cheers,
Benjamin

[0] https://lore.kernel.org/linux-input/7df26319-f4ee-6dd1-a1b8-1caaf595528d@nvidia.com/T/#m9416ad54e2ef63244585c4ef83d07bebedf6e143
[1] https://lore.kernel.org/linux-input/CABRcYmKyRchQhabi1Vd9RcMQFCcb=EtWyEbFDFRTc-L-U8WhgA@mail.gmail.com/