Re: [PATCH net-next v9 8/8] selftests: hsr: Add test for the inline PTP header on HSR
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-18 15:51:46
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.
quoted hunk ↗ jump to hunk
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)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ 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