Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220421140740.459558-1-benjamin.tissoires@redhat.com>
 <20220421140740.459558-4-benjamin.tissoires@redhat.com> <20220426041147.gwnxhcjftl2kaz6g@MBP-98dd607d3435.dhcp.thefacebook.com>
 <CAO-hwJLWxtZcs-ynzAaF4hGf6zPF5wAni3Etzb1_XrvQpx2Jxw@mail.gmail.com> <CAADnVQL3U5Gz_spmdTsC6zOuR=XLEMppwn+=bkyOsSt0=7pP7Q@mail.gmail.com>
In-Reply-To: <CAADnVQL3U5Gz_spmdTsC6zOuR=XLEMppwn+=bkyOsSt0=7pP7Q@mail.gmail.com>
From:   Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date:   Sat, 30 Apr 2022 09:24:31 +0200
Message-ID: <CAO-hwJL8TQ2wkFYpR99S7zsujPrH2NC1Wp13EvkyzijkY6oY8A@mail.gmail.com>
Subject: Re: [RFC bpf-next v4 3/7] error-inject: add new type that carries if
 the function is non sleepable
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        Jiri Kosina <jikos@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Tero Kristo <tero.kristo@linux.intel.com>,
        lkml <linux-kernel@vger.kernel.org>,
        "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Apr 30, 2022 at 5:30 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Apr 26, 2022 at 12:52 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Tue, Apr 26, 2022 at 6:11 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.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@redhat.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
>

OK, I'll work on this in v5.

But I need to rethink the whole sleepable/non-sleepable definitions
for my use case, because I have now a clear separation between not
sleepable context (in fentry/fexit/fmod_ret) and sleepable context (in
SEC("syscall")), so maybe the whole thing is not really required.

Cheers,
Benjamin