Re: [PATCH net 2/2] selftests/bpf: cover sockmap drop after partial TCP read

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The test `test_sockmap_drop_after_partial_read` relies on asynchronous packet processing without any synchronization, making it inherently flaky.
--

commit 375b8cdbb19df93db8af9c80a28cbeb5b3bdb312
Author: Dong Chenchen <dongchenchen2@huawei.com>

selftests/bpf: cover sockmap drop after partial TCP read

This commit adds a regression test for a TCP socket that is partially read
before it is inserted into a sockmap with an SK_DROP verdict. The test
sends an additional byte to drive the verdict path, removes the socket,
and checks sequence numbers via TCP_ZEROCOPY_RECEIVE.

> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index cb3229711f93a..106dd03cde84e 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
[ ... ]
> +static void test_sockmap_drop_after_partial_read(void)
> +{
[ ... ]
> +	err = bpf_map_update_elem(map, &zero, &p0, BPF_NOEXIST);
> +	if (!ASSERT_OK(err, "bpf_map_update_elem"))
> +		goto end;
> +
> +	sent = xsend(c0, buf, 1, 0);
> +	if (!ASSERT_EQ(sent, 1, "xsend(drop)"))
> +		goto end;
> +
> +	err = bpf_map_delete_elem(map, &zero);

[Severity: Medium]
Will this map deletion race with the asynchronous packet processing?

Because TCP delivery and BPF program execution occur asynchronously in softirq
context, the map deletion might remove the socket from the map before the
queued byte is processed and dropped by the BPF program.

If that happens, the byte could bypass the SK_DROP verdict and be delivered
normally. This would add an unexpected byte to the receive queue, causing the
subsequent TCP_ZEROCOPY_RECEIVE step to copy 2 bytes instead of 1, which fails
the final ASSERT_EQ(zc.copybuf_len, 1, ...) assertion and leads to test
flakiness.

Is there a need for some form of synchronization here before deleting the map
element to ensure the packet is actually processed and dropped?

> +	if (!ASSERT_OK(err, "bpf_map_delete_elem"))
> +		goto end;
> +
> +	sent = xsend(c0, buf, 1, 0);
> +	if (!ASSERT_EQ(sent, 1, "xsend(native again)"))
> +		goto end;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702140959.1806754-1-dongchenchen2@huawei.com?part=2