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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2026-10-06 14:31:22 [+0000], netdev-bot+sashiko@kernel.org wrote:
> > +	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.

It does not check the HSR header details, true. It expects a reasonable
HSR header followed by the payload. The payload is checked but it is not
modified during the individual transfers. It is however checked that the
packet arrives only on one of the two ports and the "inline header" must
not exist. That should be enough.

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

That could be added. But a if you remove the "has_foreign_header" check
then we should end up with two HSR header at which point we don't have
the expected payload after the HSR header.

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

It is accurate. The interface either uses the "foreign HSR header" _or_
"prepend its own header". Both things are checked.

Sebastian