Re: [PATCH net 5/7] selftests: bpf: cover wrapped sk_msg ring chaining in ktls TX path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Test fragility due to hardcoded fragment count allows silent false positive passes
- [Medium] Flawed splice() return value validation masks setup failures
- [Low] Usage of ASSERT_TRUE() for pointer validation instead of ASSERT_OK_PTR()
--

commit ff8ec72cdd60f8b61132bc64e2221ed120c71020
Author: Jakub Kicinski <kuba@kernel.org>

selftests: bpf: cover wrapped sk_msg ring chaining in ktls TX path

This commit adds a regression test for an off-by-one error in
tls_push_record(). It uses a BPF program to drop bytes and splices
data to force an sk_msg scatterlist ring to wrap and trigger the bug.

> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c

[ ... ]

> +static void test_sockmap_ktls_tx_wrapped_chain(int family, int sotype)
> +{
> +	int c = -1, p = -1, one = 1, prog_fd, map_fd;
> +	int pipefd[2] = { -1, -1 };
> +	struct test_sockmap_ktls *skel;
> +	char byte;
> +	ssize_t n;
> +	int err, i;
> +
> +	skel = test_sockmap_ktls__open_and_load();
> +	if (!ASSERT_TRUE(skel, "open ktls skel"))

This isn't a bug, but should this use ASSERT_OK_PTR() instead of
ASSERT_TRUE() for validating the skeleton pointer? ASSERT_OK_PTR()
is specifically designated for checking pointers in BPF selftests
and properly handles both NULL and ERR_PTR values.

> +		return;

[ ... ]

> +	/* packetized pipe so each splice frag becomes its own sg entry */
> +	err = pipe2(pipefd, O_DIRECT);
> +	if (!ASSERT_OK(err, "pipe2"))
> +		goto out;
> +	err = fcntl(pipefd[0], F_SETPIPE_SZ, 17 * 4096);
> +	if (!ASSERT_GE(err, 17 * 4096, "F_SETPIPE_SZ"))
> +		goto out;
> +
> +	for (i = 0; i < 17; i++) {
> +		byte = 'A' + i;
> +		if (!ASSERT_EQ(write(pipefd[1], &byte, 1), 1, "write to pipe"))
> +			goto out;
> +	}

Will this silently fail to wrap the ring if the kernel is configured
with a CONFIG_MAX_SKB_FRAGS value greater than 17?

The test relies on 17 fragments to fill the ring, as the default capacity
for NR_MSG_FRAG_IDS is MAX_SKB_FRAGS + 1. However, MAX_SKB_FRAGS can be
configured up to 45. If the capacity exceeds 17, the 18th byte sent later
in the test will just be placed at index 17, so sg.end will never wrap to 0.

Would it be more robust to dynamically determine the fragment limit by
reading /proc/sys/net/core/max_skb_frags instead of hardcoding 17?

> +
> +	/* drop the first 16 bytes so sg.start advances to 16 */
> +	skel->bss->apply_bytes = 16;
> +
> +	n = splice(pipefd[0], NULL, c, NULL, 17, 0);
> +	if (n < 0)
> +		ASSERT_EQ(errno, EACCES, "splice errno");

Could this splice check mask potential test setup failures?

If the BPF program drops 16 bytes as expected, tls_sw_sendmsg() will
return the remaining 1 byte, making splice() return 1. The n < 0 check
evaluates to false and is bypassed.

However, if the BPF program fails to drop the bytes due to a setup issue
or future regression, splice() will return 17. The n < 0 check is still
bypassed, but the full sk_msg ring will be flushed. The subsequent
1-byte send() will then start a new ring at sg.end = 1 instead of
wrapping the existing one.

Should this strictly assert the exact expected return value, for example
using ASSERT_EQ(n, 1, "splice returns remaining byte"), to guarantee
the ring state is set up correctly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429222944.2139041-1-kuba@kernel.org?part=5