Received-SPF: pass (google.com: domain of linux-kernel+bounces-68109-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <a72147f5-2b7d-4267-9881-6a645c575838@linux.dev>
Date: Thu, 15 Feb 2024 22:36:16 -0800
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH RFC bpf-next v2 02/10] bpf/helpers: introduce sleepable
 timers
Content-Language: en-US
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-input@vger.kernel.org, linux-doc@vger.kernel.org,
 linux-kselftest@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
 Daniel Borkmann <daniel@iogearbox.net>,
 John Fastabend <john.fastabend@gmail.com>,
 Andrii Nakryiko <andrii@kernel.org>, Eduard Zingerman <eddyz87@gmail.com>,
 Song Liu <song@kernel.org>, Yonghong Song <yonghong.song@linux.dev>,
 KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>,
 Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
 Jiri Kosina <jikos@kernel.org>,
 Benjamin Tissoires <benjamin.tissoires@redhat.com>,
 Jonathan Corbet <corbet@lwn.net>, Shuah Khan <shuah@kernel.org>
References: <20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org>
 <20240214-hid-bpf-sleepable-v2-2-5756b054724d@kernel.org>
From: Martin KaFai Lau <martin.lau@linux.dev>
In-Reply-To: <20240214-hid-bpf-sleepable-v2-2-5756b054724d@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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);
> +}
> +