Re: [PATCH v1 bpf-next/net 5/5] selftest: bpf: Add test for hwtstamp proxy.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Comparing a potentially negative return value to `sizeof()` causes failure handling to be skipped, erroneously dropping packets.
- [Medium] Incorrect struct cast for SO_TIMESTAMPING_NEW cmsg data causes test failures on 32-bit systems.
- [Medium] Typo in the test assertion prevents verification of TX timestamp keys.
- [Low] Multi-line comments violate the BPF subsystem comment style.
- [Low] The `buf_cmsg` array is not fully initialized before being passed to `sendmsg()`, passing uninitialized stack data to the kernel.
--

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

selftest: bpf: Add test for hwtstamp proxy.

This selftest simulates the hardware timestamp proxy scenario using two
UDP sockets, simulating a userspace proxy via a GENEVE device.

> diff --git a/tools/testing/selftests/bpf/prog_tests/proxy_hwtstamp.c b/tools/testing/selftests/bpf/prog_tests/proxy_hwtstamp.c
> new file mode 100644
> index 0000000000000..d0f90f22bea22
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/proxy_hwtstamp.c
[ ... ]
> +char *ipv4_commands[] = {
> +	"ip link set dev lo up",
> +	"ip link add geneve0 type geneve local " APP_SRC_IPV4 " external",
> +	"ip addr add " APP_SRC_IPV4 "/24 dev geneve0",
> +	"ip link set dev geneve0 address aa:bb:cc:dd:ee:ff",
> +	"ip link set dev geneve0 up",
> +	"ip route add " APP_DST_IPV4 "/32 dev geneve0",
> +	/* We do not forward ARP to the wire in this test,
> +	 * so a static neighbour entry is needed for APP_DST_IPV4.
> +	 */

[Severity: Low]
Does this multi-line comment format violate the BPF subsystem comment style?
It seems the subsystem guidelines prefer the opening /* on its own line for
multi-line comments.

> +	"ip neigh add " APP_DST_IPV4 " lladdr ab:bc:cd:de:ef:fa dev geneve0",
> +};
[ ... ]
> +static int check_tstamp(struct proxy_hwtstamp_test_case *test_case, bool tx)
> +{
[ ... ]
> +	for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
> +		if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SO_TIMESTAMPING_NEW) {
> +			struct scm_timestamping *ts;
> +
> +			ts = (struct scm_timestamping *)CMSG_DATA(cmsg);
> +			ASSERT_EQ(ts->ts[2].tv_sec, 0, "tv_sec");
> +			ASSERT_EQ(ts->ts[2].tv_nsec, HWTSTAMP, "tv_nsec");

[Severity: Medium]
Since the test requests SO_TIMESTAMPING_NEW, the kernel will return timestamps
using the 64-bit time architecture (via struct scm_timestamping64). Could
casting CMSG_DATA(cmsg) to struct scm_timestamping * cause issues on 32-bit
architectures where time_t is 32-bit?

It looks like this might misalign the array access, reading the zeroed
software timestamp ts[1] instead of the hardware timestamp, potentially
causing the test to fail.

> +
> +			saw_tstamp = true;
> +		} else if ((cmsg->cmsg_level == SOL_IP && cmsg->cmsg_type == IP_RECVERR) ||
> +			   (cmsg->cmsg_level == SOL_IPV6 && cmsg->cmsg_type == IPV6_RECVERR)) {
> +			struct sock_extended_err *ee;
> +
> +			ee = (struct sock_extended_err *)CMSG_DATA(cmsg);
> +
> +			if (ee->ee_origin == SO_EE_ORIGIN_TIMESTAMPING) {
> +				ASSERT_EQ(ee->ee_data, TSKEY, "tskey");
> +				saw_tskey = true;
> +			}
> +		}
> +	}
> +
> +	ASSERT_TRUE(saw_tstamp && (!tx || saw_tstamp), "no timestamp");

[Severity: Medium]
Should this check validate saw_tskey instead of checking saw_tstamp twice?
It looks like if the kernel fails to deliver the timestamp key, this check
will silently pass, potentially hiding regressions.

> +
> +	return ret;
> +}
> +
> +static int test_proxy_hwtstamp_tx(struct proxy_hwtstamp_test_case *test_case)
> +{
> +	char h_source_dummy[ETH_HLEN] = {0xFF, 0xEE, 0xDD, 0xCC, 0xBB, 0xAA};
> +	char buf_cmsg[CMSG_SPACE(sizeof(u32))];
> +	struct proxy_header *phdr;
> +	struct msghdr msg = {};
> +	struct iovec iov = {};
> +	struct cmsghdr *cmsg;
> +	int ret;
> +
> +	memset(test_case->app_payload, 0xAB, sizeof(test_case->app_payload));
> +	iov.iov_base = test_case->app_payload;
> +	iov.iov_len = sizeof(test_case->app_payload);
> +
> +	msg.msg_iov = &iov;
> +	msg.msg_iovlen = 1;
> +	msg.msg_control = buf_cmsg;
> +	msg.msg_controllen = sizeof(buf_cmsg);
> +
> +	cmsg = CMSG_FIRSTHDR(&msg);
> +	cmsg->cmsg_level = SOL_SOCKET;
> +	cmsg->cmsg_type = SCM_TS_OPT_ID;
> +	cmsg->cmsg_len = CMSG_LEN(sizeof(u32));
> +	*(u32 *)CMSG_DATA(cmsg) = TSKEY;
> +
> +	ret = sendmsg(test_case->app_fd, &msg, 0);

[Severity: Low]
Is it possible that uninitialized stack memory from buf_cmsg is being passed
to the kernel here?

Only the first CMSG_LEN bytes are initialized, but msg_controllen is set to
the full CMSG_SPACE size which includes uninitialized alignment padding.
Could this trigger MemorySanitizer warnings?

> +	if (!ASSERT_EQ(ret, sizeof(test_case->app_payload), "send"))
> +		return -1;
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/proxy_hwtstamp.c b/tools/testing/selftests/bpf/progs/proxy_hwtstamp.c
> new file mode 100644
> index 0000000000000..c15428e4c20f6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/proxy_hwtstamp.c
[ ... ]
> +SEC("tcx/egress")
> +int proxy_hwtstamp_egress(struct __sk_buff *skb)
> +{
[ ... ]
> +	kskb = bpf_cast_to_kern_ctx(skb);
> +	shared_info = bpf_core_cast(kskb->head + kskb->end, struct skb_shared_info);
> +	if (!shared_info->tx_flags) {
> +		/* If TX tstamp is not needed, don't insert the GENEVE option.
> +		 * The proxy socket will see genevehdr.opt_len == 0.
> +		 */

[Severity: Low]
Does this multi-line comment format violate the BPF subsystem comment style?
It seems the subsystem guidelines prefer the opening /* on its own line for
multi-line comments.

> +		goto pass;
> +	}
[ ... ]
> +SEC("tcx/ingress")
> +int proxy_hwtstamp_ingress(struct __sk_buff *skb)
> +{
> +	struct proxy_hwtstamp_opt opt;
> +	struct bpf_tunnel_key key;
> +	int ret;
> +
> +	/* Get the GENEVE header. */
> +	ret = bpf_skb_get_tunnel_key(skb, &key, sizeof(key), tunnel_rx_flags);
> +	if (ret < 0)
> +		goto drop;
> +
> +	if (key.tunnel_id != GENEVE_VNI)
> +		goto drop;
> +
> +	/* Get the GENEVE option. */
> +	ret = bpf_skb_get_tunnel_opt(skb, &opt, sizeof(opt));
> +	if (ret < sizeof(opt)) {

[Severity: High]
Can this check drop packets without a GENEVE option?

bpf_skb_get_tunnel_opt() returns a signed integer, but sizeof(opt) is
unsigned. If the return value is negative (e.g., -ENOENT), it will be
promoted to a large unsigned integer during the comparison.

Would this cause ret < sizeof(opt) to evaluate to false and skip the failure
handling, inadvertently dropping legitimate traffic?

> +		 /* If TX/RX tstamp is not needed, the proxy socket
> +		  * does not insert the GENEVE option.
> +		  */
> +		goto pass;
> +	}

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