On 2026-10-06 14:31:15 [+0000], netdev-bot+sashiko@kernel.org wrote:
quoted
+ skb->dev = master->dev;
+ if (skb->protocol == htons(ETH_P_1588)) {
[Severity: Medium]
Does this drop every PTP frame that lacks the new inline header?
Yes. Intended.
…
That covers local senders that ask for TX timestamps. Does it also hold
for transit PTP traffic, or for PTP traffic that doesn't use
timestamps?
PTP traffic without timestamps does not sound like accurate usage of
PTP.
The commit message calls the header mandatory. It doesn't mention that
existing senders without the header now lose their traffic.
hsr_dev_xmit() isn't changed later in the series, so this stays in place.
Mandatory implies this.
quoted
+ struct hsr_inline_header *hsr_opt;
+ struct ethhdr *eth_hdr;
+ unsigned int hdr_len;
+
+ BUILD_BUG_ON(sizeof(struct hsr_inline_header) != sizeof(struct ethhdr));
+
+ /* need to access the magic header */
+ if (!pskb_may_pull(skb, sizeof(struct hsr_inline_header)))
+ goto drop;
+
+ hsr_opt = (struct hsr_inline_header *)skb_mac_header(skb);
+ if (hsr_opt->magic != htonl(HSR_INLINE_HDR))
+ goto drop;
[Severity: Medium]
Can a remote host provide this control header?
The only check is a constant magic in the source MAC position, and the
struct maps directly onto dst MAC, src MAC and ethertype.
Some frames are received from the wire and forwarded into hsr0 by tc
mirred (ingress to egress) or by an OVS output action. They keep
skb->protocol = ETH_P_1588 from eth_type_trans(). Their first 14 bytes
are the Ethernet header that the remote sender chose.
Consider a frame with:
- dst MAC {01 or 02, hsr_hdr, 00, 00, 00, 00}
- src MAC {af:48:53:52, 00, 00}
- ethertype 0x88f7
This shouldn't happen. This injection/ magic header is in hsr_dev_xmit()
and only for the HSR_PT_MASTER port which is the hsr0 interface. So it
does not happen on received packets which would be the HSR_PT_SLAVE_{AB}
interface. So no.
…[Severity: Low]
Should hsr_hdr be checked against the inner frame before it is trusted
here?
With hsr_hdr set, mac_len becomes ETH_HLEN + HSR_HLEN. Nothing checks
that eth_hdr->h_proto is really an HSR ethertype.
Take a VLAN-tagged frame with an encapsulated ETH_P_HSR and hsr_hdr = 1.
It gets past this guard in hsr_fill_frame_info(), which would otherwise
reject it:
if (skb->mac_len < sizeof(struct hsr_ethhdr))
return -EINVAL;
Then hsr_get_skb_sequence_nr() and hsr_set_path_id() read and write the
tag at mac_header + 14, which is inside the VLAN header, instead of at
mac_header + 18.
The pskb_may_pull() above keeps these accesses within the linear data.
The later commit "hsr: Use the port and header information in
hsr_forward_skb()" makes hsr_fill_frame_info() return early when
has_foreign_header is set. hsr_forward_do() then sends a clone of
skb_std without calling hsr_set_path_id(). So this affects only this
intermediate commit.
If this only effects intermediate commit, then it should be fine.
[ ... ]
quoted
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 73bc4b8950071..4afe9e1d65b7e 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -773,7 +773,8 @@ static int fill_frame_info(struct hsr_frame_info *frame,
}
/* Must be called holding rcu read lock (because of the port parameter) */
-void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port)
+void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port,
+ enum hsr_port_type tx_port, bool has_hsr_header)
{
struct hsr_frame_info frame;
[Severity: Low]
At this commit, hsr_forward_skb() accepts tx_port and has_hsr_header but
doesn't use them. The body still does:
…
The commit message says the values "will be used later". The follow-up
commit "hsr: Use the port and header information in hsr_forward_skb()"
adds frame->req_tx_port and frame->has_foreign_header. It acts on them in
fill_frame_info(), hsr_fill_frame_info(), hsr_forward_do() and
hsr_xmit(). So this only matters when bisecting onto this commit.
This is fine as-is.
Sebastian