On 2/14/24 9:18 AM, Benjamin Tissoires wrote:
+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; + void *value = t->value; + bpf_callback_t callback_fn; + void *key; + u32 idx; + + BTF_TYPE_EMIT(struct bpf_timer); + + rcu_read_lock(); + callback_fn = rcu_dereference(t->sleepable_cb_fn); + rcu_read_unlock();
I took a very brief look at patch 2. One thing that may worth to ask here, the rcu_read_unlock() seems to be done too early. It is protecting the t->sleepable_cb_fn (?), so should it be done after finished using the callback_fn?
A high level design question. The intention of the new bpf_timer_set_sleepable_cb() kfunc is actually to delay work to a workqueue. It is useful to delay work from the bpf_timer_cb and it may also useful to delay work from other bpf running context (e.g. the networking hooks like "tc"). The bpf_timer_set_sleepable_cb() seems to be unnecessary forcing delay-work must be done in a bpf_timer_cb.
Have you thought about if it is possible to create a more generic kfunc like bpf_schedule_work() to delay work to a workqueue ?
+ if (!callback_fn) + return; + + /* FIXME: do we need any locking? */ + 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); + } + + /* FIXME: this crashes the system with + * BUG: kernel NULL pointer dereference, address: 000000000000000b + */ + /* callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0); */ + /* The verifier checked that return value is zero. */ +} +
[ ... ]
+/* FIXME: use kernel doc style */ +/* Description + * Configure the timer to call *callback_fn* static function in a + * sleepable context. + * Return + * 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 timers + * 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. + */ +__bpf_kfunc int bpf_timer_set_sleepable_cb(struct bpf_timer_kern *timer, + int (callback_fn)(void *map, int *key, struct bpf_timer *timer)) +{ + struct bpf_throw_ctx ctx = {}; + + /* FIXME: definietely not sure this is OK */ + arch_bpf_stack_walk(bpf_stack_walker, &ctx); + WARN_ON_ONCE(!ctx.aux); + + if (!ctx.aux) + return -EINVAL; + + return __bpf_timer_set_callback(timer, (void *)callback_fn, ctx.aux, true); +} +