Re: [PATCH net-next v8 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] The 'PortA, hsr-header' and 'PortB, hsr-header' cases of
  hsr_ptp_test do not check that the kernel preserves a supplied…

--

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

This adds a selftest for the inline header the HSR stack uses with ether
type ETH_P_1588. The test sends four sample PTP packets through the HSR
device: one to each port, each sent with and without an HSR header. It
checks that each packet shows up only on the requested slave port and
that the received payload matches what was sent.

> 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.

[ ... ]

> +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]
Do the "PortA, hsr-header" and "PortB, hsr-header" cases check that the
kernel keeps the HSR header supplied by the caller?

pkt_send() fills the foreign header with values that would identify it:

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

raw_recv() only checks dst, type, encap_type and the payload. It never
compares src, sequence_nr or pathid_and_LSDU_size with the values that
were sent.

On the kernel side, you can only see the foreign header handling on the
wire through these two paths in net/hsr/hsr_forward.c:

hsr_xmit() {
	if (frame->port_rcv->type == HSR_PT_MASTER && !frame->has_foreign_header) {
		hsr_addr_subst_dest(frame->node_src, skb, port);
		...
		ether_addr_copy(eth_hdr(skb)->h_source, port->dev->dev_addr);
	}
	...
}

hsr_forward_do() {
	...
		if (frame->has_foreign_header && frame->skb_std) {
			skb = skb_clone(frame->skb_std, GFP_ATOMIC);
	...
}

Suppose the !frame->has_foreign_header check in hsr_xmit() were dropped.
The source MAC would then be rewritten to the slave's address, and
hsr_addr_subst_dest() returns early for the multicast 01:80:c2:00:00:0e
destination. Would every check in raw_recv() still pass in that case?

Could raw_recv() also compare the source MAC, sequence_nr and path id
against the values pkt_send() used for the hsr-header cases?

Also, the file header comment says the interface needs to "accept a
foreign HSR header and prepend its own header". Is that accurate?
hsr_forward_do() seems to keep the supplied header ("PTP TX packets may
already have a HSR header which needs to be preserved") and does not
prepend a new one.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-hsr_ptp-v8-0-60dabc07e554%40linutronix.de