Received-SPF: pass (google.com: domain of linux-kernel+bounces-153784-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 22 Apr 2024 19:07:58 +0200
From: Benjamin Tissoires <bentiss@kernel.org>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: 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@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] bpf: verifier: allow arrays of progs to be used in
 sleepable context
Message-ID: <un4jw2ef45vu3vwojpjca3wezso7fdp5gih7np73f4pmsmhmaj@csm3ix2ygd5i>
References: <20240422-sleepable_array_progs-v1-1-7c46ccbaa6e2@kernel.org>
 <7344022a-6f59-7cbf-ee45-6b7d59114be6@iogearbox.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7344022a-6f59-7cbf-ee45-6b7d59114be6@iogearbox.net>

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 only
> > 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 sleepable
> > 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 sleepable 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 this
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.

Cheers,
Benjamin

> 
> >   kernel/bpf/verifier.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 68cfd6fc6ad4..880b32795136 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -18171,6 +18171,7 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
> >   		case BPF_MAP_TYPE_QUEUE:
> >   		case BPF_MAP_TYPE_STACK:
> >   		case BPF_MAP_TYPE_ARENA:
> > +		case BPF_MAP_TYPE_PROG_ARRAY:
> >   			break;
> >   		default:
> >   			verbose(env,
> > 
> > ---
> > base-commit: 735f5b8a7ccf383e50d76f7d1c25769eee474812
> > change-id: 20240422-sleepable_array_progs-e0c07b17cabb
> > 
> > Best regards,
> > 
>