On Fri, Mar 22, 2024 at 7:56 AM Benjamin Tissoires <bentiss@xxxxxxxxxx> wrote:
>
> They are implemented as a workqueue, which means that there are no
> guarantees of timing nor ordering.
>
> Signed-off-by: Benjamin Tissoires <bentiss@xxxxxxxxxx>
>
> ---
>
> no changes in v5
>
> changes in v4:
> - dropped __bpf_timer_compute_key()
> - use a spin_lock instead of a semaphore
> - ensure bpf_timer_cancel_and_free is not complaining about
> non sleepable context and use cancel_work() instead of
> cancel_work_sync()
> - return -EINVAL if a delay is given to bpf_timer_start() with
> BPF_F_TIMER_SLEEPABLE
>
> changes in v3:
> - extracted the implementation in bpf_timer only, without
> bpf_timer_set_sleepable_cb()
> - rely on schedule_work() only, from bpf_timer_start()
> - add semaphore to ensure bpf_timer_work_cb() is accessing
> consistent data
>
> changes in v2 (compared to the one attaches to v1 0/9):
> - make use of a kfunc
> - add a (non-used) BPF_F_TIMER_SLEEPABLE
> - the callback is *not* called, it makes the kernel crashes
> ---
> include/uapi/linux/bpf.h | 4 +++
> kernel/bpf/helpers.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 3c42b9f1bada..b90def29d796 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7461,10 +7461,14 @@ struct bpf_core_relo {
> * - BPF_F_TIMER_ABS: Timeout passed is absolute time, by default it is
> * relative to current time.
> * - BPF_F_TIMER_CPU_PIN: Timer will be pinned to the CPU of the caller.
> + * - BPF_F_TIMER_SLEEPABLE: Timer will run in a sleepable context, with
> + * no guarantees of ordering nor timing (consider this as being just
> + * offloaded immediately).
> */
> enum {
> BPF_F_TIMER_ABS = (1ULL << 0),
> BPF_F_TIMER_CPU_PIN = (1ULL << 1),
> + BPF_F_TIMER_SLEEPABLE = (1ULL << 2),
> };
>
> /* BPF numbers iterator state */
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index a89587859571..38de73a9df83 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1094,14 +1094,20 @@ const struct bpf_func_proto bpf_snprintf_proto = {
> * bpf_timer_cancel() cancels the timer and decrements prog's refcnt.
> * Inner maps can contain bpf timers as well. ops->map_release_uref is
> * freeing the timers when inner map is replaced or deleted by user space.
> + *
> + * sleepable_lock protects only the setup of the workqueue, not the callback
> + * itself. This is done to ensure we don't run concurrently a free of the
> + * callback or the associated program.
I recall there was a discussion about this lock earlier,
but I don't remember what the conclusion was.
The above comment is not enough to understand what it protects.
In general how sleepable cb is fundamentally different
from non-sleepable one when it comes to races ?
bpf_timer_set_callback() is racy for both sleepable and non-sleepable
and the latter handles it fine.
Note that struct bpf_hrtimer is rcu protected.
See kfree_rcu(t, rcu); in bpf_timer_cancel_and_free().
> */
> struct bpf_hrtimer {
> 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;
> + spinlock_t sleepable_lock;
> };
>
> /* the actual struct hidden inside uapi struct bpf_timer */
> @@ -1114,6 +1120,49 @@ struct bpf_timer_kern {
> struct bpf_spin_lock lock;
> } __attribute__((aligned(8)));
>
> +static void bpf_timer_work_cb(struct work_struct *work)
> +{
> + struct bpf_hrtimer *t = container_of(work, struct bpf_hrtimer, work);
> + struct bpf_map *map = t->map;
> + bpf_callback_t callback_fn;
> + void *value = t->value;
> + unsigned long flags;
> + void *key;
> + u32 idx;
> +
> + BTF_TYPE_EMIT(struct bpf_timer);
> +
> + spin_lock_irqsave(&t->sleepable_lock, flags);
> +
> + callback_fn = READ_ONCE(t->callback_fn);
> + if (!callback_fn) {
> + spin_unlock_irqrestore(&t->sleepable_lock, flags);
> + return;
> + }
> +
> + if (map->map_type == BPF_MAP_TYPE_ARRAY) {
> + struct bpf_array *array = container_of(map, struct bpf_array, map);
> +
> + /* compute the key */
> + idx = ((char *)value - array->value) / array->elem_size;
> + key = &idx;
> + } else { /* hash or lru */
> + key = value - round_up(map->key_size, 8);
> + }
> +
> + /* prevent the callback to be freed by bpf_timer_cancel() while running
> + * so we can release the sleepable lock
> + */
> + bpf_prog_inc(t->prog);
> +
> + spin_unlock_irqrestore(&t->sleepable_lock, flags);
why prog_inc ?
The sleepable progs need rcu_read_lock_trace() + migrate_disable()
anyway, which are missing here.
Probably best to call __bpf_prog_enter_sleepable_recur()
like kern_sys_bpf() does.
Now with that, the bpf_timer_cancel() can drop prog refcnt to zero
and it's ok, since rcu_read_lock_trace() will protect it.
> +
> + callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0);
> + /* The verifier checked that return value is zero. */
the prog will finish and will be freed after rcu_read_unlock_trace().
Seems fine to me. No need for inc/dec refcnt.
> +
> + bpf_prog_put(t->prog);
> +}
> +
> static DEFINE_PER_CPU(struct bpf_hrtimer *, hrtimer_running);
>
> static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer)
> @@ -1192,6 +1241,8 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map
> t->prog = NULL;
> rcu_assign_pointer(t->callback_fn, NULL);
> hrtimer_init(&t->timer, clockid, HRTIMER_MODE_REL_SOFT);
> + INIT_WORK(&t->work, bpf_timer_work_cb);
> + spin_lock_init(&t->sleepable_lock);
> t->timer.function = bpf_timer_cb;
> WRITE_ONCE(timer->timer, t);
> /* Guarantee the order between timer->timer and map->usercnt. So
> @@ -1237,6 +1288,7 @@ BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callb
> ret = -EINVAL;
> goto out;
> }
> + spin_lock(&t->sleepable_lock);
> if (!atomic64_read(&t->map->usercnt)) {
> /* maps with timers must be either held by user space
> * or pinned in bpffs. Otherwise timer might still be
> @@ -1263,6 +1315,8 @@ BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callb
> }
> rcu_assign_pointer(t->callback_fn, callback_fn);
> out:
> + if (t)
> + spin_unlock(&t->sleepable_lock);
> __bpf_spin_unlock_irqrestore(&timer->lock);
If lock is really needed why timer->lock cannot be reused?
The pattern of two locks in pretty much the same data structure
is begging for questions about what is going on here.
> return ret;
> }
> @@ -1283,8 +1337,12 @@ BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, fla
>
> if (in_nmi())
> return -EOPNOTSUPP;
> - if (flags & ~(BPF_F_TIMER_ABS | BPF_F_TIMER_CPU_PIN))
> + if (flags & ~(BPF_F_TIMER_ABS | BPF_F_TIMER_CPU_PIN | BPF_F_TIMER_SLEEPABLE))
> return -EINVAL;
> +
> + if ((flags & BPF_F_TIMER_SLEEPABLE) && nsecs)
> + return -EINVAL;
> +
> __bpf_spin_lock_irqsave(&timer->lock);
> t = timer->timer;
> if (!t || !t->prog) {
> @@ -1300,7 +1358,10 @@ BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, fla
> if (flags & BPF_F_TIMER_CPU_PIN)
> mode |= HRTIMER_MODE_PINNED;
>
> - hrtimer_start(&t->timer, ns_to_ktime(nsecs), mode);
> + if (flags & BPF_F_TIMER_SLEEPABLE)
> + schedule_work(&t->work);
> + else
> + hrtimer_start(&t->timer, ns_to_ktime(nsecs), mode);
> out:
> __bpf_spin_unlock_irqrestore(&timer->lock);
> return ret;
> @@ -1348,13 +1409,22 @@ BPF_CALL_1(bpf_timer_cancel, struct bpf_timer_kern *, timer)
> ret = -EDEADLK;
> goto out;
> }
> + spin_lock(&t->sleepable_lock);
> drop_prog_refcnt(t);
> + spin_unlock(&t->sleepable_lock);
this also looks odd.
> out:
> __bpf_spin_unlock_irqrestore(&timer->lock);
> /* Cancel the timer and wait for associated callback to finish
> * if it was running.
> */
> ret = ret ?: hrtimer_cancel(&t->timer);
> +
> + /* also cancel the sleepable work, but *do not* wait for
> + * it to finish if it was running as we might not be in a
> + * sleepable context
> + */
> + ret = ret ?: cancel_work(&t->work);
> +
> rcu_read_unlock();
> return ret;
> }
> @@ -1383,11 +1453,13 @@ void bpf_timer_cancel_and_free(void *val)
> t = timer->timer;
> if (!t)
> goto out;
> + spin_lock(&t->sleepable_lock);
> drop_prog_refcnt(t);
> /* The subsequent bpf_timer_start/cancel() helpers won't be able to use
> * this timer, since it won't be initialized.
> */
> WRITE_ONCE(timer->timer, NULL);
> + spin_unlock(&t->sleepable_lock);
This one I don't understand either.
pw-bot: cr
