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

[netdev-bot+sashiko@kernel.org]

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

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

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

- [High] In hsr_handle_frame() (net/hsr/hsr_slave.c) the new PTP drop
  only runs in the `else` branch for non-interlink ports.
- [Medium] The new PTP check in hsr_handle_frame() only compares the
  immediate Ethernet h_proto (untagged/PRP branch) or…
- [Low] The commit message says 'Drop all received PTP packets', but
  hsr_handle_frame() only matches Layer-2 PTP.
- [Low] In hsr_handle_frame(), the intentional PTP policy drops go
  through the shared finish_free_consume label.
- [Low] In hsr_handle_frame() the two new branches are labelled '/* HSR
  */' and '/* PRP */'.

--

hsr: Drop received PTP packets

This patch changes hsr_handle_frame() so that PTP frames received on
HSR/PRP slave ports are dropped. Before, they were forwarded and delivered
to the hsr master. Tagged HSR frames are checked by their encapsulated
protocol. All other frames are checked by their ethertype against
ETH_P_1588. The error paths now share a single finish_free_consume label.

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

[ ... ]

> @@ -81,10 +78,32 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
>  		hsr_forward_skb(skb, port, HSR_PT_NONE, false);
>  		spin_unlock_bh(&hsr->seqnr_lock);
>  	} else {
> +		struct hsr_ethhdr *hsr_ethhdr;
> +
> +		/* 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.
> +		 */

[Severity: High]
The new check is only in the else branch. Should PTP frames received on
the interlink (RedBox) port be dropped too?

When port->type == HSR_PT_INTERLINK, the frame is still forwarded without
any protocol check:

	if (port->type == HSR_PT_INTERLINK) {
		spin_lock_bh(&hsr->seqnr_lock);
		hsr_forward_skb(skb, port, HSR_PT_NONE, false);
		spin_unlock_bh(&hsr->seqnr_lock);

An L2 PTP multicast frame (ethertype 0x88F7) from a SAN then takes this
path:

hsr_handle_frame()
  hsr_forward_skb()
    hsr_forward_do()
      hsr_deliver_master()     -> delivered to hsr0/prp0
      create_tagged_frame()    -> sent out slave A and slave B

hsr_drop_frame() and prp_drop_frame() only check supervision frames and
unicast destinations found in the node/proxy tables. They don't filter
multicast PTP.

The frame gets a sequence number, is tagged, and is forwarded into the
ring with the uncompensated software latency. The commit message says this
should stop ("Drop all received PTP packets", "Forwarding a PTP packet by
the HSR stack is undesired").

The TX side works differently. hsr_dev_xmit() only lets ETH_P_1588 out on
SLAVE_A/SLAVE_B via the inline header. Suppose a PTP stack on the RedBox
also relays interlink PTP into the ring via the inline header. Would
downstream nodes then get two copies of each message, the kernel's
uncorrected one and the stack's corrected one?

hsr_slave.c doesn't seem to change again later in the series. Would
moving the ETH_P_1588 check above the interlink/else split cover both
paths?

> +		if ((!hsr->prot_version && protocol == htons(ETH_P_PRP)) ||
> +		    protocol == htons(ETH_P_HSR)) {
> +			/* HSR */

[Severity: Low]
This isn't a bug, but are the /* HSR */ and /* PRP */ labels accurate?

The condition tests whether an HSR tag is present, not whether the device
is set up for HSR or PRP. So:

- The "HSR" branch also handles PRP-mode frames that carry ETH_P_HSR.
- The "PRP" branch also handles untagged frames in general. This includes
  HSR-mode frames on ports with NETIF_F_HW_HSR_TAG_RM, where the tag has
  already been stripped.

The logic is correct in all of these cases. Would "tagged" and "untagged"
be more accurate labels?

> +			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 {
> +			/* PRP */
> +			if (protocol == htons(ETH_P_1588))
> +				goto finish_free_consume;

[Severity: Medium]
Can L2 PTP with stacked VLAN tags get past this check?

__netif_receive_skb_core() strips only one VLAN tag before the rx_handler
runs:

	if (eth_type_vlan(skb->protocol)) {
		skb = skb_vlan_untag(skb);

Take a frame with stacked tags, such as an 802.1ad outer tag and an
802.1Q inner tag. It reaches hsr_handle_frame() with
h_proto == ETH_P_8021Q.

prp_ops has no .invalid_dan_ingress_frame hook. On a PRP slave the frame
therefore reaches the untagged branch here, where 0x8100 is compared with
0x88F7 and doesn't match.

The frame then goes to hsr_forward_skb()->fill_frame_info(). That
function does unwrap the VLAN header to find the real protocol:

	if (proto == htons(ETH_P_8021Q))
		frame->is_vlan = true;

It applies no PTP rejection, though. The frame is delivered to the
master, and on a RedBox to the interlink as well.

The same applies to HSR frames whose tag has encap_proto 0x8100 (a VLAN
header after the HSR tag). They also get past the encap_proto comparison.

Should this filter look through a VLAN header the same way
fill_frame_info() does?

[Severity: Low]
The commit message says:

    Drop all received PTP packets.

Both comparisons here match only Layer-2 PTP (ETH_P_1588). PTP over
UDP/IPv4 or UDP/IPv6 (ports 319/320) has ethertype ETH_P_IP or
ETH_P_IPV6, or that value as encap_proto in the HSR tag. That traffic is
still de-duplicated, delivered to hsr0, and forwarded around the ring in
software.

Limiting this to L2 is probably intended. The IEC 62439-3 and
IEC 61850-9-3 profiles use L2 PTP, and the inline TX header in the rest of
the series only handles ETH_P_1588. Could the commit message and the
comment say "Layer-2 PTP (ETH_P_1588)" instead of "all received PTP
packets"?

>  		hsr_forward_skb(skb, port, HSR_PT_NONE, false);
>  	}
>  
> -finish_consume:
> +	return RX_HANDLER_CONSUMED;
> +
> +finish_free_consume:
> +	kfree_skb(skb);
>  	return RX_HANDLER_CONSUMED;

[Severity: Low]
Would a dedicated drop reason help here?

The deliberate PTP drops now share this label with two error paths: the
self-sent frame case and the failed pskb_may_pull() case. kfree_skb()
reports SKB_DROP_REASON_NOT_SPECIFIED and no counter is incremented.

On a network with a lot of PTP traffic, every Sync, Follow_Up, Delay_Req
and Pdelay frame shows up in drop_monitor and the kfree_skb tracepoint as
an unexplained drop. The error drops also can't be told apart from the
policy drops.

Could the PTP path use kfree_skb_reason() with its own reason?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-hsr_ptp-v6-0-6ea07b3fb8a8%40linutronix.de