Re: [PATCH bpf-next 1/2] selftests/bpf: Cover concurrent sockmap update and delete

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] selftests/bpf: non-standard multi-line comments in sockmap_listen.c
- [Low] selftests/bpf: deprecated CHECK_FAIL wrappers used in test_update_delete_with_prog()
--

commit c179b4e18de0bb4b1070c566c62402a2a9a2d3f1
Author: Jakub Sitnicki <jakub@cloudflare.com>

selftests/bpf: Cover concurrent sockmap update and delete

This commit adds a sockmap_listen subtest that concurrently inserts and
deletes a TCP socket in a sockmap with an attached verdict program to
exercise races between insert and delete paths under KCSAN.

> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> index 235e1d56f044f..dca1f0de92e0e 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> @@ -646,6 +646,82 @@ static void test_race_insert_listen(struct test_sockmap_listen *skel __always_un
>  	xclose(s);
>  }
>  
> +struct update_delete_ctx {
> +	unsigned int nr_iter;
> +	unsigned int done;
> +	int mapfd;
> +};
> +
> +static void *delete_elem_thread(void *arg)
> +{
> +	struct update_delete_ctx *ctx = arg;
> +	unsigned int i;
> +
> +	for (i = 0; i < ctx->nr_iter; i++) {
> +		/* Expecting EINVAL (sockmap) or ENOENT (sockhash) when the
> +		 * entry is already gone.
> +		 */
> +		if (bpf_map_delete_elem(ctx->mapfd, &(u32){0}) &&
> +		    errno != EINVAL && errno != ENOENT) {
> +			FAIL_ERRNO("map_delete");
> +			break;
> +		}
> +	}
> +
> +	WRITE_ONCE(ctx->done, 1);
> +	return NULL;
> +}
> +
> +/* One thread keeps inserting a socket into a sockmap with an attached
> + * verdict program while another thread deletes the same map entry.
> + */
> +static void test_update_delete_with_prog(struct test_sockmap_listen *skel,
> +					 int family, int sotype, int mapfd)
> +{
> +	struct update_delete_ctx ctx = { 0 };
> +	int err, s, verdict;
> +	pthread_t t;
> +	u64 value;
> +
> +	verdict = bpf_program__fd(skel->progs.prog_skb_verdict);
> +	err = xbpf_prog_attach(verdict, mapfd, BPF_SK_SKB_VERDICT, 0);
> +	if (err)
> +		return;
> +
> +	s = xsocket(family, sotype, 0);
> +	if (s < 0)
> +		goto detach;
> +
> +	err = xlisten(s, 1);
> +	if (err)
> +		goto close;
> +
> +	ctx.mapfd = mapfd;
> +	ctx.nr_iter = 10000;
> +
> +	err = xpthread_create(&t, NULL, delete_elem_thread, &ctx);
> +	if (err)
> +		goto close;
> +
> +	value = s;
> +	while (!READ_ONCE(ctx.done)) {
> +		/* Expecting EBUSY while a deleted socket's psock still
> +		 * holds the verdict program.
> +		 */

[Severity: Low]
This isn't a bug, but should the three new multi-line comments in
delete_elem_thread() and test_update_delete_with_prog() in
tools/testing/selftests/bpf/prog_tests/sockmap_listen.c place the opening
/* on its own line with the comment text starting on the following line to
match BPF coding style conventions?

> +		err = bpf_map_update_elem(mapfd, &(u32){0}, &value, BPF_ANY);
> +		if (err && errno != EBUSY) {
> +			FAIL_ERRNO("map_update");
> +			break;
> +		}

[Severity: Low]
In tools/testing/selftests/bpf/prog_tests/socket_helpers.h, FAIL_ERRNO()
and the x* helper wrappers (such as xbpf_prog_attach(), xsocket(),
xlisten(), xpthread_create(), xpthread_join(), xclose(), and
xbpf_prog_detach2()) report failures through _FAIL(), which expands to the
deprecated CHECK_FAIL() macro:

#define _FAIL(errnum, fmt...)                                                  \
	({                                                                     \
		error_at_line(0, (errnum), __func__, __LINE__, fmt);           \
		CHECK_FAIL(true);                                              \
	})

Could delete_elem_thread() and test_update_delete_with_prog() in
tools/testing/selftests/bpf/prog_tests/sockmap_listen.c use the ASSERT_*()
macros from test_progs.h (such as ASSERT_OK(), ASSERT_OK_FD(), and
ASSERT_TRUE() or ASSERT_EQ()) instead of FAIL_ERRNO() and the CHECK_FAIL()
wrappers when checking setup and map update/delete results?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-bpf-next-v1-0-035eabf8eae7@cloudflare.com?part=1