Re: [PATCH net-next v9 4/8] hsr: Drop received PTP packets
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-18 15:51:43
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -64,15 +64,28 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
[ ... ]
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
+ /* 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