linux-kernel - Re: [RFC bpf-next v4 3/7] error-inject: add new type that carries if the function is non sleepable

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]

Date:   Fri, 29 Apr 2022 20:29:43 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Jiri Kosina <jikos@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Tero Kristo <tero.kristo@...ux.intel.com>,
        lkml <linux-kernel@...r.kernel.org>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [RFC bpf-next v4 3/7] error-inject: add new type that carries if
 the function is non sleepable

On Tue, Apr 26, 2022 at 12:52 AM Benjamin Tissoires
<benjamin.tissoires@...hat.com> wrote:
>
> On Tue, Apr 26, 2022 at 6:11 AM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> >
> > On Thu, Apr 21, 2022 at 04:07:36PM +0200, Benjamin Tissoires wrote:
> > > When using error-injection function through bpf to change the return
> > > code, we need to know if the function is sleepable or not.
> > >
> > > Currently the code assumes that all error-inject functions are sleepable,
> > > except for a few selected of them, hardcoded in kernel/bpf/verifier.c
> > >
> > > Add a new flag to error-inject so we can code that information where the
> > > function is declared.
> > >
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
> > >
> > > ---
> > >
> > > new in v4:
> > > - another approach would be to define a new kfunc_set, and register
> > >   it with btf. But in that case, what program type would we use?
> > >   BPF_PROG_TYPE_UNSPEC?
> > > - also note that maybe we should consider all of the functions
> > >   non-sleepable and only mark some as sleepable. IMO it makes more
> > >   sense to be more restrictive by default.
> >
> > I think the approach in this patch is fine.
> > We didn't have issues with check_non_sleepable_error_inject() so far,
> > so I wouldn't start refactoring it.
>
> OK... though I can't help but thinking that adding a new
> error-inject.h enum value is going to be bad, because it's an API
> change, and users might not expect NS_ERRNO.

Not sure about api concern. This is the kernel internal tag.
bpf progs are not aware of them. The functions can change
from sleepable to non-sleepable too.
allow_error_inject can be removed. And so on.

> OTOH, if we had a new kfunc_set, we keep the existing error-inject API
> in place with all the variants and we just teach the verifier that the
> function is non sleepable.
...
> IIUC, the kfunc_set approach would solve that, no?

Makes sense. Let's figure out an extensible kfunc_set approach
that is not centralized in verifier.c