Re: [PATCH bpf-next 3/6] selftests/bpf: Add ksock kfunc test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Dead code and flawed insert logic in `ksock_ctx_insert()` due to unconditional map element overwriting.
- [Low] Unnecessary manual validation of BPF program FDs after successful skeleton load.
--

commit 559d17f26b2fc7936f58b27cbc4a4f83dacd3a73
Author: Mahe Tardy <mahe.tardy@gmail.com>

selftests/bpf: Add ksock kfunc test
    
Add a selftest that exercises the ksock kfuncs end-to-end. One sycall
bpf setup program creates a ksock context and connect the socket.
Another syscall bpf program lookup the context and send test data.

> diff --git a/tools/testing/selftests/bpf/prog_tests/ksock.c b/tools/testing/selftests/bpf/prog_tests/ksock.c
> new file mode 100644
> index 0000000000000..085ddb59067e8
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/ksock.c

[ ... ]

> +static bool ksock_setup_ctx(struct ksock_basic *skel)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, opts);
> +	int err, pfd;
> +
> +	skel->bss->ipv4_remote = inet_addr(LOOPBACK_IP);
> +	skel->bss->remote_port = RECV_PORT;
> +
> +	pfd = bpf_program__fd(skel->progs.ksock_setup);
> +	if (!ASSERT_OK_FD(pfd, "ksock_setup fd"))
> +		return false;

[Severity: Low]
This isn't a bug, but is it necessary to manually validate the program
file descriptor here?

According to the BPF subsystem selftest guidelines, after a successful
skeleton load, skeleton-generated program and map FDs are guaranteed to be
valid. Since this is called after a successful skeleton load, the
ASSERT_OK_FD check might be redundant.

> +
> +	err = bpf_prog_test_run_opts(pfd, &opts);
> +	if (!ASSERT_OK(err, "ksock_setup run"))
> +		return false;

[ ... ]

> +void test_ksock_basic(void)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, opts);
> +	struct ksock_test_env env;
> +	struct ksock_basic *skel;
> +	int err, pfd;
> +
> +	skel = ksock_basic__open();
> +	if (!ASSERT_OK_PTR(skel, "skel open"))
> +		return;
> +
> +	err = ksock_basic__load(skel);
> +	if (!ASSERT_OK(err, "skel load")) {
> +		ksock_basic__destroy(skel);
> +		return;
> +	}
> +
> +	if (!ksock_test_env_setup(&env, NS_TEST))
> +		goto fail;
> +
> +	/* Step 1: Run the setup SYSCALL prog to create ksock */
> +	if (!ksock_setup_ctx(skel))
> +		goto fail;
> +
> +	/* Step 2: Run the send SYSCALL prog */
> +	pfd = bpf_program__fd(skel->progs.ksock_send);
> +	if (!ASSERT_OK_FD(pfd, "ksock_send fd"))
> +		goto fail;

[Severity: Low]
This isn't a bug, but similarly to the check in ksock_setup_ctx, is this
ASSERT_OK_FD necessary since the skeleton is already successfully loaded?

> +
> +	err = bpf_prog_test_run_opts(pfd, &opts);
> +	if (!ASSERT_OK(err, "ksock_send run"))
> +		goto fail;

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/ksock_common.h b/tools/testing/selftests/bpf/progs/ksock_common.h
> new file mode 100644
> index 0000000000000..87d92372d28dc
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/ksock_common.h

[ ... ]

> +static inline int ksock_ctx_insert(struct bpf_ksock *ctx)
> +{
> +	struct __ksock_ctx_value local, *v;
> +	struct bpf_ksock *old;
> +	u32 key = 0;
> +	int err;
> +
> +	local.ctx = NULL;
> +	err = bpf_map_update_elem(&__ksock_ctx_map, &key, &local, 0);

[Severity: Medium]
Will this unconditional bpf_map_update_elem call defeat the intended
collision detection logic below?

Because __ksock_ctx_map is a BPF_MAP_TYPE_ARRAY map, calling
bpf_map_update_elem with a zero-initialized value (local.ctx = NULL)
will clear the existing entry and implicitly free any pre-existing kptr.

Consequently, the subsequent bpf_kptr_xchg will always see a NULL pointer,
making the if (old) check dead code and causing the function to silently
overwrite existing entries instead of returning -EEXIST.

Could the logic be adjusted to perform the lookup and exchange without
overwriting the element first?

> +	if (err) {
> +		bpf_ksock_release(ctx);
> +		return err;
> +	}
> +
> +	v = bpf_map_lookup_elem(&__ksock_ctx_map, &key);
> +	if (!v) {
> +		bpf_ksock_release(ctx);
> +		return -ENOENT;
> +	}
> +
> +	old = bpf_kptr_xchg(&v->ctx, ctx);
> +	if (old) {
> +		bpf_ksock_release(old);
> +		return -EEXIST;
> +	}
> +
> +	return 0;
> +}

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