Re: [PATCH net-next v9 8/8] selftests: hsr: Add test for the inline PTP header on HSR

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In tools/testing/selftests/net/hsr/hsr_ptp_test.c, the two 'hsr-
  header' cases never check whether the caller-supplied HSR header…
- [Low] The header comment of
  tools/testing/selftests/net/hsr/hsr_ptp_test.c (lines 5-7) says the…

--

selftests: hsr: Add test for the inline PTP header on HSR

This adds a selftest, hsr_ptp.sh and hsr_ptp_test.c, for the inline
ETH_P_1588 header that the HSR stack uses. The test sends a PTP frame
through the HSR master to port A or port B, with and without a
caller-supplied HSR header. It then checks that the frame shows up only on
the requested slave port and that the payload matches.

> diff --git a/tools/testing/selftests/net/hsr/hsr_ptp_test.c b/tools/testing/selftests/net/hsr/hsr_ptp_test.c
> new file mode 100644
> index 0000000000000..5b69d4594768b
> --- /dev/null
> +++ b/tools/testing/selftests/net/hsr/hsr_ptp_test.c
> @@ -0,0 +1,482 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Simple test to verify the usage of the inline header used for HSR with
> + * ether type ETH_P_1588.
> + * The inline header has to be stripped, the sent packet must only appear on the
> + * specified port and the interface needs to accept a foreign HSR header and
> + * prepend its own header.

[Severity: Low]
Does this match what the kernel does? When the inline header has hsr_hdr
set, the frame gets has_foreign_header, and hsr_forward_do() sends the
caller's frame unchanged:

net/hsr/hsr_forward.c:hsr_forward_do() {
    ...
		if (frame->has_foreign_header && frame->skb_std) {
			skb = skb_clone(frame->skb_std, GFP_ATOMIC);
    ...
}

That path skips hsr_create_tagged_frame(). hsr_xmit() also skips the
source address substitution for these frames:

	if (frame->port_rcv->type == HSR_PT_MASTER && !frame->has_foreign_header) {

So the foreign HSR header is kept and no second header is added. The test
expects this too: raw_recv() requires encap_type to be ETH_P_1588 right
after the first HSR header.

Should the comment say that the foreign header is kept as-is?

[ ... ]

> +static int raw_recv(int fd, void *sample, int sample_len)
> +{

[ ... ]

> +	if (memcmp(hsr_hdr->dst, p2p_dst_mac, MAC_LEN))
> +		goto again;
> +
> +	if (hsr_hdr->type != htons(ETH_P_HSR)) {
> +		printf("Error: Unexpected ether type: 0x%x\n", ntohs(hsr_hdr->type));
> +		return -1;
> +	}
> +
> +	if (hsr_hdr->encap_type != htons(ETH_P_1588)) {
> +		printf("Error: Unexpected encapsulated type: 0x%x\n",
> +		       htons(hsr_hdr->encap_type));
> +		return -1;
> +	}
> +
> +	if (cnt < sizeof(struct hsr_hdr) + sample_len) {
> +		printf("Error: Packet %d is too small for data check\n", cnt);
> +		return -1;
> +	}
> +
> +	if (!memcmp(&buf[sizeof(struct hsr_hdr)], sample, sample_len))
> +		return 1;

[Severity: Low]
In the two hsr-header cases, does this check that the caller-supplied HSR
header reaches the slave port unchanged?

pkt_send() fills the foreign header with distinctive values:

	hdr->hsr_hdr.src[3] ^= 0x21;
	...
	hdr->hsr_hdr.sequence_nr = htons(hsr_seq++);
	...
	pathid_size |= (port - 1) << 12;

raw_recv() only compares the destination MAC, the outer type, encap_type
and the payload. It never reads the received src, sequence_nr or
pathid_and_LSDU_size.

Keeping these fields intact is what makes the has_foreign_header path in
hsr_forward_do() and hsr_xmit() different from the normal path. Suppose
hsr_dev_xmit() stopped passing the hsr_hdr flag through. The frame would
then be handled as an ordinary HSR-tagged master frame:
hsr_create_tagged_frame() would clone it, and hsr_xmit() would overwrite
h_source with the slave's address. The result would still pass every check
here. Only a doubled header would be caught, through the encap_type check.

Could the test also compare the received src, sequence_nr and path id with
the values pkt_send() wrote?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-hsr_ptp-v9-0-2d20a2a0f816%40linutronix.de