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

[Mahe Tardy <mahe.tardy@gmail.com>]

On Mon, Jul 06, 2026 at 10:10:40AM +0000, sashiko-bot@kernel.org wrote:
> 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.

Sure, can be removed.

> 
> > +
> > +	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?

Dito.

> 
> > +
> > +	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?

So this is not erasing the content since local.ctx is a kptr, so the
feedback is kinda wrong, it will be ignored by the map_update part. It
was copied from crypto_common.h but is kinda doing nothing so could be
simplified to only lookup & kptr_xchg.

> 
> > +	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