Received-SPF: pass (google.com: domain of linux-kernel+bounces-157625-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240422-sleepable_array_progs-v1-1-7c46ccbaa6e2@kernel.org>
 <7344022a-6f59-7cbf-ee45-6b7d59114be6@iogearbox.net> <un4jw2ef45vu3vwojpjca3wezso7fdp5gih7np73f4pmsmhmaj@csm3ix2ygd5i>
 <35nbgxc7hqyef3iobfvhbftxtbxb3dfz574gbba4kwvbo6os4v@sya7ul5i6mmd>
In-Reply-To: <35nbgxc7hqyef3iobfvhbftxtbxb3dfz574gbba4kwvbo6os4v@sya7ul5i6mmd>
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Wed, 24 Apr 2024 12:45:03 -0700
Message-ID: <CAADnVQJaG8kDaJr5LV29ces+gVpgARLAWiUvE9Ee5huuiW5X=Q@mail.gmail.com>
Subject: Re: [PATCH] bpf: verifier: allow arrays of progs to be used in
 sleepable context
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>, Alexei Starovoitov <ast@kernel.org>, 
	John Fastabend <john.fastabend@gmail.com>, Andrii Nakryiko <andrii@kernel.org>, 
	Martin KaFai Lau <martin.lau@linux.dev>, Eduard Zingerman <eddyz87@gmail.com>, Song Liu <song@kernel.org>, 
	Yonghong Song <yonghong.song@linux.dev>, KP Singh <kpsingh@kernel.org>, 
	Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>, 
	bpf <bpf@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 24, 2024 at 7:17=E2=80=AFAM Benjamin Tissoires <bentiss@kernel.=
org> wrote:
>
> On Apr 22 2024, Benjamin Tissoires wrote:
> > On Apr 22 2024, Daniel Borkmann wrote:
> > > On 4/22/24 9:16 AM, Benjamin Tissoires wrote:
> > > > Arrays of progs are underlying using regular arrays, but they can o=
nly
> > > > be updated from a syscall.
> > > > Therefore, they should be safe to use while in a sleepable context.
> > > >
> > > > This is required to be able to call bpf_tail_call() from a sleepabl=
e
> > > > tracing bpf program.
> > > >
> > > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> > > > ---
> > > > Hi,
> > > >
> > > > a small patch to allow to have:
> > > >
> > > > ```
> > > > SEC("fmod_ret.s/__hid_bpf_tail_call_sleepable")
> > > > int BPF_PROG(hid_tail_call_sleepable, struct hid_bpf_ctx *hctx)
> > > > {
> > > >   bpf_tail_call(ctx, &hid_jmp_table, hctx->index);
> > > >
> > > >   return 0;
> > > > }
> > > > ```
> > > >
> > > > This should allow me to add bpf hooks to functions that communicate=
 with
> > > > the hardware.
> > >
> > > Could you also add selftests to it? In particular, I'm thinking that =
this is not
> > > sufficient given also bpf_prog_map_compatible() needs to be extended =
to check on
> > > prog->sleepable. For example we would need to disallow calling sleepa=
ble programs
> > > in that map from non-sleepable context.
> >
> > Just to be sure, if I have to change bpf_prog_map_compatible(), that
> > means that a prog array map can only have sleepable or non-sleepable
> > programs, but not both at the same time?
> >
> > FWIW, indeed, I just tested and the BPF verifier/core is happy with thi=
s
> > patch only if the bpf_tail_call is issued from a non-sleepable context
> > (and crashes as expected).
> >
> > But that seems to be a different issue TBH: I can store a sleepable BPF
> > program in a prog array and run it from a non sleepable context. I don'=
t
> > need the patch at all as bpf_tail_call() is normally declared. I assume
> > your suggestion to change bpf_prog_map_compatible() will fix that part.
> >
> > I'll digg some more tomorrow.
> >
>
> Quick update:
> forcing the prog array to only contain sleepable programs or not seems
> to do the trick, but I'm down a rabbit hole as when I return from my
> trampoline, I get an invalid page fault, trying to execute NX-protected
> page.
>
> I'll report if it's because of HID-BPF or if there are more work to be
> doing for bpf_tail_call (which I suspect).

bpf_tail_call is an old mechanism.
Instead of making it work for sleepable (which is ok to do)
have you considered using "freplace" logic to "add bpf hooks to functions" =
?
You can have a global noinline function and replace it at run-time
with another bpf program.
Like:
__attribute__ ((noinline))
int get_constant(long val)
{
        return val - 122;
}

in progs/test_pkt_access.c

is replaced with progs/freplace_get_constant.c

With freplace you can pass normal arguments, do the call and get
return value, while with bpf_tail_call it's ctx only and no return.