Re: [PATCH bpf-next v2] bpf: allow ksock connect/send calls from wq

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: u64 map key causes workqueue double-init on big-endian
--

commit 5eb6c40a460d2cca2d0dee2b208a0164f311d369
Author: Mahe Tardy <mahe.tardy@gmail.com>

bpf: allow ksock connect/send calls from wq

This commit relaxes the constraint preventing the use of ksock kfuncs from
asynchronous workqueues. It also updates the ksock_wq selftest to verify
that connect() and send() function properly when called from workqueues.

> diff --git a/tools/testing/selftests/bpf/progs/ksock_wq.c b/tools/testing/selftests/bpf/progs/ksock_wq.c
> index 16a1873d132ed..290736e219e1d 100644
> --- a/tools/testing/selftests/bpf/progs/ksock_wq.c
> +++ b/tools/testing/selftests/bpf/progs/ksock_wq.c
[ ... ]
> @@ -43,20 +73,40 @@ static int ksock_wq_callback(void *map, int *key, void *value)
>  SEC("syscall")
>  int ksock_wq_start(void *ctx)
>  {
> +	struct bpf_ksock_create_opts opts = {
> +		.family = AF_INET,
> +		.type = SOCK_DGRAM,
> +		.protocol = IPPROTO_UDP,
> +	};
>  	struct ksock_wq_value *value;
> -	u32 key = 0;
> -	int err;
> +	struct bpf_ksock *ks;
> +	u64 key = 0;
> +	int err = 0;
>  
> -	value = bpf_map_lookup_elem(&work_map, &key);
> -	if (!value)
> -		return -ENOENT;
> -	err = bpf_wq_init(&value->work, &work_map, 0);
> -	if (err)
> +	ks = bpf_ksock_create(&opts, sizeof(opts), &err);
> +	if (!ks)
>  		return err;
> -	err = bpf_wq_set_callback(&value->work, ksock_wq_callback, 0);
> +	err = ksock_ctx_insert(ks);
>  	if (err)
>  		return err;
> -	return bpf_wq_start(&value->work, 0);
> +	for (key = 0; key < 2; key++) {
> +		value = bpf_map_lookup_elem(&work_map, &key);

[Severity: Medium]
Can this cause a workqueue double-init on big-endian architectures?

The work_map map is defined with a u32 key, but the key variable here was
changed to u64. On big-endian systems, passing &key to bpf_map_lookup_elem()
will read the most significant 32 bits, which will evaluate to 0 for both
key = 0 and key = 1.

Because of this, the loop would fetch the map element for index 0 twice.
During the second iteration, bpf_wq_init() would be called on value->work
while it may already be actively queued from the first iteration. Does
re-initializing an active work_struct without serialization run the risk
of corrupting the global workqueue list and crashing?

> +		if (!value)
> +			return -ENOENT;
> +		err = bpf_wq_init(&value->work, &work_map, 0);
> +		if (err)
> +			return err;
> +		if (key)
> +			err = bpf_wq_set_callback(&value->work, ksock_wq_create, 0);
> +		else
> +			err = bpf_wq_set_callback(&value->work, ksock_wq_connect_send, 0);
> +		if (err)
> +			return err;
> +		err = bpf_wq_start(&value->work, 0);
> +		if (err)
> +			return err;
> +	}
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013182726.276372-1-mahe.tardy@gmail.com?part=1