Re: [PATCH v1 bpf 2/2] selftest: bpf: Add test for bpf_tcp_sock() and RAW socket.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The selftest logic is inverted: it will fail on a correctly fixed kernel and pass on a buggy kernel (if KASAN is disabled).
--

commit ba32f094566f3d6a513a47837fc65f3b355951f9
Author: Kuniyuki Iwashima <kuniyu@google.com>

selftest: bpf: Add test for bpf_tcp_sock() and RAW socket.

This patch extends sockopt_sk.c to test the behavior of bpf_tcp_sock()
when called on a socket with the wrong type (SOCK_RAW).

> diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c b/tools/testing/selftests/bpf/progs/sockopt_sk.c
> index cb990a7d3d458..5e0b27e7855cb 100644
> --- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
> +++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c

[ ... ]

> @@ -149,6 +149,20 @@ int _setsockopt(struct bpf_sockopt *ctx)
>  	if (sk && sk->family == AF_NETLINK)
>  		goto out;
>  
> +	if (sk && sk->family == AF_INET && sk->type == SOCK_RAW) {
> +		struct bpf_tcp_sock *tp = bpf_tcp_sock(sk);
> +
> +		if (tp) {
> +			char saved_syn[60];
> +
> +			bpf_getsockopt(sk, SOL_TCP, TCP_SAVED_SYN,
> +				       &saved_syn, sizeof(saved_syn));
> +			goto consumed;
> +		}
> +
> +		goto out;
> +	}

On a fixed kernel where bpf_tcp_sock() correctly returns NULL for a SOCK_RAW
socket, won't tp evaluate to false? This causes the program to jump to the
out label instead of the consumed label. 

When jumping to out, ctx->optlen is left unmodified, which tells the kernel
to proceed with the native setsockopt handler instead of bypassing it.

> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> index 53637431ec5de..87e771c8991fc 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c

[ ... ]

> @@ -211,6 +211,21 @@ static int getsetsockopt(void)
>  	}
>  	ASSERT_EQ(optlen, 8, "Unexpected NETLINK_LIST_MEMBERSHIPS value");
>  
> +	/* Trick bpf_tcp_sock() with IPPROTO_TCP */
> +	close(fd);
> +	fd = socket(AF_INET, SOCK_RAW, IPPROTO_TCP);
> +	if (fd < 0) {
> +		log_err("Failed to create RAW socket");
> +		goto err;
> +	}
> +
> +	optlen = 60;
> +	err = setsockopt(fd, SOL_TCP, TCP_SAVED_SYN, &buf, optlen);
> +	if (err) {
> +		log_err("Unexpected setsockopt(TCP_SAVED_SYN)");
> +		goto err;
> +	}

If the BPF program falls back to the native handler (because bpf_tcp_sock()
returned NULL on a fixed kernel), the kernel will process this through
raw_setsockopt() and ip_setsockopt(). Since the level is SOL_TCP, doesn't
ip_setsockopt() return -ENOPROTOOPT here?

This would cause setsockopt() to return -1 and err to be non-zero, meaning
the test will fail unconditionally on a correctly fixed kernel.

Ironically, on a buggy kernel where bpf_tcp_sock() incorrectly returns a
pointer, the BPF program hits the consumed label, sets ctx->optlen = -1
to bypass the kernel, and setsockopt() returns 0, causing the test to
falsely pass.

Should this test logic be inverted to expect an error (such as -ENOPROTOOPT)
to properly validate the fix?

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