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

[netdev-bot+sashiko@kernel.org]

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

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

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 "VLAN tagged packets are not
  considered", but the code does drop VLAN-tagged PTP frames.
- [Low] Every received PTP frame goes to finish_free_consume, which calls
  kfree_skb(skb) and returns RX_HANDLER_CONSUMED (hsr_slave.c:101-103).

--

hsr: Drop received PTP packets

With this patch, hsr_handle_frame() drops PTP frames that arrive on HSR/PRP
slave ports instead of forwarding them. For HSR/PRP tagged frames it checks
the encapsulated protocol, and for other frames it checks the ethertype.
The existing kfree_skb() error paths are merged into a new
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:

  VLAN tagged packets are not considered.

Does that match what this code does? Before the rx_handler runs,
__netif_receive_skb_core() has already moved a single 802.1Q tag into
skb->vlan_tci. NIC rx-VLAN offload does this in hardware. Without offload,
the software path does it:

net/core/dev.c:__netif_receive_skb_core() {
    ...
	if (eth_type_vlan(skb->protocol)) {
		skb = skb_vlan_untag(skb);
    ...
}

As a result, protocol = eth_hdr(skb)->h_proto in hsr_handle_frame() holds
the inner ethertype:

- A VLAN-tagged PTP frame on a PRP slave matches
  protocol == htons(ETH_P_1588).
- A VLAN-tagged HSR frame matches
  hsr_tag.encap_proto == htons(ETH_P_1588).

Both frames take the finish_free_consume path.

The only exception seems to be a VLAN upper device on the slave that
matches the VID. In that case vlan_do_receive() moves the skb to that
device before hsr_handle_frame() is called.

Since VLAN-tagged PTP frames are dropped whether offload is on or off,
should the commit message say so? Or should these frames keep being
forwarded as before?

> +
>  	/* Only the frames received over the interlink port will assign a
>  	 * sequence number and require synchronisation vs other sender.
>  	 */
> @@ -84,7 +97,10 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
>  		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]
I have two questions about the PTP frames that now take this exit path.

First, RX_HANDLER_CONSUMED makes __netif_receive_skb_core() jump straight
to out:

	case RX_HANDLER_CONSUMED:
		ret = NET_RX_SUCCESS;
		goto out;

That skips ptype_base and ptype_specific delivery. Only ETH_P_ALL taps see
the frame, because they are delivered before the rx_handler. A packet
socket bound to ETH_P_1588 on the slave never receives it.

The commit message says a PTP stack "needs to retrieve the PTP packet on
the original slave interface where it was received". That works with
ETH_P_ALL plus a filter, which is what linuxptp's raw transport uses. It
does not work for a socket bound to ETH_P_1588. Could the commit message
mention this?

Alternatively, on PRP the frame is a plain ETH_P_1588 frame. Could it be
returned with RX_HANDLER_PASS after undoing the skb_push(), so that normal
protocol delivery takes place?

This isn't a new loss. Before this patch, ETH_P_1588-bound sockets on the
slave didn't get these frames either, because the frames were consumed for
forwarding.

Second, kfree_skb() reports SKB_DROP_REASON_NOT_SPECIFIED. Every PTP frame
filtered on purpose (sync, follow-up, announce, pdelay, on each slave port)
will show up in the kfree_skb tracepoint and drop_monitor as a drop with
no reason given. Would a dedicated drop reason, or consume_skb() since ETH_P_ALL
taps have already received the frame, fit better here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-hsr_ptp-v9-0-2d20a2a0f816%40linutronix.de