Received-SPF: pass (google.com: domain of linux-kernel+bounces-83585-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 27 Feb 2024 17:18:19 +0100
From: Benjamin Tissoires <bentiss@kernel.org>
To: Eduard Zingerman <eddyz87@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>, 
	Daniel Borkmann <daniel@iogearbox.net>, John Fastabend <john.fastabend@gmail.com>, 
	Andrii Nakryiko <andrii@kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>, 
	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>, Jiri Kosina <jikos@kernel.org>, 
	Benjamin Tissoires <benjamin.tissoires@redhat.com>, Jonathan Corbet <corbet@lwn.net>, Shuah Khan <shuah@kernel.org>, 
	bpf@vger.kernel.org, linux-kernel@vger.kernel.org, linux-input@vger.kernel.org, 
	linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH RFC bpf-next v3 08/16] bpf/verifier: do_misc_fixups for
 is_bpf_timer_set_sleepable_cb_kfunc
Message-ID: <pocfd5n6lxriqg7r6usyhrlprgslclxs44jqoq63lw734fjl2g@5kv4hjaux2fp>
References: <20240221-hid-bpf-sleepable-v3-0-1fb378ca6301@kernel.org>
 <20240221-hid-bpf-sleepable-v3-8-1fb378ca6301@kernel.org>
 <55177311ccdc24a74811d4a291ee1880044a5227.camel@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <55177311ccdc24a74811d4a291ee1880044a5227.camel@gmail.com>

On Feb 23 2024, Eduard Zingerman wrote:
> On Wed, 2024-02-21 at 17:25 +0100, Benjamin Tissoires wrote:
> > This is still a WIP, but I think this can be dropped as we never
> > get to this instruction. So what should we do here?
> 
> As Alexei replied in a separate sub-thread you probably want this
> for sleepable timers. Here is full source code block:
> 
>         if (insn->imm == BPF_FUNC_timer_set_callback ||
>             is_bpf_timer_set_sleepable_cb_kfunc(insn->imm)) {
>             ...
>             struct bpf_insn ld_addrs[2] = {
>                 BPF_LD_IMM64(BPF_REG_3, (long)prog->aux),
>             };
> 
>             insn_buf[0] = ld_addrs[0];
>             insn_buf[1] = ld_addrs[1];
>             insn_buf[2] = *insn;
>             cnt = 3;
> 
>             new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
>             ...
>         }
> 
> Effectively, it sets up third function call parameter (R3)
> for timer_set_callback() to be prog->aux.
> E.g. before bpf_patch_insn_data():
> 
>    r1 = ... timer ...
>    r2 = ... callback address ...
>    call timer_set_callback
> 
> After bpf_patch_insn_data():
> 
>    r1 = ... timer ...
>    r2 = ... callback address ...
>    r3 = prog->aux ll
>    call timer_set_callback
> 
> This way it won't be necessary to walk stack in search for ctx.aux
> in bpf_timer_set_sleepable_cb().

Hmm, I must still be missing a piece of the puzzle:
if I declare bpf_timer_set_sleepable_cb() to take a third "aux"
argument, given that it is declared as kfunc, I also must declare it in
my bpf program, or I get the following:

# libbpf: extern (func ksym) 'bpf_timer_set_sleepable_cb': func_proto [264] incompatible with vmlinux [18151]

And if I declare it, then I don't know what to pass, given that this is
purely added by the verifier:

43: (85) call bpf_timer_set_sleepable_cb#18152
arg#2 pointer type STRUCT bpf_prog_aux must point to scalar, or struct with scalar

Maybe I should teach the verifier that this kfunc only takes 2
arguments, and the third one is virtual, but that also means that when
the kfunc definitions are to be included in vmlinux.h, they would also
have this special case.

(I just tried with a blank u64 instead of the struct bpf_prog_aux*, but
it crashes with KASAN complaining).

Cheers,
Benjamin