Re: [PATCH bpf-next v5 1/6] bpf/helpers: introduce sleepable bpf_timers

[Alexei Starovoitov]

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