Re: [PATCH net-next v8 4/8] hsr: Drop received PTP packets

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says "Drop all received PTP packets", but
  hsr_handle_frame() (net/hsr/hsr_slave.c) only matches Layer-2 PTP.

--

hsr: Drop received PTP packets

This patch changes hsr_handle_frame() so that PTP frames received on the
HSR/PRP slave ports are dropped instead of forwarded. It matches either an
outer ethertype of ETH_P_1588 or an HSR-encapsulated protocol of
ETH_P_1588. It also merges the kfree_skb() error paths into a single
finish_free_consume label.

> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> index 5274ba6dd36e6..a5b47f016abd7 100644
> --- a/net/hsr/hsr_slave.c
> +++ b/net/hsr/hsr_slave.c

[ ... ]

> @@ -64,15 +64,28 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)

[ ... ]

>  	skb_reset_mac_len(skb);
>  
> +	/* PTP packets are not supposed to be forwarded via HSR as-is. The
> +	 * latency introduced by forwarding renders the time information
> +	 * useless. Userland needs to capture the packet on the original
> +	 * interface instead of hsr.
> +	 */
> +	if ((!hsr->prot_version && protocol == htons(ETH_P_PRP)) ||
> +	    protocol == htons(ETH_P_HSR)) {
> +		hsr_ethhdr = (struct hsr_ethhdr *)skb_mac_header(skb);
> +		if (hsr_ethhdr->hsr_tag.encap_proto == htons(ETH_P_1588))
> +			goto finish_free_consume;
> +	} else {
> +		if (protocol == htons(ETH_P_1588))
> +			goto finish_free_consume;
> +	}
> +

[Severity: Medium]
The commit message says:

    Drop all received PTP packets.

Do these checks catch PTP carried over UDP? Both branches only match
ETH_P_1588, which is the Layer 2 mapping.

PTP over UDP/IPv4 or IPv6 (ports 319/320) has ETH_P_IP or ETH_P_IPV6 in
two places. For untagged, PRP, interlink or HW-tag-removed frames it is in
h_proto. For HSR-tagged frames it is in hsr_tag.encap_proto. Either way,
the frame takes neither drop branch.

These frames then go into hsr_forward_skb(). There they are de-duplicated,
delivered to the hsr master and forwarded around the ring. That causes the
same unaccounted forwarding latency and loss of per-port timestamps that
the commit message describes. linuxptp uses network_transport UDPv4 by
default, so this is a common configuration.

UDP PTP behaves the same as it did before this patch, and the IEC 62439-3
PTP profile for HSR/PRP uses the L2 mapping. Still, the commit message
names only VLAN-tagged packets as out of scope. The new comment also
states the policy for PTP in general.

Should the commit message and the comment say that only L2 (ETH_P_1588)
PTP is dropped? Or should the UDP case be handled as well?

>  	/* Only the frames received over the interlink port will assign a
>  	 * sequence number and require synchronisation vs other sender.
>  	 */

[ ... ]

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