Received-SPF: pass (google.com: domain of linux-kernel+bounces-111806-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: <20240322-hid-bpf-sleepable-v5-0-179c7b59eaaa@kernel.org> <20240322-hid-bpf-sleepable-v5-3-179c7b59eaaa@kernel.org>
In-Reply-To: <20240322-hid-bpf-sleepable-v5-3-179c7b59eaaa@kernel.org>
From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Date: Fri, 22 Mar 2024 17:50:11 +0100
Message-ID: <CAP01T74+naPau2_=1G2_TUSjY_ZCAWQ2XVBKxs9xSKHobZcEeQ@mail.gmail.com>
Subject: Re: [PATCH bpf-next v5 3/6] bpf/helpers: introduce
 bpf_timer_set_sleepable_cb() kfunc
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, 
	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>, John Fastabend <john.fastabend@gmail.com>, 
	KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, 
	Jiri Olsa <jolsa@kernel.org>, Mykola Lysenko <mykolal@fb.com>, Shuah Khan <shuah@kernel.org>, 
	bpf@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Fri, 22 Mar 2024 at 15:57, Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> In this patch, bpf_timer_set_sleepable_cb() is functionally equivalent
> to bpf_timer_set_callback(), to the exception that it enforces
> the timer to be started with BPF_F_TIMER_SLEEPABLE.
>
> But given that bpf_timer_set_callback() is a helper when
> bpf_timer_set_sleepable_cb() is a kfunc, we need to teach the verifier
> about its attached callback.
> Marking that callback as sleepable will be done in a separate patch
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
>
> ---
> [...]
>
> @@ -19548,6 +19582,28 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>                    desc->func_id == special_kfunc_list[KF_bpf_rdonly_cast]) {
>                 insn_buf[0] = BPF_MOV64_REG(BPF_REG_0, BPF_REG_1);
>                 *cnt = 1;
> +       } else if (is_bpf_timer_set_sleepable_cb_impl_kfunc(desc->func_id)) {
> +               /* The verifier will process callback_fn as many times as necessary
> +                * with different maps and the register states prepared by
> +                * set_timer_callback_state will be accurate.
> +                *
> +                * The following use case is valid:
> +                *   map1 is shared by prog1, prog2, prog3.
> +                *   prog1 calls bpf_timer_init for some map1 elements
> +                *   prog2 calls bpf_timer_set_callback for some map1 elements.
> +                *     Those that were not bpf_timer_init-ed will return -EINVAL.
> +                *   prog3 calls bpf_timer_start for some map1 elements.
> +                *     Those that were not both bpf_timer_init-ed and
> +                *     bpf_timer_set_callback-ed will return -EINVAL.
> +                */
> +               struct bpf_insn ld_addrs[2] = {
> +                       BPF_LD_IMM64(BPF_REG_3, (long)env->prog->aux),
> +               };
> +
> +               insn_buf[0] = ld_addrs[0];
> +               insn_buf[1] = ld_addrs[1];
> +               insn_buf[2] = *insn;
> +               *cnt = 3;
>         }

Would be better to handle the fixup of all kfuncs in one place, i.e.
fixup_kfunc_call.

>         return 0;
>  }
>
> --
> 2.44.0
>
>