Re: [PATCH v3 bpf-next 11/11] selftest: bpf: Add test for BPF_SOCK_OPS_RCVQ_CB.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The test case incorrectly asserts that `SO_RCVLOWAT` is automatically updated to `RPC_DESC_SIZE` (8) immediately after `recv()` reads the first RPC frame.
- [Low] Multi-line comments do not follow the mandatory BPF subsystem style.
- [Low] Incorrect ASSERT macros used for file descriptor validation.
--

commit ab873db7faa26eaced426077409e35dd60974a4f
Author: Kuniyuki Iwashima <kuniyu@google.com>
Subject: selftest: bpf: Add test for BPF_SOCK_OPS_RCVQ_CB.

This commit introduces a new selftest for the BPF_SOCK_OPS_RCVQ_CB hook.
It simulates an RPC frame exchange over TCP to verify that a BPF program
can dynamically adjust sk_rcvlowat and properly control epoll wakeups.

> diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_autolowat.c b/tools/testing/selftests/bpf/prog_tests/tcp_autolowat.c
> new file mode 100644
> index 0000000000000..5e971c42a32a4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/tcp_autolowat.c

[ ... ]

> +	{
> +		.desc = {
> +			{.header_len = 100, .payload_len = 150},
> +			{.header_len = 200, .payload_len = 500},
> +		},
> +		.event = {
> +			{ .type = RPC_EVENT_AUTOLOWAT,	.val = 1},

[ ... ]

> +			/* skb has the first payload and 1 byte of the next descriptor. */
> +			{.type = RPC_EVENT_SEND,	.len = 150 + 1},
> +			{.type = RPC_EVENT_EPOLL,	.nfds = 1},
> +			{.type = RPC_EVENT_RCVLOWAT,	.rcvlowat = RPC_DESC_SIZE + 150 + 100},
> +			/* After reading the first RPC message, SO_RCVLOWAT should be RPC_DESC_SIZE. */
> +			{.type = RPC_EVENT_RECV,	.len = RPC_DESC_SIZE + 150 + 100},
> +			{.type = RPC_EVENT_EPOLL,	.nfds = 0},
> +			{.type = RPC_EVENT_RCVLOWAT,	.rcvlowat = RPC_DESC_SIZE},

[Severity: Medium]
Does this assertion accurately reflect the kernel's behavior?

The test calls recv() here, which executes in process context. However,
tcp_recvmsg() does not trigger the BPF_SOCK_OPS_RCVQ_CB hook, as that hook
only runs when new SKBs are enqueued from the network stack.

Since no new network packet has arrived yet, the BPF program hasn't run to
update sk->sk_rcvlowat. Could this cause sk->sk_rcvlowat to remain at 258
instead of dropping to 8 (RPC_DESC_SIZE), resulting in a test failure here?

> +			/* The second descriptor is available. */
> +			{.type = RPC_EVENT_SEND,	.len = RPC_DESC_SIZE - 1},
> +			{.type = RPC_EVENT_EPOLL,	.nfds = 0},
> +			{.type = RPC_EVENT_RCVLOWAT,	.rcvlowat = RPC_DESC_SIZE + 200 + 500},
> +		},
> +	},
> +};

[ ... ]

> +static int tcp_autolowat_setup_cb(struct tcp_autolowat_test_cb *cb, int family)
> +{
> +	struct epoll_event ev = {};
> +	int err;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(cb->fd); i++)
> +		cb->fd[i] = -1;
> +
> +	cb->saved_netns = open("/proc/self/ns/net", O_RDONLY);
> +	if (!ASSERT_NEQ(cb->saved_netns, -1, "save netns"))

[Severity: Low]
Is it preferred to use the ASSERT_OK_FD() macro here instead of ASSERT_NEQ()
for file descriptor validation, to better align with modern BPF selftest
patterns?

> +		goto err;
> +
> +	err = unshare(CLONE_NEWNET);
> +	if (!ASSERT_OK(err, "unshare"))
> +		goto err;
> +
> +	err = system("ip link set dev lo up");
> +	if (!ASSERT_OK(err, "set up lo"))
> +		goto err;
> +
> +	cb->server = start_server(family, SOCK_STREAM, NULL, 0, 0);
> +	if (!ASSERT_NEQ(cb->server, -1, "start_server"))

[Severity: Low]
Similarly, could this use ASSERT_OK_FD() as well? This pattern is repeated
for connect_to_fd(), accept(), and epoll_create1() below.

> +		goto err;
> +

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/tcp_autolowat.c b/tools/testing/selftests/bpf/progs/tcp_autolowat.c
> new file mode 100644
> index 0000000000000..41eacfdb34aa1
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/tcp_autolowat.c

[ ... ]

> +	/* Since LLVM commit 324e27e8bad83ca23a3cd276d7e2e729b1b0b8c7,
> +	 * clang omits the "copy_len == 0" check below, which is necessary
> +	 * to satisfy the BPF verifier's range check for bpf_skb_load_bytes().
> +	 */

[Severity: Low]
This isn't a bug, but could we update these multi-line comments to follow
the preferred BPF subsystem style, where the opening /* is placed on its
own line?

> +	barrier_var(copy_len);
> +
> +	/* Do not swap the order of the two copy_len checks below.
> +	 * The BPF verifier somehow does not properly track the minimum
> +	 * value for 'copy_len == 0'.

[Severity: Low]
This isn't a bug, but this comment also starts the text on the same line as
the opening /*.

> +	 *
> +	 * 91: (15) if r7 == 0x0 goto pc+40      ; R7=scalar(smin=smin32=-247,smax=smax32=8)
> +	 * 92: (25) if r7 > 0x8 goto pc+39       ; R7=scalar(smin=smin32=0,smax=umax=smax32=umax32=8,var_off=(0x0; 0xf))
> +	 *
> +	 * This does not occur if copy_len is u32.
> +	 */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260523083001.2911931-1-kuniyu@google.com?part=11