Received-SPF: pass (google.com: domain of linux-kernel+bounces-77341-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: <20240221-hid-bpf-sleepable-v3-0-1fb378ca6301@kernel.org> <20240221-hid-bpf-sleepable-v3-6-1fb378ca6301@kernel.org>
In-Reply-To: <20240221-hid-bpf-sleepable-v3-6-1fb378ca6301@kernel.org>
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Thu, 22 Feb 2024 12:53:15 -0800
Message-ID: <CAADnVQK_2qhcZD0Wtd=8Bb8NVwBCg5Q+j8bF0HY8Hm91ACNrNA@mail.gmail.com>
Subject: Re: [PATCH RFC bpf-next v3 06/16] 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>, 
	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>, 
	Jiri Kosina <jikos@kernel.org>, Benjamin Tissoires <benjamin.tissoires@redhat.com>, 
	Jonathan Corbet <corbet@lwn.net>, Shuah Khan <shuah@kernel.org>, bpf <bpf@vger.kernel.org>, 
	LKML <linux-kernel@vger.kernel.org>, 
	"open list:HID CORE LAYER" <linux-input@vger.kernel.org>, 
	"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>, 
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 21, 2024 at 8:25=E2=80=AFAM 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
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
>
> ---
>
> new in v3 (split from v2 02/10)
> ---
>  kernel/bpf/helpers.c  | 48 +++++++++++++++++++++++++++++++++++++++++++++=
+--
>  kernel/bpf/verifier.c | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 75 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index f9add0abe40a..2c6dc3d0ffff 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1108,6 +1108,7 @@ struct bpf_hrtimer {
>         void __rcu *callback_fn;
>         void *value;
>         struct semaphore sleepable_lock;
> +       bool is_sleepable;
>  };
>
>  /* the actual struct hidden inside uapi struct bpf_timer */
> @@ -1270,8 +1271,8 @@ static const struct bpf_func_proto bpf_timer_init_p=
roto =3D {
>         .arg3_type      =3D ARG_ANYTHING,
>  };
>
> -BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void =
*, callback_fn,
> -          struct bpf_prog_aux *, aux)
> +static int __bpf_timer_set_callback(struct bpf_timer_kern *timer, void *=
callback_fn,
> +                                   struct bpf_prog_aux *aux, bool is_sle=
epable)
>  {
>         struct bpf_prog *prev, *prog =3D aux->prog;
>         struct bpf_hrtimer *t;
> @@ -1311,12 +1312,19 @@ BPF_CALL_3(bpf_timer_set_callback, struct bpf_tim=
er_kern *, timer, void *, callb
>                 t->prog =3D prog;
>         }
>         rcu_assign_pointer(t->callback_fn, callback_fn);
> +       t->is_sleepable =3D is_sleepable;
>         up(&t->sleepable_lock);
>  out:
>         __bpf_spin_unlock_irqrestore(&timer->lock);
>         return ret;
>  }
>
> +BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void =
*, callback_fn,
> +          struct bpf_prog_aux *, aux)
> +{
> +       return __bpf_timer_set_callback(timer, callback_fn, aux, false);
> +}
> +
>  static const struct bpf_func_proto bpf_timer_set_callback_proto =3D {
>         .func           =3D bpf_timer_set_callback,
>         .gpl_only       =3D true,
> @@ -1342,6 +1350,11 @@ BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern =
*, timer, u64, nsecs, u64, fla
>                 goto out;
>         }
>
> +       if (t->is_sleepable && !(flags & BPF_F_TIMER_SLEEPABLE)) {
> +               ret =3D -EINVAL;
> +               goto out;
> +       }
> +
>         if (flags & BPF_F_TIMER_ABS)
>                 mode =3D HRTIMER_MODE_ABS_SOFT;
>         else
> @@ -2606,6 +2619,36 @@ __bpf_kfunc void bpf_throw(u64 cookie)
>         WARN(1, "A call to BPF exception callback should never return\n")=
;
>  }
>
> +/**
> + * bpf_timer_set_sleepable_cb() - Configure the timer to call %callback_=
fn
> + * static function in a sleepable context.
> + * @timer: The bpf_timer that needs to be configured
> + * @callback_fn: a static bpf function
> + *
> + * @returns %0 on success. %-EINVAL if %timer was not initialized with
> + * bpf_timer_init() earlier. %-EPERM if %timer is in a map that doesn't
> + * have any user references.
> + * The user space should either hold a file descriptor to a map with tim=
ers
> + * or pin such map in bpffs. When map is unpinned or file descriptor is
> + * closed all timers in the map will be cancelled and freed.
> + *
> + * This kfunc is equivalent to %bpf_timer_set_callback except that it te=
lls
> + * the verifier that the target callback is run in a sleepable context.
> + */
> +__bpf_kfunc int bpf_timer_set_sleepable_cb(struct bpf_timer_kern *timer,
> +                                          int (callback_fn)(void *map, i=
nt *key, struct bpf_timer *timer))
> +{
> +       struct bpf_throw_ctx ctx =3D {};
> +
> +       arch_bpf_stack_walk(bpf_stack_walker, &ctx);
> +       WARN_ON_ONCE(!ctx.aux);

Sorry. Why such complexity?
Please see how do_misc_fixups() handles BPF_FUNC_timer_set_callback.