linux-kernel - Re: [PATCH RFC bpf-next v6 1/6] bpf/helpers: introduce sleepable bpf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 8 Apr 2024 19:20:26 +0200
From: Benjamin Tissoires <benjamin.tissoires@...hat.com>
To: Eduard Zingerman <eddyz87@...il.com>
Cc: Benjamin Tissoires <bentiss@...nel.org>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>, 
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH RFC bpf-next v6 1/6] bpf/helpers: introduce sleepable bpf_timers

On Mon, Apr 8, 2024 at 7:08 PM Eduard Zingerman <eddyz87@...il.com> wrote:
>
> On Mon, 2024-04-08 at 10:09 +0200, Benjamin Tissoires wrote:
>
> [...]
>
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 9234174ccb21..fd05d4358b31 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -1096,12 +1096,19 @@ const struct bpf_func_proto bpf_snprintf_proto = {
> >   * freeing the timers when inner map is replaced or deleted by user space.
> >   */
> >  struct bpf_hrtimer {
> > -     struct hrtimer timer;
> > +     union {
> > +             struct hrtimer timer;
> > +             struct work_struct work;
> > +     };
> >       struct bpf_map *map;
> >       struct bpf_prog *prog;
> >       void __rcu *callback_fn;
> >       void *value;
> > -     struct rcu_head rcu;
> > +     union {
> > +             struct rcu_head rcu;
> > +             struct work_struct sync_work;
>
> Nit:
> I find this name very confusing, the field is used to cancel timer
> execution, is it a convention to call such things '...sync...'?
>
> > +     };
> > +     u64 flags;
> >  };
> >
>
> [...]
>
> > +static void bpf_timer_sync_work_cb(struct work_struct *work)
> > +{
> > +     struct bpf_hrtimer *t = container_of(work, struct bpf_hrtimer, sync_work);
> > +
> > +     cancel_work_sync(&t->work);
> > +
> > +     kfree_rcu(t, rcu);
>
> Sorry, I might be wrong, but this looks suspicious.
> The 'rcu' field of 'bpf_hrtimer' is defined as follows:
>
> struct bpf_hrtimer {
>         ...
>         union {
>                 struct rcu_head rcu;
>                 struct work_struct sync_work;
>         };
>         ...
> };
>
> And for sleepable timers the 'sync_work' field is set as follows:
>
> BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map,
>            u64, flags)
> {
>         ...
>         INIT_WORK(&t->sync_work, bpf_timer_sync_work_cb);
>         ...
> }
>
> So, it looks like 'kfree_rcu' would be called for a non-rcu pointer.

That was my initial assumption too, but Alexei told me it was fine.
And I think he is correct because kfree_rcu doesn't need the rcu_head
to be initialized.

So in the end, we initialize the memory as a work_struct, and when
that work kicks in, we reuse that exact same memory as the rcu_head.
This is fine because that work will never be reused.

If I understand correctly, this is to save a few bytes as this is a
critical struct used in programs with a high rate usage, and every
byte counts.

Cheers,
Benjamin

>
> > +}
> > +
>
>