Messages in this thread | | | Subject | Re: [PATCH RFC bpf-next v6 1/6] bpf/helpers: introduce sleepable bpf_timers | From | Eduard Zingerman <> | Date | Mon, 08 Apr 2024 20:07:56 +0300 |
| |
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.
> +} > +
| |