[PATCH net-next v6 0/8] hsr: Add additional info to send/ receive skbs

[PATCH net-next v6 0/8] hsr: Add additional info to send/ receive skbs

[Sebastian Andrzej Siewior]

I am trying to extend linuxptp to support PTP over a HSR network.

This is the kernel side of the changes. In short PTP over HSR sends its
packets to a multicast address and every node needs to forward the PTP
packet (SYNC and FOLLOW-UP for instance) within the HSR ring.
In order to achieve this, the HSR stack must not duplicate and forward
the PTP packets as it would do with other packets. The delay caused by
the duplication and forwarding adds overhead which in turn makes the
timing information within the PTP packet inaccurate.

My current approach is to open the slave devices (eth0/ eth1) from
userland in order to receive the PTP packets. Sending happens from the
hsr0 device. The actual packet has an inline header prepended of type
struct hsr_inline_header. The size of the header is equivalent to
ethhdr. The header has a type (h_proto) at the same position as ethhdr
and expects it to be ETH_P_1588 as this extra meta information is only
relevant for PTP packets. It makes no sense to send PTP packets via the
HSR interface because it gets duplicated and the timestamp information
is lost so this should not break anything. As an additional safe guard
there is a magic value at h_source position. The value has '0xaf' at the
most significant byte which makes the address a locally administered
multicast address.

The header passes two information from userland: On which slave port
the packet has to be sent and does the HSR stack need to prepend a
header or not.
The header is skipped so that the remaining stack sees the actual data
and can send it as requested.

The PRP packets are sent directly via the SLAVE interface. The standard
mandates not add a PRP trailer (PRP, redundancy control trailer) to PTP
packets. There is not really a reason to use hsr interface.

HSR hardware offloading is optional. The driver needs to know if the
operating mode is HSR or PRP. In PRP mode it needs to check the ether
type and for ETH_P_1588 it must not perform any offloading.
In HSR mode, for ether-type ETH_P_1588 there must be no offloading. If
the ether-type is ETH_P_HSR there must be no offloading if the
encapsulated protocol is ETH_P_1588.

This has been tested in a pure software environment and in an HW-assisted
environment where the HW is able to duplicate and duplicate packets
but does not do it for PTP packets.
It has not been tested within an environment where the HW is able to
forward the PTP packet and correctly update the timing information.

---
v5…v6: https://lore.kernel.org/r/20260527-hsr_ptp-v5-0-158a7633eac0@linutronix.de
- hsr_ptp_test: Let it wait up to 100ms for a packet. Otherwise it will
  complain if the recevied packet is not already in socket.
- Use TEST_GEN_PROGS instead TEST_GEN_FILES, the test can not run
  without additional arguments.
- Make the inline header mandatory for sending ETH_P_1588 packets via
  the HSR device.
- Use kfree_skb() instead kfree() for the skb.
- Rephrase the commit message saying that skb_clone() shares
  skb_shared_info and does not copy it.

v4…v5: https://lore.kernel.org/r/20260508-hsr_ptp-v4-1-aa19aa7c6a71@linutronix.de
- Split the patch into smaller pieces
- Added a test for the added inline header (which signals the port while
  sending packets).
- Replaced __pskb_copy() with skb_clone() + skb_cow_head() in
  hsr_create_tagged_frame() to preserve timestamp request.

v3…v4: https://lore.kernel.org/r/20260429-hsr_ptp-v3-1-afbf8f200f48@linutronix.de
- Removed skb extention. The information within HSR is passed via
  struct hsr_frame_info. Driver with HSR-offloading capabilities need to
  know the HSR mode (HSR or PRP) and parse the skb to decide what needs
  to be done (whether to send on both ports and if adding a header is
  needed).

v2…v3: https://patch.msgid.link/20260309-hsr_ptp-v2-0-798262aad3a4@linutronix.de
- Remove af_packet changes entirely.
- Add an internal header to pass additional information for HSR-PTP
  packets.
- Remove PRP, userland will use slave devices directly.
- Drop all received PTP packets. Userland needs to use the slave device
  for RX.

v1…v2: https://patch.msgid.link/20260204-hsr_ptp-v1-0-b421c69a77da@linutronix.de
- Added PRP support
- skb extention is used instead of extending struct skb_shared_info
- in af_packet
  - packet_sendmsg_spkt() is no longer extended
  - jump labels are used to avoid the overhead if there no socket that
    is using this HSR extension.

---
Sebastian Andrzej Siewior (8):
      hsr: Add header_ops::parse_protocol
      hsr: Use skb_clone() while adding the HSR header
      hsr: Add a magic header for sending PTP packets
      hsr: Drop received PTP packets
      hsr: Use the port and header information in hsr_forward_skb()
      hsr: Assign a socket for cloned skbs
      hsr: Move struct hsr_ethhdr to a global header
      selftests: hsr: Add test for the inline PTP header on HSR

 include/linux/if_hsr.h                         |  16 +
 net/hsr/hsr_device.c                           |  67 +++-
 net/hsr/hsr_forward.c                          |  77 +++-
 net/hsr/hsr_forward.h                          |   3 +-
 net/hsr/hsr_framereg.h                         |   2 +
 net/hsr/hsr_main.h                             |   5 -
 net/hsr/hsr_slave.c                            |  37 +-
 tools/testing/selftests/net/hsr/.gitignore     |   1 +
 tools/testing/selftests/net/hsr/Makefile       |   5 +
 tools/testing/selftests/net/hsr/hsr_ptp.sh     | 109 ++++++
 tools/testing/selftests/net/hsr/hsr_ptp_test.c | 495 +++++++++++++++++++++++++
 11 files changed, 774 insertions(+), 43 deletions(-)
---
base-commit: dec005ae90a2946656a090f37bf1cfbd22f08e57
change-id: 20260204-hsr_ptp-1f6380f1d35f

Best regards,
--  
Sebastian Andrzej Siewior <bigeasy@linutronix.de>

[PATCH net-next v6 1/8] hsr: Add header_ops::parse_protocol

[Sebastian Andrzej Siewior]

af_packet uses dev_parse_header_protocol() to assign skb::protocol.
The hsr stack does not assign a callback so the field is assigned to 0.

The header used by the hsr stack corresponds to the ethernet header for
the "first" part so the protocol member is at the same position.

Use eth_header_parse_protocol() for the hsr_header_ops's parse_protocol
callback.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/hsr/hsr_device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index d14de44e14b7a..6d96bc4a12446 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -247,6 +247,7 @@ static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 static const struct header_ops hsr_header_ops = {
 	.create	 = eth_header,
 	.parse	 = eth_header_parse,
+	.parse_protocol = eth_header_parse_protocol,
 };
 
 static struct sk_buff *hsr_init_skb(struct hsr_port *master, int extra)

-- 
2.55.0

Re: [PATCH net-next v6 1/8] hsr: Add header_ops::parse_protocol

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch has no Fixes: tag.

--

hsr: Add header_ops::parse_protocol

This patch adds eth_header_parse_protocol() as the parse_protocol callback
in hsr_header_ops. With it, af_packet's dev_parse_header_protocol() sets
skb->protocol correctly on hsr devices. Before this change the callback was
missing and the field was set to 0.

> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index fc512204600cd..59df647d2fd45 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -247,6 +247,7 @@ static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  static const struct header_ops hsr_header_ops = {
>  	.create	 = eth_header,
>  	.parse	 = eth_header_parse,
> +	.parse_protocol = eth_header_parse_protocol,
>  };

[Severity: Low]
This isn't a bug, but should this patch carry a Fixes: tag?

hsr_dev_setup() calls ether_setup(), which installs eth_header_ops. It then
replaces them:

net/hsr/hsr_device.c:hsr_dev_setup() {
    ...
	dev->header_ops = &hsr_header_ops;
    ...
}

The eth_header_ops parse_protocol callback is lost at this point. So for
SOCK_RAW sends with protocol 0 or ETH_P_ALL on an hsr device,
packet_parse_headers() has been leaving skb->protocol at 0 on the master:

net/packet/af_packet.c:packet_parse_headers() {
    ...
		skb->protocol = dev_parse_header_protocol(skb);
    ...
}

The frames that reach the slave ports still look correct. In hsr_forward.c,
fill_frame_info() reads ethhdr->h_proto directly. hsr_fill_tag(),
prp_fill_rct() and create_stripped_skb_hsr() also re-derive skb->protocol
from the header, for example:

net/hsr/hsr_forward.c:hsr_fill_tag() {
    ...
	skb->protocol = hsr_ethhdr->ethhdr.h_proto;
    ...
}

The remaining effects look limited to things like protocol-based tc or tap
matching on hsr0 egress. The patch also reads as preparation for the PTP
series rather than a fix. If it isn't meant for stable, leaving out the
Fixes: tag seems fine. Was that the intent?

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

[PATCH net-next v6 2/8] hsr: Use skb_clone() while adding the HSR header

[Sebastian Andrzej Siewior]

hsr_create_tagged_frame() creates a new skb where the HSR header can be
prepended via __pskb_copy(). This does not copy the skb_shared_info part
of the original skb so any timestamp request is lost.

An alternative is to use skb_clone() where the skb_shared_info part is
shared and then skb_cow_head() to make the header part writeable. This
avoids touching manually the tx_flags and tskey members.
Using skb_clone() allows also return early (after the clone) in the
offloaded case so this can be unified a bit.

This is a prepartion for PTP handling where timestatmps are requested.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/hsr/hsr_forward.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 7734a521a96c2..0c16fbf8fd14f 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -342,16 +342,18 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
 		/* set the lane id properly */
 		hsr_set_path_id(frame, hsr_ethhdr, port);
 		return skb_clone(frame->skb_hsr, GFP_ATOMIC);
-	} else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
-		return skb_clone(frame->skb_std, GFP_ATOMIC);
 	}
 
-	/* Create the new skb with enough headroom to fit the HSR tag */
-	skb = __pskb_copy(frame->skb_std,
-			  skb_headroom(frame->skb_std) + HSR_HLEN, GFP_ATOMIC);
+	skb = skb_clone(frame->skb_std, GFP_ATOMIC);
 	if (!skb)
 		return NULL;
-	skb_reset_mac_header(skb);
+
+	if (port->dev->features & NETIF_F_HW_HSR_TAG_INS)
+		return skb;
+
+	/* Ensure the cloned skb has enough headroom to fit the HSR tag */
+	if (skb_cow_head(skb, HSR_HLEN))
+		goto err;
 
 	if (skb->ip_summed == CHECKSUM_PARTIAL)
 		skb->csum_start += HSR_HLEN;
@@ -360,15 +362,24 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
 	if (frame->is_vlan)
 		movelen += VLAN_HLEN;
 
+	/* The entire header area needs to be linear */
+	if (!pskb_may_pull(skb, HSR_HLEN + movelen))
+		goto err;
+
 	src = skb_mac_header(skb);
 	dst = skb_push(skb, HSR_HLEN);
 	memmove(dst, src, movelen);
+	skb_set_network_header(skb, ETH_HLEN + HSR_HLEN);
 	skb_reset_mac_header(skb);
+	skb_reset_mac_len(skb);
 
 	/* skb_put_padto free skb on error and hsr_fill_tag returns NULL in
 	 * that case
 	 */
 	return hsr_fill_tag(skb, frame, port, port->hsr->prot_version);
+err:
+	kfree_skb(skb);
+	return NULL;
 }
 
 struct sk_buff *prp_create_tagged_frame(struct hsr_frame_info *frame,

-- 
2.55.0

Re: [PATCH net-next v6 2/8] hsr: Use skb_clone() while adding the HSR header

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 · 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).

- [High] hsr_create_tagged_frame() in net/hsr/hsr_forward.c now builds
  the tagged frame with skb_clone() + skb_cow_head(), but it still runs…
- [Medium] hsr_create_tagged_frame() now calls
  skb_set_network_header(skb, ETH_HLEN + HSR_HLEN) and…
- [Medium] For locally originated and interlink frames,
  hsr_create_tagged_frame() now places network_header after the HSR tag…
- [Low] In hsr_create_tagged_frame(), the new `pskb_may_pull(skb,
  HSR_HLEN + movelen)` (line 366) runs before skb_push(), while…

--

hsr: Use skb_clone() while adding the HSR header

This patch changes hsr_create_tagged_frame() to build the tagged frame
with skb_clone() and skb_cow_head() instead of __pskb_copy(). The goal is
to keep skb_shared_info, and with it any timestamp request. The
NETIF_F_HW_HSR_TAG_INS case now returns right after the clone, and the
network header and mac_len are now set explicitly after the HSR tag is
inserted.

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 44f6d9c83180a..d5e6597917c26 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -342,16 +342,18 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
>  		/* set the lane id properly */
>  		hsr_set_path_id(frame, hsr_ethhdr, port);
>  		return skb_clone(frame->skb_hsr, GFP_ATOMIC);
> -	} else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
> -		return skb_clone(frame->skb_std, GFP_ATOMIC);
>  	}
>  
> -	/* Create the new skb with enough headroom to fit the HSR tag */
> -	skb = __pskb_copy(frame->skb_std,
> -			  skb_headroom(frame->skb_std) + HSR_HLEN, GFP_ATOMIC);
> +	skb = skb_clone(frame->skb_std, GFP_ATOMIC);
>  	if (!skb)
>  		return NULL;
> -	skb_reset_mac_header(skb);
> +
> +	if (port->dev->features & NETIF_F_HW_HSR_TAG_INS)
> +		return skb;
> +
> +	/* Ensure the cloned skb has enough headroom to fit the HSR tag */
> +	if (skb_cow_head(skb, HSR_HLEN))
> +		goto err;
>  
>  	if (skb->ip_summed == CHECKSUM_PARTIAL)
>  		skb->csum_start += HSR_HLEN;

[Severity: High]
Is this csum_start adjustment still correct after the switch to
skb_clone()?

With __pskb_copy(), skb_copy_header() copied csum_start as a raw offset
from head into a buffer with HSR_HLEN more headroom. That made the
+HSR_HLEN necessary.

Now skb_clone() copies csum_start unchanged. The clone's header is always
cloned, so skb_cow_head() goes through pskb_expand_head(), which already
rebases the offset:

net/core/skbuff.c:skb_headers_offset_update() {
    ...
	if (skb->ip_summed == CHECKSUM_PARTIAL)
		skb->csum_start += off;
    ...
}

The skb_push() and memmove() below only move the Ethernet/VLAN header into
the headroom. L3/L4 stay where they are. That seems to leave csum_start
HSR_HLEN bytes past the real L4 header.

hsr_dev_setup() sets NETIF_F_HW_CSUM on the hsr device, so locally sent
TCP/UDP is CHECKSUM_PARTIAL. Consider a slave without
NETIF_F_HW_HSR_TAG_INS that falls back to skb_checksum_help() for
ETH_P_HSR. Wouldn't the checksum be computed from the wrong start and
written to the wrong offset (into the padding for a short ACK)?

When the linear area ends right after a 20 byte TCP header, would it hit
WARN_ONCE("offset+2 (%zu) > skb_headlen() (%u)\n") and drop the frame?
Slaves with HW_CSUM would get the shifted csum_start in their descriptors.

This line is still unchanged at the end of the series.

> @@ -360,15 +362,24 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
>  	if (frame->is_vlan)
>  		movelen += VLAN_HLEN;
>  
> +	/* The entire header area needs to be linear */
> +	if (!pskb_may_pull(skb, HSR_HLEN + movelen))
> +		goto err;

[Severity: Low]
This pull runs before skb_push(), while skb->data still points at the
original MAC header. The extra HSR_HLEN bytes come from headroom that
skb_cow_head() already guaranteed. Would pskb_may_pull(skb, movelen) be
enough here?

As written it requires skb->len >= movelen + HSR_HLEN. fill_frame_info()
only checks:

	if (skb->mac_len < sizeof(struct ethhdr))
		return -EINVAL;

So 14 to 19 byte frames (18 to 23 with VLAN) now reach the err label and
get freed. For example, AF_PACKET SOCK_RAW on the hsr device can send
them.

Before this patch, hsr_fill_tag() padded such frames and forwarded them:

	if (skb_put_padto(skb, ETH_ZLEN + HSR_HLEN))
		return NULL;

This is also still present at the end of the series.

> +
>  	src = skb_mac_header(skb);
>  	dst = skb_push(skb, HSR_HLEN);
>  	memmove(dst, src, movelen);
> +	skb_set_network_header(skb, ETH_HLEN + HSR_HLEN);

[Severity: Medium]
Should this offset include VLAN_HLEN when frame->is_vlan is set?

In that case movelen includes VLAN_HLEN, and hsr_fill_tag() puts the HSR
tag after the VLAN header:

	hsr_ethhdr = (struct hsr_ethhdr *)(pc + VLAN_HLEN);

So L3 starts at mac + ETH_HLEN + VLAN_HLEN + HSR_HLEN. As written,
network_header points 4 bytes into the HSR tag, and skb_reset_mac_len()
sets mac_len to 20 for in-band 802.1Q frames.

Two ways to get such frames: an 8021q upper with txvlan turned off on the
hsr device, or AF_PACKET frames that carry an 802.1Q header.

Before this line, skb_clone() plus skb_cow_head() had already left
network_header at the real L3 offset. The explicit set overwrites it. This
is still present at the end of the series.

[Severity: Medium]
Separately, hsr_fill_tag() sets skb->protocol to ETH_P_HSR or ETH_P_PRP.
For those protocols the flow dissector expects a struct hsr_tag at
skb_network_offset():

net/core/flow_dissector.c:__skb_flow_dissect() {
    ...
		hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
		...
		proto = hdr->encap_proto;
		nhoff += HSR_HLEN;
    ...
}

With network_header now past the HSR tag, won't the dissector treat the
inner IP header as the HSR tag? It would take encap_proto from the IP ID
field and then skip another 6 bytes.

The old __pskb_copy() path left network_header at the start of the HSR
tag for locally originated frames, so dissection of those frames used to
work. Ring-forwarded frames already had this layout from
hsr_handle_frame(), but this patch extends it to local and interlink
traffic.

This would affect egress tc flower matching on the slave (fl_classify()).
It would also affect skb_get_hash() for traffic without a precomputed
hash, which could spread one flow across fq_codel buckets or TX queues.
It is not fixed later in the series.

>  	skb_reset_mac_header(skb);
> +	skb_reset_mac_len(skb);

[ ... ]

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

[PATCH net-next v6 3/8] hsr: Add a magic header for sending PTP packets

[Sebastian Andrzej Siewior]

Sending PTP packets (ETH_P_1588) via the HSR stack is pointless in its
current shape because the requested PTP timestamp is not routed to the
sender. It also needs to be distinguished on which port the message
should be sent and whether or not a HSR header should be attached.

To pass this information, a custom header (struct hsr_inline_header) is
introduced. This header is expected and mandatory if the ether type is
ETH_P_1588. To avoid any wrong usage, there is a magic field to ensure
it really is the header.

The inline header is just prepended containing the port and header
parameter. After the header, a regular packet follows. It is ensured that
the inline header and the following ethernet or HSR header is linear
and can be accessed.

The two retrieved parameters are passed to hsr_forward_skb() and will be
used later.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/if_hsr.h | 11 +++++++++
 net/hsr/hsr_device.c   | 66 +++++++++++++++++++++++++++++++++++++++++---------
 net/hsr/hsr_forward.c  |  3 ++-
 net/hsr/hsr_forward.h  |  3 ++-
 net/hsr/hsr_slave.c    |  4 +--
 5 files changed, 71 insertions(+), 16 deletions(-)

diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
index f4cf2dd36d193..1db1bd0fa181d 100644
--- a/include/linux/if_hsr.h
+++ b/include/linux/if_hsr.h
@@ -3,6 +3,7 @@
 #define _LINUX_IF_HSR_H_
 
 #include <linux/types.h>
+#include <linux/skbuff.h>
 
 struct net_device;
 
@@ -22,6 +23,16 @@ enum hsr_port_type {
 	HSR_PT_PORTS,	/* This must be the last item in the enum */
 };
 
+#define HSR_INLINE_HDR	0xaf485352
+struct hsr_inline_header {
+	uint8_t tx_port;
+	uint8_t hsr_hdr;
+	uint8_t __pad0[4];
+	__be32 magic;
+	uint8_t __pad1[2];
+	__be16 eth_type;
+} __packed;
+
 /* HSR Tag.
  * As defined in IEC-62439-3:2010, the HSR tag is really { ethertype = 0x88FB,
  * path, LSDU_size, sequence Nr }. But we let eth_header() create { h_dest,
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 6d96bc4a12446..c5ea7ef0209db 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -223,24 +223,66 @@ static netdev_features_t hsr_fix_features(struct net_device *dev,
 
 static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 {
+	enum hsr_port_type tx_port = HSR_PT_NONE;
 	struct hsr_priv *hsr = netdev_priv(dev);
 	struct hsr_port *master;
+	bool has_header = false;
 
 	rcu_read_lock();
 	master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
-	if (master) {
-		skb->dev = master->dev;
-		skb_reset_mac_header(skb);
-		skb_reset_mac_len(skb);
-		spin_lock_bh(&hsr->seqnr_lock);
-		hsr_forward_skb(skb, master);
-		spin_unlock_bh(&hsr->seqnr_lock);
-	} else {
-		dev_core_stats_tx_dropped_inc(dev);
-		dev_kfree_skb_any(skb);
+	if (!master)
+		goto drop;
+
+	skb->dev = master->dev;
+	if (skb->protocol == htons(ETH_P_1588)) {
+		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;
+
+		has_header = hsr_opt->hsr_hdr;
+		tx_port = hsr_opt->tx_port;
+		if (tx_port != HSR_PT_SLAVE_A && tx_port != HSR_PT_SLAVE_B)
+			goto drop;
+
+		if (has_header)
+			hdr_len = ETH_HLEN + HSR_HLEN;
+		else
+			hdr_len = ETH_HLEN;
+
+		/* Ensure the real header after the stripped one can be
+		 * accessed
+		 */
+		if (!pskb_may_pull(skb, sizeof(struct hsr_inline_header) +
+				   hdr_len))
+			goto drop;
+
+		eth_hdr = skb_pull(skb, sizeof(struct hsr_inline_header));
+		skb_set_network_header(skb, hdr_len);
+		skb->protocol = eth_hdr->h_proto;
 	}
+
+	skb_reset_mac_header(skb);
+	skb_reset_mac_len(skb);
+	spin_lock_bh(&hsr->seqnr_lock);
+	hsr_forward_skb(skb, master, tx_port, has_header);
+	spin_unlock_bh(&hsr->seqnr_lock);
 	rcu_read_unlock();
 
+	return NETDEV_TX_OK;
+drop:
+	rcu_read_unlock();
+	dev_core_stats_tx_dropped_inc(dev);
+	dev_kfree_skb_any(skb);
 	return NETDEV_TX_OK;
 }
 
@@ -362,7 +404,7 @@ static void send_hsr_supervision_frame(struct hsr_port *port,
 		return;
 	}
 
-	hsr_forward_skb(skb, port);
+	hsr_forward_skb(skb, port, HSR_PT_NONE, false);
 	spin_unlock_bh(&hsr->seqnr_lock);
 	return;
 }
@@ -430,7 +472,7 @@ static void send_prp_supervision_frame(struct hsr_port *master,
 		return;
 	}
 
-	hsr_forward_skb(skb, master);
+	hsr_forward_skb(skb, master, HSR_PT_NONE, false);
 	spin_unlock_bh(&hsr->seqnr_lock);
 }
 
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 0c16fbf8fd14f..7cae91dd3fc6b 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -776,7 +776,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;
 
diff --git a/net/hsr/hsr_forward.h b/net/hsr/hsr_forward.h
index 206636750b300..e64b0358907a9 100644
--- a/net/hsr/hsr_forward.h
+++ b/net/hsr/hsr_forward.h
@@ -13,7 +13,8 @@
 #include <linux/netdevice.h>
 #include "hsr_main.h"
 
-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 sk_buff *prp_create_tagged_frame(struct hsr_frame_info *frame,
 					struct hsr_port *port);
 struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index a546f70f9cc8b..5274ba6dd36e6 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -78,10 +78,10 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
 	 */
 	if (port->type == HSR_PT_INTERLINK) {
 		spin_lock_bh(&hsr->seqnr_lock);
-		hsr_forward_skb(skb, port);
+		hsr_forward_skb(skb, port, HSR_PT_NONE, false);
 		spin_unlock_bh(&hsr->seqnr_lock);
 	} else {
-		hsr_forward_skb(skb, port);
+		hsr_forward_skb(skb, port, HSR_PT_NONE, false);
 	}
 
 finish_consume:

-- 
2.55.0

Re: [PATCH net-next v6 3/8] hsr: Add a magic header for sending PTP packets

[netdev-bot+sashiko]

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

Critical: 1 · 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).

- [Critical] net/hsr: UAPI breakage for standard ETH_P_1588 packets
- [Medium] The patch adds a new userspace ABI: an in-band 14-byte header
  (struct hsr_inline_header, magic HSR_INLINE_HDR 0xaf485352) that PTP…
- [Low] At this commit hsr_forward_skb() (net/hsr/hsr_forward.c:779-789)
  accepts the new tx_port and has_hsr_header parameters but ignores them.

--

hsr: Add a magic header for sending PTP packets

This commit adds struct hsr_inline_header, which must be prepended to
ETH_P_1588 frames sent through the hsr device. The header is identified
by a magic value and carries the requested transmit port and whether an
HSR header is already present. hsr_dev_xmit() checks and strips the
header, then passes both values to hsr_forward_skb(), which gains two new
parameters.

> diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
> index f4cf2dd36d193..1db1bd0fa181d 100644
> --- a/include/linux/if_hsr.h
> +++ b/include/linux/if_hsr.h

[ ... ]

> @@ -22,6 +23,16 @@ enum hsr_port_type {
>  	HSR_PT_PORTS,	/* This must be the last item in the enum */
>  };
>  
> +#define HSR_INLINE_HDR	0xaf485352
> +struct hsr_inline_header {
> +	uint8_t tx_port;
> +	uint8_t hsr_hdr;
> +	uint8_t __pad0[4];
> +	__be32 magic;
> +	uint8_t __pad1[2];
> +	__be16 eth_type;
> +} __packed;

[Severity: Medium]
Userspace has to build this header for every PTP frame written to an hsr
device, so isn't this a new userspace ABI? It is defined in
include/linux/if_hsr.h, which is not exported. The only exported HSR UAPI
header is include/uapi/linux/hsr_netlink.h, and the patch does not
document the format anywhere.

The tx_port values are the internal enum hsr_port_type values
(HSR_PT_SLAVE_A = 1, HSR_PT_SLAVE_B = 2). That enum lives in the same
non-UAPI header and has a "must be the last item" ordering rule. If the
enum is ever reordered, would a given userspace value then select a
different port without any warning?

The selftest in the series (tools/testing/selftests/net/hsr/hsr_ptp_test.c)
already has to redefine struct hsr_inline_header and HSR_INLINE_HDR. It
also hard-codes the port numbers:

    #define PORT_1 1
    #define PORT_2 2

Also, hsr_dev_xmit() only checks magic and tx_port:

    has_header = hsr_opt->hsr_hdr;
    tx_port = hsr_opt->tx_port;
    if (tx_port != HSR_PT_SLAVE_A && tx_port != HSR_PT_SLAVE_B)
        goto drop;

__pad0[], __pad1[] and eth_type are never checked, and any non-zero
hsr_hdr is treated as true. Kernels with this patch will accept any value
in those fields. Doesn't that mean these fields can never be given a
meaning later?

Could the definition and the port values move to a uapi header, with the
format documented and non-zero padding rejected?

[ ... ]

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index d5e6597917c26..bb111a18d24e9 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -776,7 +776,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
never uses them. The body is unchanged:

net/hsr/hsr_forward.c:hsr_forward_skb() {
    ...
	if (fill_frame_info(&frame, skb, port) < 0)
		goto out_drop;

	hsr_register_frame_in(frame.node_src, port, frame.sequence_nr);
	hsr_forward_do(&frame);
    ...
}

hsr_dev_xmit() now parses and strips the inline header and passes both
values in. With only this commit applied, would a PTP frame that asks for
one slave port still be sent on both ports?

And would a frame with hsr_hdr = 1, which already carries an HSR header,
still go through node registration, duplicate tracking and source
substitution?

This is fixed later in the series by "hsr: Use the port and header
information in hsr_forward_skb()". That commit passes tx_port and
has_hsr_header to fill_frame_info(), skips ports that don't match in
hsr_forward_do(), and takes the foreign-header shortcut. The commit
message here also says the parameters "will be used later", so this only
affects bisection.

Could the parsing and the functional change go into the same commit, so
the new header isn't a no-op in between?

[ ... ]

> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -223,24 +223,66 @@ static netdev_features_t hsr_fix_features(struct net_device *dev,
>  
>  static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  {
> +	enum hsr_port_type tx_port = HSR_PT_NONE;
>  	struct hsr_priv *hsr = netdev_priv(dev);
>  	struct hsr_port *master;
> +	bool has_header = false;
>  
>  	rcu_read_lock();
>  	master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
> -	if (master) {
> -		skb->dev = master->dev;
> -		skb_reset_mac_header(skb);
> -		skb_reset_mac_len(skb);
> -		spin_lock_bh(&hsr->seqnr_lock);
> -		hsr_forward_skb(skb, master);
> -		spin_unlock_bh(&hsr->seqnr_lock);
> -	} else {
> -		dev_core_stats_tx_dropped_inc(dev);
> -		dev_kfree_skb_any(skb);
> +	if (!master)
> +		goto drop;
> +
> +	skb->dev = master->dev;
> +	if (skb->protocol == htons(ETH_P_1588)) {
> +		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;
> +
> +		has_header = hsr_opt->hsr_hdr;
> +		tx_port = hsr_opt->tx_port;
> +		if (tx_port != HSR_PT_SLAVE_A && tx_port != HSR_PT_SLAVE_B)
> +			goto drop;
> +
> +		if (has_header)
> +			hdr_len = ETH_HLEN + HSR_HLEN;
> +		else
> +			hdr_len = ETH_HLEN;
> +
> +		/* Ensure the real header after the stripped one can be
> +		 * accessed
> +		 */
> +		if (!pskb_may_pull(skb, sizeof(struct hsr_inline_header) +
> +				   hdr_len))
> +			goto drop;
> +
> +		eth_hdr = skb_pull(skb, sizeof(struct hsr_inline_header));
> +		skb_set_network_header(skb, hdr_len);
> +		skb->protocol = eth_hdr->h_proto;
>  	}
> +
> +	skb_reset_mac_header(skb);
> +	skb_reset_mac_len(skb);
> +	spin_lock_bh(&hsr->seqnr_lock);
> +	hsr_forward_skb(skb, master, tx_port, has_header);
> +	spin_unlock_bh(&hsr->seqnr_lock);
>  	rcu_read_unlock();
>  
> +	return NETDEV_TX_OK;
> +drop:
> +	rcu_read_unlock();
> +	dev_core_stats_tx_dropped_inc(dev);
> +	dev_kfree_skb_any(skb);
>  	return NETDEV_TX_OK;
>  }
>  

[Severity: Critical]
Before this patch, an ETH_P_1588 frame sent on the hsr device took the
same path as any other frame. hsr_dev_xmit() handed it to
hsr_forward_skb(), and it went out on both slave ports with the HSR tag
or PRP trailer added.

After this patch, any skb with skb->protocol == htons(ETH_P_1588) that
does not start with struct hsr_inline_header is dropped here. Isn't
that a user-visible regression? An existing application that writes
plain PTP frames to an AF_PACKET socket bound to the hsr interface had
those frames duplicated onto both LANs. Now every one of them is
thrown away, even if it never cared about getting a TX timestamp back.

The drop is also silent. The only trace is the tx_dropped counter
bumped by dev_core_stats_tx_dropped_inc(), and sendmsg() still returns
success. A user upgrading the kernel would have a hard time working
out why PTP stopped working over the hsr device.

The new layout places magic over h_source[0..3]. The 0xaf in the first
byte has the group bit set, so a valid ethernet source address can
never match HSR_INLINE_HDR. Given that, could a frame without the magic
fall through to the old forwarding path instead of hitting goto drop?
The inline header would then be opt-in rather than mandatory.

If dropping plain PTP frames is intentional, could the commit message
say so and explain why the old behaviour is not worth keeping? The
other option is to select the new mode explicitly, for example per
socket or per hsr device through netlink, rather than keying it only
on the ethertype. Which of these did you have in mind?

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

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

[Sebastian Andrzej Siewior]

Receiving PTP packets via the HSR interface does not make sense. The HSR
stack will forward one copy to the user and ignore the duplicate from
the other port. The PTP stack is however interested in both copies since
they will have different content (due to different processing times
within the HSR ring) and different timestamp information which is not
forwarded at all.

Forwarding a PTP packet by the HSR stack is undesired because the
forwarding takes time, it is not accounted in the packet and this makes
the outgoing PTP packet inaccurate and therefore useless.

Drop all received PTP packets. For the PRP configuration it is the
ether type, for HSR configuration it is the encapsulated protocol, that
is checked. A PTP stack that is doing PTP over a HSR network needs to
retrieve the PTP packet on the original slave interface where it was
received.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/hsr/hsr_slave.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

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
@@ -44,8 +44,7 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
 
 	if (hsr_addr_is_self(port->hsr, eth_hdr(skb)->h_source)) {
 		/* Directly kill frames sent by ourselves */
-		kfree_skb(skb);
-		goto finish_consume;
+		goto finish_free_consume;
 	}
 
 	/* For HSR, only tagged frames are expected (unless the device offloads
@@ -64,10 +63,8 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
 	skb_reset_mac_header(skb);
 	if ((!hsr->prot_version && protocol == htons(ETH_P_PRP)) ||
 	    protocol == htons(ETH_P_HSR)) {
-		if (!pskb_may_pull(skb, ETH_HLEN + HSR_HLEN)) {
-			kfree_skb(skb);
-			goto finish_consume;
-		}
+		if (!pskb_may_pull(skb, ETH_HLEN + HSR_HLEN))
+			goto finish_free_consume;
 
 		skb_set_network_header(skb, ETH_HLEN + HSR_HLEN);
 	}
@@ -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.
+		 */
+		if ((!hsr->prot_version && protocol == htons(ETH_P_PRP)) ||
+		    protocol == htons(ETH_P_HSR)) {
+			/* 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 {
+			/* PRP */
+			if (protocol == htons(ETH_P_1588))
+				goto finish_free_consume;
+		}
+
 		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;
 
 finish_pass:

-- 
2.55.0

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

[netdev-bot+sashiko]

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

[PATCH net-next v6 5/8] hsr: Use the port and header information in hsr_forward_skb()

[Sebastian Andrzej Siewior]

The upper layer can submit a port on which the skb should be sent and
whether or not a HSR header should be added. Use this information while
forwarding the packet:
- If the port set, deliver it only on the requested port.
- If a header is already present then skip parts of the stack:
  - The packet originates from the master port and we don't want to
    assign a sequence number to it, it needs to preserve the original
    one.
  - The existing h_source must be used (and not replaced).
  - The node should already exist in the node_db if "other" packets are
    exchaned. If the node information is not yet existing then
    creating it makes no sense because it is used for deduplication of
    incoming packets and this is not used in the PTP case. If no other
    packets are exchanged then the node_db will age and then removed.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/hsr/hsr_forward.c  | 45 ++++++++++++++++++++++++++++++++++++---------
 net/hsr/hsr_framereg.h |  2 ++
 2 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 7cae91dd3fc6b..0f708ef598ded 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -431,7 +431,7 @@ static void hsr_deliver_master(struct sk_buff *skb, struct net_device *dev,
 static int hsr_xmit(struct sk_buff *skb, struct hsr_port *port,
 		    struct hsr_frame_info *frame)
 {
-	if (frame->port_rcv->type == HSR_PT_MASTER) {
+	if (frame->port_rcv->type == HSR_PT_MASTER && !frame->has_foreign_header) {
 		hsr_addr_subst_dest(frame->node_src, skb, port);
 
 		/* Address substitution (IEC62439-3 pp 26, 50): replace mac
@@ -552,11 +552,12 @@ bool hsr_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port)
 static void hsr_forward_do(struct hsr_frame_info *frame)
 {
 	struct hsr_port *port;
-	struct sk_buff *skb;
 	bool sent = false;
 
 	hsr_for_each_port(frame->port_rcv->hsr, port) {
 		struct hsr_priv *hsr = port->hsr;
+		struct sk_buff *skb = NULL;
+
 		/* Don't send frame back the way it came */
 		if (port == frame->port_rcv)
 			continue;
@@ -575,6 +576,17 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
 		if ((port->dev->features & NETIF_F_HW_HSR_DUP) && sent)
 			continue;
 
+		/* PTP TX packets have an outgoing port specified */
+		if (frame->req_tx_port != HSR_PT_NONE && frame->req_tx_port != port->type)
+			continue;
+		/* PTP TX packets may already have a HSR header which needs to
+		 * be preserved
+		 */
+		if (frame->has_foreign_header && frame->skb_std) {
+			skb = skb_clone(frame->skb_std, GFP_ATOMIC);
+			goto inject_into_stack;
+		}
+
 		/* Don't send frame over port where it has been sent before.
 		 * Also for SAN, this shouldn't be done.
 		 */
@@ -602,6 +614,7 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
 		else
 			skb = hsr->proto_ops->get_untagged_frame(frame, port);
 
+inject_into_stack:
 		if (!skb) {
 			frame->port_rcv->dev->stats.rx_dropped++;
 			continue;
@@ -666,6 +679,13 @@ int hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
 	struct hsr_port *port = frame->port_rcv;
 	struct hsr_priv *hsr = port->hsr;
 
+	if (frame->has_foreign_header) {
+		frame->skb_std = skb;
+
+		WARN_ON_ONCE(port->type != HSR_PT_MASTER);
+		WARN_ON_ONCE(skb->mac_len < sizeof(struct hsr_ethhdr));
+		return 0;
+	}
 	/* HSRv0 supervisory frames double as a tag so treat them as tagged. */
 	if ((!hsr->prot_version && proto == htons(ETH_P_PRP)) ||
 	    proto == htons(ETH_P_HSR)) {
@@ -707,7 +727,8 @@ int prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
 }
 
 static int fill_frame_info(struct hsr_frame_info *frame,
-			   struct sk_buff *skb, struct hsr_port *port)
+			   struct sk_buff *skb, struct hsr_port *port,
+			   enum hsr_port_type tx_port, bool has_hsr_header)
 {
 	struct hsr_priv *hsr = port->hsr;
 	struct hsr_vlan_ethhdr *vlan_hdr;
@@ -730,10 +751,15 @@ static int fill_frame_info(struct hsr_frame_info *frame,
 	if (port->type == HSR_PT_INTERLINK)
 		n_db = &hsr->proxy_node_db;
 
-	frame->node_src = hsr_get_node(port, n_db, skb,
-				       frame->is_supervision, port->type);
-	if (!frame->node_src)
-		return -1; /* Unknown node and !is_supervision, or no mem */
+	frame->req_tx_port = tx_port;
+	frame->has_foreign_header = has_hsr_header;
+
+	if (!frame->has_foreign_header) {
+		frame->node_src = hsr_get_node(port, n_db, skb,
+					       frame->is_supervision, port->type);
+		if (!frame->node_src)
+			return -1; /* Unknown node and !is_supervision, or no mem */
+	}
 
 	ethhdr = (struct ethhdr *)skb_mac_header(skb);
 	frame->is_vlan = false;
@@ -782,10 +808,11 @@ void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port,
 	struct hsr_frame_info frame;
 
 	rcu_read_lock();
-	if (fill_frame_info(&frame, skb, port) < 0)
+	if (fill_frame_info(&frame, skb, port, tx_port, has_hsr_header) < 0)
 		goto out_drop;
 
-	hsr_register_frame_in(frame.node_src, port, frame.sequence_nr);
+	if (!frame.has_foreign_header)
+		hsr_register_frame_in(frame.node_src, port, frame.sequence_nr);
 	hsr_forward_do(&frame);
 	rcu_read_unlock();
 	/* Gets called for ingress frames as well as egress from master port.
diff --git a/net/hsr/hsr_framereg.h b/net/hsr/hsr_framereg.h
index 127a3fb64d5f0..e5e6363c0466f 100644
--- a/net/hsr/hsr_framereg.h
+++ b/net/hsr/hsr_framereg.h
@@ -29,6 +29,8 @@ struct hsr_frame_info {
 	bool is_from_san;
 	bool dst_in_node_db;
 	bool dst_in_proxy_node_db;
+	bool has_foreign_header;
+	enum hsr_port_type req_tx_port;
 };
 
 void hsr_del_self_node(struct hsr_priv *hsr);

-- 
2.55.0

Re: [PATCH net-next v6 5/8] hsr: Use the port and header information in hsr_forward_skb()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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] This patch makes the per-protocol
  hsr_proto_ops->fill_frame_info callback responsible for handling…
- [Medium] hsr_forward_do() now applies the requested TX port
  (`frame->req_tx_port != port->type` then continue) and the pre-built-…

--

hsr: Use the port and header information in hsr_forward_skb()

This commit makes hsr_forward_skb() use the TX port requested by the upper
layer, and a flag that says the skb already carries an HSR header. When the
header is present, the forwarding path skips sequence number assignment,
node lookup and source address substitution, and sends a clone of the frame
unchanged on the requested port.

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index bb111a18d24e9..b68aada1a1393 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c

[ ... ]

> @@ -575,6 +576,17 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
>  		if ((port->dev->features & NETIF_F_HW_HSR_DUP) && sent)
>  			continue;
>  
> +		/* PTP TX packets have an outgoing port specified */
> +		if (frame->req_tx_port != HSR_PT_NONE && frame->req_tx_port != port->type)
> +			continue;
> +		/* PTP TX packets may already have a HSR header which needs to
> +		 * be preserved
> +		 */
> +		if (frame->has_foreign_header && frame->skb_std) {
> +			skb = skb_clone(frame->skb_std, GFP_ATOMIC);
> +			goto inject_into_stack;
> +		}
> +

[Severity: Medium]
How does this work on slave ports that advertise NETIF_F_HW_HSR_DUP or
NETIF_F_HW_HSR_TAG_INS?

The commit message says "If the port set, deliver it only on the requested
port". Here, though, the port is only selected in software. After
dev_queue_xmit() hands the frame to the requested slave, the lower device
may still duplicate it. For DSA, dsa_xmit_port_mask() in net/dsa/tag.h
does:

	if (IS_ENABLED(CONFIG_HSR) &&
	    unlikely(dev->features & NETIF_F_HW_HSR_DUP)) {
		...
		dsa_hsr_foreach_port(other_dp, dp->ds, hsr_dev)
			mask |= BIT(other_dp->index);

tag_ksz.c and tag_xrs700x.c both use this. The TI icssg driver also sends
the frame as undirected when NETIF_F_HW_HSR_DUP is set.

Would a PTP message meant for one port then go out on both rings?

For NETIF_F_HW_HSR_TAG_INS, hsr_create_tagged_frame() normally leaves tag
insertion to the hardware:

	if (port->dev->features & NETIF_F_HW_HSR_TAG_INS)
		return skb;

The new has_foreign_header branch skips that. It passes a frame that is
already tagged to hardware that is set up to insert its own tag. Depending
on the device, could that produce a second HSR tag or a sequence number
assigned by the hardware?

A later patch in this series ("hsr: Move struct hsr_ethhdr to a global
header") notes that when offloading, the network driver has to inspect the
HSR header to turn off offloading for PTP. However, the series doesn't
change any driver or tagger.

Should this path reject the offloaded case, or handle it?

[ ... ]

> @@ -666,6 +679,13 @@ int hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
>  	struct hsr_port *port = frame->port_rcv;
>  	struct hsr_priv *hsr = port->hsr;
>  
> +	if (frame->has_foreign_header) {
> +		frame->skb_std = skb;
> +
> +		WARN_ON_ONCE(port->type != HSR_PT_MASTER);
> +		WARN_ON_ONCE(skb->mac_len < sizeof(struct hsr_ethhdr));
> +		return 0;
> +	}

[Severity: Medium]
Does prp_fill_frame_info() need the same early return?

The generic code now assumes that every proto_ops->fill_frame_info()
handles has_foreign_header:

- fill_frame_info() skips hsr_get_node(), so node_src stays NULL.
- hsr_forward_skb() skips hsr_register_frame_in().
- The bypass in hsr_forward_do() only runs if frame->skb_std is set.

Only hsr_fill_frame_info() was updated, though. hsr_dev_xmit() accepts
hsr_hdr and tx_port without checking hsr->prot_version. A PRP device can
therefore reach prp_fill_frame_info() with has_foreign_header set.

If the frame has a valid PRP RCT, prp_fill_frame_info() does:

	frame->skb_hsr = NULL;
	frame->skb_std = NULL;
	frame->skb_prp = skb;

hsr_forward_do() then skips the bypass and sends the frame through
prp_create_tagged_frame(). That function calls prp_set_lan_id() on the
caller's trailer in place. It then returns skb_clone(frame->skb_prp) with
no socket owner.

Wouldn't that change the header that should be preserved, and also lose
the PTP TX timestamp? The later patch "hsr: Assign a socket for cloned
skbs" only covers hsr_create_tagged_frame() and the foreign-header branch.

If there is no valid RCT, handle_std_frame() still takes and increments
hsr->sequence_nr. The frame is then sent unchanged by the bypass, so the
sequence number is never used.

On these PRP paths, node_src being NULL doesn't seem to be dereferenced
today. That only holds because of how prp_register_frame_out() is
currently written and because the master port is skipped.

Could has_foreign_header handling be added to prp_fill_frame_info()? The
other option would be for hsr_dev_xmit() to reject hsr_hdr and tx_port on
PRP devices.

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

[PATCH net-next v6 6/8] hsr: Assign a socket for cloned skbs

[Sebastian Andrzej Siewior]

The cloned skb does not have a socket information recorded of
the original skb. The original skb is never submitted. This means the
requested timestamp information gets lost.

Assign the socket of the original skb to the clone so the timestamp is
forwarded to the user.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/hsr/hsr_forward.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 0f708ef598ded..f2e1e7ab9c9e6 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -12,6 +12,7 @@
 #include <linux/skbuff.h>
 #include <linux/etherdevice.h>
 #include <linux/if_vlan.h>
+#include <net/sock.h>
 #include "hsr_main.h"
 #include "hsr_framereg.h"
 
@@ -348,6 +349,9 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
 	if (!skb)
 		return NULL;
 
+	if (frame->req_tx_port != HSR_PT_NONE && frame->skb_std->sk)
+		skb_set_owner_w(skb, frame->skb_std->sk);
+
 	if (port->dev->features & NETIF_F_HW_HSR_TAG_INS)
 		return skb;
 
@@ -584,6 +588,8 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
 		 */
 		if (frame->has_foreign_header && frame->skb_std) {
 			skb = skb_clone(frame->skb_std, GFP_ATOMIC);
+			if (skb && frame->skb_std->sk)
+				skb_set_owner_w(skb, frame->skb_std->sk);
 			goto inject_into_stack;
 		}
 

-- 
2.55.0

Re: [PATCH net-next v6 6/8] hsr: Assign a socket for cloned skbs

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · 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).

- [High] The patch fixes PTP TX timestamps only for HSR.
- [Medium] The commit message describes a general fix and does not
  mention that the socket is reattached only for PTP-directed frames.

--

hsr: Assign a socket for cloned skbs

This patch gives the clone the socket of the original skb in two places:
hsr_create_tagged_frame() and the has_foreign_header path of
hsr_forward_do(). Only the clone is ever transmitted, so this lets
requested TX timestamps reach the user.

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index b68aada1a1393..318e1ca438c25 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c

[ ... ]

> @@ -348,6 +349,9 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
>  	if (!skb)
>  		return NULL;
>  
> +	if (frame->req_tx_port != HSR_PT_NONE && frame->skb_std->sk)
> +		skb_set_owner_w(skb, frame->skb_std->sk);
> +

[Severity: High]
Does this fix leave PRP devices out?

hsr_dev_xmit() accepts the ETH_P_1588 inline header and sets tx_port
without checking prot_version. A PRP device therefore reaches the same
req_tx_port path:

net/hsr/hsr_device.c:hsr_dev_xmit() {
    ...
	has_header = hsr_opt->hsr_hdr;
	tx_port = hsr_opt->tx_port;
	if (tx_port != HSR_PT_SLAVE_A && tx_port != HSR_PT_SLAVE_B)
		goto drop;
    ...
}

Here is the path when hsr_hdr is not set:

- prp_fill_frame_info() calls handle_std_frame(), so frame->skb_std is
  the original skb.
- hsr_forward_do() skips the has_foreign_header branch.
- prp_register_frame_out() and prp_drop_frame() both let the frame
  through.
- The outgoing skb is then built by prp_create_tagged_frame() through
  prp_ops. This patch does not change that function:

net/hsr/hsr_forward.c:prp_create_tagged_frame() {
    ...
	} else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
		return skb_clone(frame->skb_std, GFP_ATOMIC);
	}

	skb = skb_copy_expand(frame->skb_std, skb_headroom(frame->skb_std),
			      skb_tailroom(frame->skb_std) + HSR_HLEN,
			      GFP_ATOMIC);
	return prp_fill_rct(skb, frame, port);
}

The NETIF_F_HW_HSR_TAG_INS branch returns the same socket-less clone the
commit message describes. The skb from skb_copy_expand() also has
skb->sk == NULL.

The copy path loses more than the socket. skb_copy_header() only copies
gso_size, gso_segs and gso_type from skb_shinfo(), so tx_flags and tskey
(SKBTX_HW_TSTAMP etc.) are dropped too. Setting the socket alone would
not be enough there.

When the slave driver reports the TX timestamp, __skb_tstamp_tx() stops
at:

	if (!sk)
		return;

Nothing reaches the user, so a PTP stack such as ptp4l on a PRP device
would time out waiting for TX timestamps.

The later patches in this series ("hsr: Move struct hsr_ethhdr to a
global header" and the selftest) don't touch prp_create_tagged_frame(),
so the gap is still there at the end of the series.

Should PRP get the same socket assignment plus a way to keep the
timestamp flags on the copy path? Or should hsr_dev_xmit() reject the
inline PTP header on PRP devices?

[Severity: Medium]
Does the commit message match what this condition does? It says:

    The cloned skb does not have a socket information recorded of
    the original skb. The original skb is never submitted. This means the
    requested timestamp information gets lost.

    Assign the socket of the original skb to the clone so the timestamp is
    forwarded to the user.

That reads like a fix for all cloned skbs. The code, however, only sets
the socket when frame->req_tx_port != HSR_PT_NONE.

hsr_dev_xmit() sets tx_port only for ETH_P_1588 frames that carry the
inline header magic. All other traffic keeps the default:

	enum hsr_port_type tx_port = HSR_PT_NONE;

Clones of those frames still go out with no socket, because
__skb_clone() clears sk and destructor. For sockets using
SO_TIMESTAMPING on hsr0, __skb_tstamp_tx() still returns early on those
frames.

Limiting this to the PTP path looks deliberate. Charging every clone to
sk_wmem_alloc and changing skb->hash and ooo_okay would change behavior
for normal traffic.

Could the commit message say that the change only covers PTP frames sent
to a specific port, and why?

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

[PATCH net-next v6 7/8] hsr: Move struct hsr_ethhdr to a global header

[Sebastian Andrzej Siewior]

In the offloading case the network driver needs to inspect the HSR
header to find out if the encapsulated type is a PTP packet where it
needs suppress offloading of the packet.

Move the header so it can be included the driver.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/if_hsr.h | 5 +++++
 net/hsr/hsr_main.h     | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
index 1db1bd0fa181d..bdcfe88e023fc 100644
--- a/include/linux/if_hsr.h
+++ b/include/linux/if_hsr.h
@@ -49,6 +49,11 @@ struct hsr_tag {
 
 #define HSR_HLEN	6
 
+struct hsr_ethhdr {
+	struct ethhdr	ethhdr;
+	struct hsr_tag	hsr_tag;
+} __packed;
+
 #if IS_ENABLED(CONFIG_HSR)
 extern bool is_hsr_master(struct net_device *dev);
 extern int hsr_get_version(struct net_device *dev, enum hsr_version *ver);
diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
index 53e95bae0ee27..36eb82774000c 100644
--- a/net/hsr/hsr_main.h
+++ b/net/hsr/hsr_main.h
@@ -72,11 +72,6 @@ static inline void set_hsr_tag_LSDU_size(struct hsr_tag *ht, u16 LSDU_size)
 				       0xF000) | (LSDU_size & 0x0FFF));
 }
 
-struct hsr_ethhdr {
-	struct ethhdr	ethhdr;
-	struct hsr_tag	hsr_tag;
-} __packed;
-
 struct hsr_vlan_ethhdr {
 	struct vlan_ethhdr vlanhdr;
 	struct hsr_tag	hsr_tag;

-- 
2.55.0

[PATCH net-next v6 8/8] selftests: hsr: Add test for the inline PTP header on HSR

[Sebastian Andrzej Siewior]

This test verifies the inline header which is used by HSR stack with the
ether type ETH_P_1588.
The HSR stack needs to recognize this header, strip it and send the
requested packet only on the requested port.

This test needs a HSR device and the two slave devices passed. It will
will send a sample PTP packet with this inline header, requesting one of
the ports combining with and without the HSR header. A total of four
packets is sent.
The test checks both ports devices for packets and complains if the
packet was not observed on the expected port and/ or observed on the
other port. The received content of the data is compared against the
send data sample.

Test suggested by Felix Maurer.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 tools/testing/selftests/net/hsr/.gitignore     |   1 +
 tools/testing/selftests/net/hsr/Makefile       |   5 +
 tools/testing/selftests/net/hsr/hsr_ptp.sh     | 109 ++++++
 tools/testing/selftests/net/hsr/hsr_ptp_test.c | 495 +++++++++++++++++++++++++
 4 files changed, 610 insertions(+)

diff --git a/tools/testing/selftests/net/hsr/.gitignore b/tools/testing/selftests/net/hsr/.gitignore
new file mode 100644
index 0000000000000..849eecb84c974
--- /dev/null
+++ b/tools/testing/selftests/net/hsr/.gitignore
@@ -0,0 +1 @@
+hsr_ptp_test
diff --git a/tools/testing/selftests/net/hsr/Makefile b/tools/testing/selftests/net/hsr/Makefile
index 2150e487ac7d7..5d0134ead65d7 100644
--- a/tools/testing/selftests/net/hsr/Makefile
+++ b/tools/testing/selftests/net/hsr/Makefile
@@ -2,14 +2,19 @@
 
 top_srcdir = ../../../../..
 
+CFLAGS += -Wall -Wl,--no-as-needed -O2 -g
+
 TEST_PROGS := \
 	hsr_ping.sh \
 	hsr_prp_redbox.sh \
 	hsr_redbox.sh \
 	link_faults.sh \
 	prp_ping.sh \
+	hsr_ptp.sh \
 # end of TEST_PROGS
 
 TEST_FILES += hsr_common.sh
 
+TEST_GEN_FILES := hsr_ptp_test
+
 include ../../lib.mk
diff --git a/tools/testing/selftests/net/hsr/hsr_ptp.sh b/tools/testing/selftests/net/hsr/hsr_ptp.sh
new file mode 100755
index 0000000000000..034c635916f81
--- /dev/null
+++ b/tools/testing/selftests/net/hsr/hsr_ptp.sh
@@ -0,0 +1,109 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ipv6=false
+
+source ./hsr_common.sh
+
+optstring="h4"
+usage() {
+	echo "Usage: $0"
+}
+
+while getopts "$optstring" option;do
+	case "$option" in
+	"h")
+		usage $0
+		exit 0
+		;;
+	"?")
+		usage $0
+		exit 1
+		;;
+esac
+done
+
+setup_hsr_interfaces()
+{
+	local HSRv="$1"
+
+	echo "INFO: Preparing interfaces for HSRv${HSRv}."
+# Three HSR nodes. Each node has one link to each of its neighbour, two links in total.
+
+#    ns1eth1 ----- ns2eth1
+#      hsr1         hsr2
+#    ns1eth2       ns2eth2
+#       |            |
+#    ns3eth1      ns3eth2
+#           \    /
+#            hsr3
+#
+	# Interfaces
+	ip link add ns1eth1 netns "$ns1" type veth peer name ns2eth1 netns "$ns2"
+	ip link add ns1eth2 netns "$ns1" type veth peer name ns3eth1 netns "$ns3"
+	ip link add ns3eth2 netns "$ns3" type veth peer name ns2eth2 netns "$ns2"
+
+	# HSRv0/1
+	ip -net "$ns1" link add name hsr1 type hsr slave1 ns1eth1 \
+		slave2 ns1eth2 supervision 45 version "$HSRv" proto 0
+	ip -net "$ns2" link add name hsr2 type hsr slave1 ns2eth1 \
+		slave2 ns2eth2 supervision 45 version "$HSRv" proto 0
+	ip -net "$ns3" link add name hsr3 type hsr slave1 ns3eth1 \
+		slave2 ns3eth2 supervision 45 version "$HSRv" proto 0
+
+	# IP for HSR
+	ip -net "$ns1" addr add 100.64.0.1/24 dev hsr1
+	ip -net "$ns1" addr add dead:beef:0::1/64 dev hsr1 nodad
+	ip -net "$ns2" addr add 100.64.0.2/24 dev hsr2
+	ip -net "$ns2" addr add dead:beef:0::2/64 dev hsr2 nodad
+	ip -net "$ns3" addr add 100.64.0.3/24 dev hsr3
+	ip -net "$ns3" addr add dead:beef:0::3/64 dev hsr3 nodad
+
+	ip -net "$ns1" link set address 00:11:22:00:01:01 dev ns1eth1
+	ip -net "$ns1" link set address 00:11:22:00:01:02 dev ns1eth2
+
+	ip -net "$ns2" link set address 00:11:22:00:02:01 dev ns2eth1
+	ip -net "$ns2" link set address 00:11:22:00:02:02 dev ns2eth2
+
+	ip -net "$ns3" link set address 00:11:22:00:03:01 dev ns3eth1
+	ip -net "$ns3" link set address 00:11:22:00:03:02 dev ns3eth2
+
+	# All Links up
+	ip -net "$ns1" link set ns1eth1 up
+	ip -net "$ns1" link set ns1eth2 up
+	ip -net "$ns1" link set hsr1 up
+
+	ip -net "$ns2" link set ns2eth1 up
+	ip -net "$ns2" link set ns2eth2 up
+	ip -net "$ns2" link set hsr2 up
+
+	ip -net "$ns3" link set ns3eth1 up
+	ip -net "$ns3" link set ns3eth2 up
+	ip -net "$ns3" link set hsr3 up
+}
+
+run_ptp_hdr_tests()
+{
+	echo "INFO: Running PTP-header tests."
+
+	ip netns exec "$ns1" ./hsr_ptp_test -H hsr1 -A ns1eth1 -B ns1eth2
+	ret=$?
+	stop_if_error "PTP header test failed (ns1)."
+
+	ip netns exec "$ns2" ./hsr_ptp_test -H hsr2 -A ns2eth1 -B ns2eth2
+	ret=$?
+	stop_if_error "PTP header test failed (ns2)."
+
+	ip netns exec "$ns3" ./hsr_ptp_test -H hsr3 -A ns3eth1 -B ns3eth2
+	ret=$?
+	stop_if_error "PTP header test failed (ns3)."
+}
+
+check_prerequisites
+trap cleanup_all_ns EXIT
+
+setup_ns ns1 ns2 ns3
+setup_hsr_interfaces 1
+run_ptp_hdr_tests
+
+exit $ret
diff --git a/tools/testing/selftests/net/hsr/hsr_ptp_test.c b/tools/testing/selftests/net/hsr/hsr_ptp_test.c
new file mode 100644
index 0000000000000..3c6f29ae1451b
--- /dev/null
+++ b/tools/testing/selftests/net/hsr/hsr_ptp_test.c
@@ -0,0 +1,495 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Simple test to verify the usage of the inline header used for HSR with
+ * ether type ETH_P_1588.
+ * The inline header has to be stripped, the sent packet must only appear on the
+ * specified port and the interface needs to accept a foreign HSR header and
+ * prepand its own header.
+ *
+ * Socket handling inspired by raw.c from linuxptp.
+ *
+ */
+#include <poll.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <time.h>
+#include <unistd.h>
+
+#include <linux/if_ether.h>
+#include <net/if.h>
+#include <netinet/in.h>
+#include <netpacket/packet.h>
+#include <sys/ioctl.h>
+
+#define PORT_1	1
+#define PORT_2	2
+
+#define MSEC_TO_NSEC(_x)	(_x * 1000000)
+#define USEC_PER_SEC		1000000
+#define NSEC_PER_SEC		1000000000
+
+#ifndef __packed
+#define __packed __attribute__((packed))
+#endif
+
+/* HSR Magic */
+#define HSR_INLINE_HDR 0xaf485352
+struct hsr_inline_header {
+	uint8_t tx_port;
+	uint8_t hsr_hdr;
+	uint8_t __pad0[4];
+	uint32_t magic;
+	uint8_t __pad1[2];
+	uint16_t eth_type;
+} __packed;
+
+#define MAC_LEN  6
+typedef uint8_t eth_addr[MAC_LEN];
+
+struct eth_hdr {
+	eth_addr dst;
+	eth_addr src;
+	uint16_t type;
+} __packed;
+
+struct hsr_hdr {
+	eth_addr dst;
+	eth_addr src;
+	uint16_t type;
+	uint16_t pathid_and_LSDU_size;
+	uint16_t sequence_nr;
+	uint16_t encap_type;
+} __packed;
+
+struct hsr_meta_header {
+	struct hsr_inline_header hsr_opt;
+	union {
+		struct hsr_hdr hsr_hdr;
+		struct eth_hdr eth_hdr;
+	};
+} __packed;
+
+static uint8_t ptp_packet[] = {
+	0x00, 0x12, 0x00, 0x2c, 0x00, 0x00, 0x02, 0x00,  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x88, 0x88, 0x88, 0x88,  0x88, 0x88, 0x88, 0x88, 0x00, 0x01, 0x00, 0x01,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,  0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+};
+
+static uint8_t p2p_dst_mac[MAC_LEN] = {
+	0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e
+};
+
+static uint8_t slave_mac_addr[MAC_LEN];
+static int32_t fd_slaveA = -1;
+static int32_t fd_slaveB = -1;
+static int32_t fd_hsr = -1;
+static uint16_t hsr_seq = 37;
+
+static inline void tsnorm(struct timespec *ts)
+{
+	while (ts->tv_nsec >= NSEC_PER_SEC) {
+		ts->tv_nsec -= NSEC_PER_SEC;
+		ts->tv_sec++;
+	}
+}
+
+static inline int64_t calcdiff(struct timespec t1, struct timespec t2)
+{
+	int64_t diff = USEC_PER_SEC * (long long)((int) t1.tv_sec - (int) t2.tv_sec);
+
+	diff += ((int) t1.tv_nsec - (int) t2.tv_nsec) / 1000;
+	return diff;
+}
+
+static inline int tsgreater(struct timespec *a, struct timespec *b)
+{
+	return ((a->tv_sec > b->tv_sec) ||
+		(a->tv_sec == b->tv_sec && a->tv_nsec > b->tv_nsec));
+}
+
+static int sk_interface_index(int fd, const char *name)
+{
+	struct ifreq ifreq;
+	int32_t err;
+
+	memset(&ifreq, 0, sizeof(ifreq));
+	strncpy(ifreq.ifr_name, name, sizeof(ifreq.ifr_name) - 1);
+	err = ioctl(fd, SIOCGIFINDEX, &ifreq);
+	if (err < 0) {
+		printf("ioctl SIOCGIFINDEX failed: %m\n");
+		return err;
+	}
+	return ifreq.ifr_ifindex;
+}
+
+static int open_socket(const char *name, uint8_t *local_addr,
+		       uint8_t *p2p_dst_mac)
+{
+	struct sockaddr_ll addr;
+	int32_t fd, index;
+
+	fd = socket(AF_PACKET, SOCK_RAW, 0);
+	if (fd < 0) {
+		printf("socket failed: %m\n");
+		goto err;
+	}
+	index = sk_interface_index(fd, name);
+	if (index < 0)
+		goto err;
+
+	memset(&addr, 0, sizeof(addr));
+	addr.sll_ifindex = index;
+	addr.sll_family = AF_PACKET;
+	addr.sll_protocol = htons(ETH_P_ALL);
+	if (bind(fd, (struct sockaddr *) &addr, sizeof(addr))) {
+		printf("bind failed: %m\n");
+		goto err;
+	}
+	if (setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE, name, strlen(name))) {
+		printf("setsockopt SO_BINDTODEVICE failed: %m\n");
+		goto err;
+	}
+
+	return fd;
+err:
+	if (fd >= 0)
+		close(fd);
+	return -1;
+}
+
+static int sk_interface_macaddr(const char *name)
+{
+	struct ifreq ifreq;
+	int32_t err, fd;
+
+	memset(&ifreq, 0, sizeof(ifreq));
+	strncpy(ifreq.ifr_name, name, sizeof(ifreq.ifr_name) - 1);
+
+	fd = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);
+	if (fd < 0) {
+		printf("socket failed: %m\n");
+		return -1;
+	}
+
+	err = ioctl(fd, SIOCGIFHWADDR, &ifreq);
+	if (err < 0) {
+		printf("ioctl SIOCGIFHWADDR failed: %m\n");
+		close(fd);
+		return -1;
+	}
+
+	close(fd);
+
+	memcpy(slave_mac_addr, &ifreq.ifr_hwaddr.sa_data, MAC_LEN);
+	return 0;
+}
+
+static int raw_open(const char *hsr_dev, const char *slaveA_dev, const char *slaveB_dev)
+{
+
+	if (sk_interface_macaddr(slaveA_dev))
+		goto err;
+
+	fd_slaveA = open_socket(slaveA_dev, slave_mac_addr, p2p_dst_mac);
+	if (fd_slaveA < 0)
+		goto err;
+
+	fd_slaveB = open_socket(slaveB_dev, slave_mac_addr, p2p_dst_mac);
+	if (fd_slaveB < 0)
+		goto err;
+
+	fd_hsr = open_socket(hsr_dev, slave_mac_addr, p2p_dst_mac);
+	if (fd_hsr < 0)
+		goto err;
+
+	return 0;
+
+err:
+	if (fd_slaveA >= 0)
+		close(fd_slaveA);
+	if (fd_slaveB >= 0)
+		close(fd_slaveB);
+	if (fd_hsr >= 0)
+		close(fd_hsr);
+
+	return -1;
+}
+
+static int sk_receive(int fd, void *buf, int buflen)
+{
+	struct iovec iov = { buf, buflen };
+	uint8_t control[256];
+	int32_t cnt = 0;
+	struct msghdr msg;
+
+	memset(control, 0, sizeof(control));
+	memset(&msg, 0, sizeof(msg));
+
+	msg.msg_iov = &iov;
+	msg.msg_iovlen = 1;
+	msg.msg_control = control;
+	msg.msg_controllen = sizeof(control);
+
+	cnt = recvmsg(fd, &msg, MSG_DONTWAIT);
+	if (cnt < 0)
+		return 0;
+	return cnt;
+}
+
+static int raw_recv(int fd, void *sample, int sample_len)
+{
+	struct timespec now, end;
+	struct hsr_hdr *hsr_hdr;
+	uint8_t buf[1500];
+	int32_t cnt;
+
+	/* The packet is expected to arrive within 100ms. HSR will send hello
+	 * packets so there an end time and poll() will wait until this point.
+	 */
+	clock_gettime(CLOCK_MONOTONIC, &now);
+	end = now;
+	end.tv_nsec += MSEC_TO_NSEC(100);
+	tsnorm(&end);
+again:
+
+	cnt = sk_receive(fd, buf, sizeof(buf));
+	if (cnt == 0) {
+		int64_t sleep_time;
+		struct pollfd pfd;
+		int ret;
+
+		clock_gettime(CLOCK_MONOTONIC, &now);
+		if (tsgreater(&now, &end))
+			return 0;
+
+		sleep_time = calcdiff(end, now) / 1000;
+		if (sleep_time <= 0)
+			return 0;
+
+		pfd.fd = fd;
+		pfd.events = POLLIN;
+		ret = poll(&pfd, 1, sleep_time);
+		if (ret <= 0)
+			return ret;
+		goto again;
+	}
+
+	if (cnt < sizeof(struct hsr_hdr))
+		goto again;
+
+	hsr_hdr = (void *)buf;
+	/* Embedded magic packet passed? */
+	if (hsr_hdr->type == htons(ETH_P_1588)) {
+		struct hsr_inline_header *hsr_opt = (void *)buf;
+
+		if (hsr_opt->magic == htonl(HSR_INLINE_HDR)) {
+			printf("Error: Found HSR_INLINE_HDR\n");
+			return -1;
+		}
+	}
+	/* Assuming it is *our* network and nobody but the test here sends
+	 * packets to p2p_dst_mac. Therefore any received packet needs to be
+	 * ours and every mismatch is considered as error.
+	 */
+	if (memcmp(hsr_hdr->dst, p2p_dst_mac, MAC_LEN))
+		goto again;
+
+	if (hsr_hdr->type != htons(ETH_P_HSR)) {
+		printf("Error: Unexpected ether type: 0x%x\n", htons(hsr_hdr->type));
+		return -1;
+	}
+
+	if (hsr_hdr->encap_type != htons(ETH_P_1588)) {
+		printf("Error: Unexpected encapsulated type: 0x%x\n",
+		       htons(hsr_hdr->encap_type));
+		return -1;
+	}
+
+	if (cnt < sizeof(struct hsr_hdr) + sample_len) {
+		printf("Error: Packet %d is too small for data check\n", cnt);
+		return -1;
+	}
+
+	if (!memcmp(&buf[sizeof(struct hsr_hdr)], sample, sample_len))
+		return 1;
+
+	printf("Error: Packet did not match the sample\n");
+	return -1;
+}
+
+static int recv_verify(int port, void *sample, int sample_len)
+{
+	int32_t error = 0;
+	int32_t ret;
+
+	ret = raw_recv(fd_slaveA, sample, sample_len);
+	if (ret < 0)
+		return ret;
+	if (port == PORT_1 && ret == 0) {
+		printf("Error: Missing packet on portA\n");
+		error = 1;
+	}
+	if (port == PORT_2 && ret == 1) {
+		printf("Error: Not expecting packet on portA\n");
+		error = 1;
+	}
+
+	ret = raw_recv(fd_slaveB, sample, sample_len);
+	if (ret < 0)
+		return ret;
+	if (port == PORT_2 && ret == 0) {
+		printf("Error: Missing packet on portB\n");
+		error = 1;
+	}
+	if (port == PORT_1 && ret == 1) {
+		printf("Error: Not expecting packet on portB\n");
+		error = 1;
+	}
+	return error;
+}
+
+static int32_t pkt_send(int port, bool hsr_hdr, void *data, int data_len)
+{
+	struct hsr_meta_header *hdr;
+	uint8_t packet[200];
+	ssize_t cnt;
+	size_t len;
+
+	memset(packet, 0, sizeof(packet));
+
+	len = sizeof(struct hsr_inline_header);
+	hdr = (struct hsr_meta_header *)packet;
+	memset(&hdr->hsr_opt, 0, sizeof(hdr->hsr_opt));
+	hdr->hsr_opt.magic = ntohl(HSR_INLINE_HDR);
+	hdr->hsr_opt.eth_type = htons(ETH_P_1588);
+	if (port != PORT_1 && port != PORT_2) {
+		printf("Wrong port requested\n");
+		return -1;
+	}
+	hdr->hsr_opt.tx_port = port;
+
+	if (hsr_hdr) {
+		uint16_t pathid_size;
+
+		len += sizeof(struct hsr_hdr);
+		memcpy(&packet[len], data, data_len);
+
+		hdr->hsr_opt.hsr_hdr = 1;
+
+		memcpy(&hdr->hsr_hdr.dst, p2p_dst_mac, MAC_LEN);
+		memcpy(&hdr->hsr_hdr.src, slave_mac_addr, MAC_LEN);
+		/* Flip a bit in SRC MAC addr so it does not look like hosts */
+		hdr->hsr_hdr.src[3] ^= 0x21;
+
+		hdr->hsr_hdr.type = htons(ETH_P_HSR);
+		hdr->hsr_hdr.sequence_nr = htons(hsr_seq++);
+		hdr->hsr_hdr.encap_type = htons(ETH_P_1588);
+
+		/* The resulting packet must be alteast 66 bytes */
+		if (data_len + sizeof(struct hsr_hdr) < 66)
+			data_len = 66 - sizeof(struct hsr_hdr);
+
+		pathid_size = data_len + sizeof(struct hsr_hdr) - sizeof(struct eth_hdr);
+		pathid_size |= (port - 1) << 12;
+
+		hdr->hsr_hdr.pathid_and_LSDU_size = htons(pathid_size);
+	} else {
+		len += sizeof(struct eth_hdr);
+		hdr->hsr_opt.hsr_hdr = 0;
+
+		memcpy(&hdr->eth_hdr.dst, p2p_dst_mac, MAC_LEN);
+		memcpy(&hdr->eth_hdr.src, slave_mac_addr, MAC_LEN);
+		hdr->eth_hdr.type = htons(ETH_P_1588);
+
+		memcpy(&packet[len], data, data_len);
+	}
+
+	cnt = send(fd_hsr, packet, len + data_len, 0);
+	if (cnt < 1)
+		return -1;
+
+	return cnt;
+}
+
+int main(int argc, char *argv[])
+{
+	char *slaveA = NULL, *slaveB = NULL, *hsr_dev = NULL;
+	char *msg_mode;
+	int opt;
+
+	while ((opt = getopt(argc, argv, "A:B:H:")) != -1) {
+		switch (opt) {
+		case 'A':
+			slaveA = strdup(optarg);
+			break;
+
+		case 'B':
+			slaveB = strdup(optarg);
+			break;
+
+		case 'H':
+			hsr_dev = strdup(optarg);
+			break;
+		default: /* '?' */
+			fprintf(stderr, "Usage: %s -A slaveA -B slaveB -H hsr_device\n",
+				argv[0]);
+			exit(EXIT_FAILURE);
+		}
+	}
+
+	if (!slaveA || !slaveB || !hsr_dev) {
+		fprintf(stderr, "Missing network devices\n");
+		exit(EXIT_FAILURE);
+	}
+
+	if (raw_open(hsr_dev, slaveA, slaveB) < 0)
+		return EXIT_FAILURE;
+
+	msg_mode = "PortA, no-hsr-header";
+	if (pkt_send(PORT_1, false, ptp_packet, sizeof(ptp_packet)) < 0) {
+		printf("Sending failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+	if (recv_verify(PORT_1, ptp_packet, sizeof(ptp_packet))) {
+		printf("Verify failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+
+	ptp_packet[16 + 4]++;
+	msg_mode = "PortB, no-hsr-header";
+	if (pkt_send(PORT_2, false, ptp_packet, sizeof(ptp_packet)) < 0) {
+		printf("Sending failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+	if (recv_verify(PORT_2, ptp_packet, sizeof(ptp_packet))) {
+		printf("Sending failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+
+	ptp_packet[16 + 4]++;
+	msg_mode = "PortA, hsr-header";
+	if (pkt_send(PORT_1, true, ptp_packet, sizeof(ptp_packet)) < 0) {
+		printf("Sending failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+	if (recv_verify(PORT_1, ptp_packet, sizeof(ptp_packet))) {
+		printf("Verify failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+
+	ptp_packet[16 + 4]++;
+	msg_mode = "PortB, hsr-header";
+	if (pkt_send(PORT_2, true, ptp_packet, sizeof(ptp_packet)) < 0) {
+		printf("Sending failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+	if (recv_verify(PORT_2, ptp_packet, sizeof(ptp_packet))) {
+		printf("Verify failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+
+	return EXIT_SUCCESS;
+}

-- 
2.55.0

Re: [PATCH net-next v6 8/8] selftests: hsr: Add test for the inline PTP header on HSR

[Hangbin Liu]

Hi Sebastian,

On Wed, Sep 23, 2026 at 02:14:09PM +0200, Sebastian Andrzej Siewior wrote:
> This test verifies the inline header which is used by HSR stack with the
> ether type ETH_P_1588.
> The HSR stack needs to recognize this header, strip it and send the
> requested packet only on the requested port.
> 
> This test needs a HSR device and the two slave devices passed. It will
> will send a sample PTP packet with this inline header, requesting one of
> the ports combining with and without the HSR header. A total of four
> packets is sent.
> The test checks both ports devices for packets and complains if the
> packet was not observed on the expected port and/ or observed on the
> other port. The received content of the data is compared against the
> send data sample.
> 
> Test suggested by Felix Maurer.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  tools/testing/selftests/net/hsr/.gitignore     |   1 +
>  tools/testing/selftests/net/hsr/Makefile       |   5 +
>  tools/testing/selftests/net/hsr/hsr_ptp.sh     | 109 ++++++
>  tools/testing/selftests/net/hsr/hsr_ptp_test.c | 495 +++++++++++++++++++++++++
>  4 files changed, 610 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/hsr/.gitignore b/tools/testing/selftests/net/hsr/.gitignore
> new file mode 100644
> index 0000000000000..849eecb84c974
> --- /dev/null
> +++ b/tools/testing/selftests/net/hsr/.gitignore
> @@ -0,0 +1 @@
> +hsr_ptp_test
> diff --git a/tools/testing/selftests/net/hsr/Makefile b/tools/testing/selftests/net/hsr/Makefile
> index 2150e487ac7d7..5d0134ead65d7 100644
> --- a/tools/testing/selftests/net/hsr/Makefile
> +++ b/tools/testing/selftests/net/hsr/Makefile
> @@ -2,14 +2,19 @@
>  
>  top_srcdir = ../../../../..
>  
> +CFLAGS += -Wall -Wl,--no-as-needed -O2 -g
> +
>  TEST_PROGS := \
>  	hsr_ping.sh \
>  	hsr_prp_redbox.sh \
>  	hsr_redbox.sh \
>  	link_faults.sh \
>  	prp_ping.sh \
> +	hsr_ptp.sh \
>  # end of TEST_PROGS

If there is a new version. Please sort the test with alphabet order.

>  
>  TEST_FILES += hsr_common.sh
>  
> +TEST_GEN_FILES := hsr_ptp_test
> +
>  include ../../lib.mk
> diff --git a/tools/testing/selftests/net/hsr/hsr_ptp.sh b/tools/testing/selftests/net/hsr/hsr_ptp.sh
> new file mode 100755
> index 0000000000000..034c635916f81
> --- /dev/null
> +++ b/tools/testing/selftests/net/hsr/hsr_ptp.sh
> @@ -0,0 +1,109 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ipv6=false
> +
> +source ./hsr_common.sh
> +
> +optstring="h4"
> +usage() {
> +	echo "Usage: $0"
> +}
> +
> +while getopts "$optstring" option;do
> +	case "$option" in
> +	"h")
> +		usage $0

usage "$0"

> +		exit 0
> +		;;
> +	"?")
> +		usage $0
> +		exit 1
> +		;;

Thanks
Hangbin

Re: [PATCH net-next v6 8/8] selftests: hsr: Add test for the inline PTP header on HSR

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In the hsr-header case, the test never checks that the kernel
  left the foreign HSR header alone.
- [Low] In main() of hsr_ptp_test.c, when recv_verify() fails for the
  'PortB, no-hsr-header' case, the test prints "Sending failed: %s"…
- [Low] hsr_ptp.sh sets optstring="h4", but its case statement has no "4"
  branch, so '-4' is accepted and silently ignored.
- [Low] selftests/hsr: unused parameters in open_socket()

--

selftests: hsr: Add test for the inline PTP header on HSR

This adds a selftest for the inline header that the HSR stack uses with
ether type ETH_P_1588. The test sends four sample PTP packets through the
HSR device, one per combination of target port and with or without an HSR
header. It then checks that each packet shows up only on the requested
slave port, with the inline header stripped and the payload intact.

> diff --git a/tools/testing/selftests/net/hsr/hsr_ptp.sh b/tools/testing/selftests/net/hsr/hsr_ptp.sh
> new file mode 100755
> index 0000000000000..034c635916f81
> --- /dev/null
> +++ b/tools/testing/selftests/net/hsr/hsr_ptp.sh
> @@ -0,0 +1,109 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ipv6=false
> +
> +source ./hsr_common.sh
> +
> +optstring="h4"

[Severity: Low]
This isn't a bug, but is the "4" in optstring left over from
hsr_ping.sh and prp_ping.sh? The case statement below has no "4" branch,
so running "hsr_ptp.sh -4" is accepted and silently ignored:

> +usage() {
> +	echo "Usage: $0"
> +}
> +
> +while getopts "$optstring" option;do
> +	case "$option" in
> +	"h")
> +		usage $0
> +		exit 0
> +		;;
> +	"?")
> +		usage $0
> +		exit 1
> +		;;
> +esac
> +done

The script also hard-codes ipv6=false and never calls the hsr_common.sh
ping helpers that read $ipv6, so the option has nothing to control. Could
optstring be just "h"?

[ ... ]

> diff --git a/tools/testing/selftests/net/hsr/hsr_ptp_test.c b/tools/testing/selftests/net/hsr/hsr_ptp_test.c
> new file mode 100644
> index 0000000000000..3c6f29ae1451b
> --- /dev/null
> +++ b/tools/testing/selftests/net/hsr/hsr_ptp_test.c
> @@ -0,0 +1,495 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Simple test to verify the usage of the inline header used for HSR with
> + * ether type ETH_P_1588.
> + * The inline header has to be stripped, the sent packet must only appear on the
> + * specified port and the interface needs to accept a foreign HSR header and
> + * prepand its own header.

[Severity: Low]
Does this comment match what the kernel does? It reads as if the HSR
device adds its own tag to a frame that already has a foreign HSR header.
In net/hsr/hsr_forward.c the foreign header is kept as-is instead:

net/hsr/hsr_forward.c:hsr_forward_do() {
    ...
		/* PTP TX packets may already have a HSR header which needs to
		 * be preserved
		 */
		if (frame->has_foreign_header && frame->skb_std) {
			skb = skb_clone(frame->skb_std, GFP_ATOMIC);
    ...
}

There is also a typo: "prepand" should be "prepend".

The same finding also covers the receive check in raw_recv(), quoted
further below.

[ ... ]

> +static int raw_recv(int fd, void *sample, int sample_len)
> +{

[ ... ]

> +	if (memcmp(hsr_hdr->dst, p2p_dst_mac, MAC_LEN))
> +		goto again;
> +
> +	if (hsr_hdr->type != htons(ETH_P_HSR)) {
> +		printf("Error: Unexpected ether type: 0x%x\n", htons(hsr_hdr->type));
> +		return -1;
> +	}
> +
> +	if (hsr_hdr->encap_type != htons(ETH_P_1588)) {
> +		printf("Error: Unexpected encapsulated type: 0x%x\n",
> +		       htons(hsr_hdr->encap_type));
> +		return -1;
> +	}
> +
> +	if (cnt < sizeof(struct hsr_hdr) + sample_len) {
> +		printf("Error: Packet %d is too small for data check\n", cnt);
> +		return -1;
> +	}
> +
> +	if (!memcmp(&buf[sizeof(struct hsr_hdr)], sample, sample_len))
> +		return 1;

[Severity: Low]
In the hsr-header case, would this catch the kernel changing the foreign
HSR header?

pkt_send() writes recognisable values into that header:

		hdr->hsr_hdr.src[3] ^= 0x21;
		...
		hdr->hsr_hdr.sequence_nr = htons(hsr_seq++);
		...
		pathid_size |= (port - 1) << 12;

raw_recv() only checks dst, type, encap_type and the payload after
struct hsr_hdr. It never compares the received src, sequence_nr or
pathid_and_LSDU_size with the values that were sent.

The kernel skips source MAC substitution for foreign headers in
hsr_xmit():

	if (frame->port_rcv->type == HSR_PT_MASTER && !frame->has_foreign_header) {

If the !frame->has_foreign_header part were dropped, or if the sequence
number or path id got rewritten on this path, it looks like all four
sub-tests would still pass. Would it make sense to also check those
fields when pkt_send() was called with hsr_hdr set?

[ ... ]

> +int main(int argc, char *argv[])
> +{

[ ... ]

> +	ptp_packet[16 + 4]++;
> +	msg_mode = "PortB, no-hsr-header";
> +	if (pkt_send(PORT_2, false, ptp_packet, sizeof(ptp_packet)) < 0) {
> +		printf("Sending failed: %s\n", msg_mode);
> +		return EXIT_FAILURE;
> +	}
> +	if (recv_verify(PORT_2, ptp_packet, sizeof(ptp_packet))) {
> +		printf("Sending failed: %s\n", msg_mode);

[Severity: Low]
This looks like a copy and paste slip. The other three sub-tests print
"Verify failed: %s" when recv_verify() fails, but this one prints
"Sending failed: %s". A receive failure in the "PortB, no-hsr-header"
case would then show up in the log as a send failure.

> +		return EXIT_FAILURE;
> +	}

[ ... ]

> @@ -0,0 +1,495 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Simple test to verify the usage of the inline header used for HSR with
> + * ether type ETH_P_1588.
> + * The inline header has to be stripped, the sent packet must only appear on the
> + * specified port and the interface needs to accept a foreign HSR header and
> + * prepand its own header.
> + *
> + * Socket handling inspired by raw.c from linuxptp.
> + *
> + */
> +#include <poll.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <time.h>
> +#include <unistd.h>
> +
> +#include <linux/if_ether.h>
> +#include <net/if.h>
> +#include <netinet/in.h>
> +#include <netpacket/packet.h>
> +#include <sys/ioctl.h>
> +
> +#define PORT_1	1
> +#define PORT_2	2
> +
> +#define MSEC_TO_NSEC(_x)	(_x * 1000000)
> +#define USEC_PER_SEC		1000000
> +#define NSEC_PER_SEC		1000000000
> +
> +#ifndef __packed
> +#define __packed __attribute__((packed))
> +#endif
> +
> +/* HSR Magic */
> +#define HSR_INLINE_HDR 0xaf485352
> +struct hsr_inline_header {
> +	uint8_t tx_port;
> +	uint8_t hsr_hdr;
> +	uint8_t __pad0[4];
> +	uint32_t magic;
> +	uint8_t __pad1[2];
> +	uint16_t eth_type;
> +} __packed;
> +
> +#define MAC_LEN  6
> +typedef uint8_t eth_addr[MAC_LEN];
> +
> +struct eth_hdr {
> +	eth_addr dst;
> +	eth_addr src;
> +	uint16_t type;
> +} __packed;
> +
> +struct hsr_hdr {
> +	eth_addr dst;
> +	eth_addr src;
> +	uint16_t type;
> +	uint16_t pathid_and_LSDU_size;
> +	uint16_t sequence_nr;
> +	uint16_t encap_type;
> +} __packed;
> +
> +struct hsr_meta_header {
> +	struct hsr_inline_header hsr_opt;
> +	union {
> +		struct hsr_hdr hsr_hdr;
> +		struct eth_hdr eth_hdr;
> +	};
> +} __packed;
> +
> +static uint8_t ptp_packet[] = {
> +	0x00, 0x12, 0x00, 0x2c, 0x00, 0x00, 0x02, 0x00,  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0x88, 0x88, 0x88, 0x88,  0x88, 0x88, 0x88, 0x88, 0x00, 0x01, 0x00, 0x01,
> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,  0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +};
> +
> +static uint8_t p2p_dst_mac[MAC_LEN] = {
> +	0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e
> +};
> +
> +static uint8_t slave_mac_addr[MAC_LEN];
> +static int32_t fd_slaveA = -1;
> +static int32_t fd_slaveB = -1;
> +static int32_t fd_hsr = -1;
> +static uint16_t hsr_seq = 37;
> +
> +static inline void tsnorm(struct timespec *ts)
> +{
> +	while (ts->tv_nsec >= NSEC_PER_SEC) {
> +		ts->tv_nsec -= NSEC_PER_SEC;
> +		ts->tv_sec++;
> +	}
> +}
> +
> +static inline int64_t calcdiff(struct timespec t1, struct timespec t2)
> +{
> +	int64_t diff = USEC_PER_SEC * (long long)((int) t1.tv_sec - (int) t2.tv_sec);
> +
> +	diff += ((int) t1.tv_nsec - (int) t2.tv_nsec) / 1000;
> +	return diff;
> +}
> +
> +static inline int tsgreater(struct timespec *a, struct timespec *b)
> +{
> +	return ((a->tv_sec > b->tv_sec) ||
> +		(a->tv_sec == b->tv_sec && a->tv_nsec > b->tv_nsec));
> +}
> +
> +static int sk_interface_index(int fd, const char *name)
> +{
> +	struct ifreq ifreq;
> +	int32_t err;
> +
> +	memset(&ifreq, 0, sizeof(ifreq));
> +	strncpy(ifreq.ifr_name, name, sizeof(ifreq.ifr_name) - 1);
> +	err = ioctl(fd, SIOCGIFINDEX, &ifreq);
> +	if (err < 0) {
> +		printf("ioctl SIOCGIFINDEX failed: %m\n");
> +		return err;
> +	}
> +	return ifreq.ifr_ifindex;
> +}
> +
> +static int open_socket(const char *name, uint8_t *local_addr,
> +		       uint8_t *p2p_dst_mac)
> +{
> +	struct sockaddr_ll addr;
> +	int32_t fd, index;
> +
> +	fd = socket(AF_PACKET, SOCK_RAW, 0);
> +	if (fd < 0) {
> +		printf("socket failed: %m\n");
> +		goto err;
> +	}
> +	index = sk_interface_index(fd, name);
> +	if (index < 0)
> +		goto err;
> +
> +	memset(&addr, 0, sizeof(addr));
> +	addr.sll_ifindex = index;
> +	addr.sll_family = AF_PACKET;
> +	addr.sll_protocol = htons(ETH_P_ALL);
> +	if (bind(fd, (struct sockaddr *) &addr, sizeof(addr))) {
> +		printf("bind failed: %m\n");
> +		goto err;
> +	}
> +	if (setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE, name, strlen(name))) {
> +		printf("setsockopt SO_BINDTODEVICE failed: %m\n");
> +		goto err;
> +	}
> +
> +	return fd;
> +err:
> +	if (fd >= 0)
> +		close(fd);
> +	return -1;
> +}
> +
> +static int sk_interface_macaddr(const char *name)
> +{
> +	struct ifreq ifreq;
> +	int32_t err, fd;
> +
> +	memset(&ifreq, 0, sizeof(ifreq));
> +	strncpy(ifreq.ifr_name, name, sizeof(ifreq.ifr_name) - 1);
> +
> +	fd = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);
> +	if (fd < 0) {
> +		printf("socket failed: %m\n");
> +		return -1;
> +	}
> +
> +	err = ioctl(fd, SIOCGIFHWADDR, &ifreq);
> +	if (err < 0) {
> +		printf("ioctl SIOCGIFHWADDR failed: %m\n");
> +		close(fd);
> +		return -1;
> +	}
> +
> +	close(fd);
> +
> +	memcpy(slave_mac_addr, &ifreq.ifr_hwaddr.sa_data, MAC_LEN);
> +	return 0;
> +}
> +
> +static int raw_open(const char *hsr_dev, const char *slaveA_dev, const char *slaveB_dev)
> +{
> +
> +	if (sk_interface_macaddr(slaveA_dev))
> +		goto err;
> +
> +	fd_slaveA = open_socket(slaveA_dev, slave_mac_addr, p2p_dst_mac);
> +	if (fd_slaveA < 0)
> +		goto err;
> +
> +	fd_slaveB = open_socket(slaveB_dev, slave_mac_addr, p2p_dst_mac);
> +	if (fd_slaveB < 0)
> +		goto err;
> +
> +	fd_hsr = open_socket(hsr_dev, slave_mac_addr, p2p_dst_mac);
> +	if (fd_hsr < 0)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	if (fd_slaveA >= 0)
> +		close(fd_slaveA);
> +	if (fd_slaveB >= 0)
> +		close(fd_slaveB);
> +	if (fd_hsr >= 0)
> +		close(fd_hsr);
> +
> +	return -1;
> +}
> +
> +static int sk_receive(int fd, void *buf, int buflen)
> +{
> +	struct iovec iov = { buf, buflen };
> +	uint8_t control[256];
> +	int32_t cnt = 0;
> +	struct msghdr msg;
> +
> +	memset(control, 0, sizeof(control));
> +	memset(&msg, 0, sizeof(msg));
> +
> +	msg.msg_iov = &iov;
> +	msg.msg_iovlen = 1;
> +	msg.msg_control = control;
> +	msg.msg_controllen = sizeof(control);
> +
> +	cnt = recvmsg(fd, &msg, MSG_DONTWAIT);
> +	if (cnt < 0)
> +		return 0;
> +	return cnt;
> +}
> +
> +static int raw_recv(int fd, void *sample, int sample_len)
> +{
> +	struct timespec now, end;
> +	struct hsr_hdr *hsr_hdr;
> +	uint8_t buf[1500];
> +	int32_t cnt;
> +
> +	/* The packet is expected to arrive within 100ms. HSR will send hello
> +	 * packets so there an end time and poll() will wait until this point.
> +	 */
> +	clock_gettime(CLOCK_MONOTONIC, &now);
> +	end = now;
> +	end.tv_nsec += MSEC_TO_NSEC(100);
> +	tsnorm(&end);
> +again:
> +
> +	cnt = sk_receive(fd, buf, sizeof(buf));
> +	if (cnt == 0) {
> +		int64_t sleep_time;
> +		struct pollfd pfd;
> +		int ret;
> +
> +		clock_gettime(CLOCK_MONOTONIC, &now);
> +		if (tsgreater(&now, &end))
> +			return 0;
> +
> +		sleep_time = calcdiff(end, now) / 1000;
> +		if (sleep_time <= 0)
> +			return 0;
> +
> +		pfd.fd = fd;
> +		pfd.events = POLLIN;
> +		ret = poll(&pfd, 1, sleep_time);
> +		if (ret <= 0)
> +			return ret;
> +		goto again;
> +	}
> +
> +	if (cnt < sizeof(struct hsr_hdr))
> +		goto again;
> +
> +	hsr_hdr = (void *)buf;
> +	/* Embedded magic packet passed? */
> +	if (hsr_hdr->type == htons(ETH_P_1588)) {
> +		struct hsr_inline_header *hsr_opt = (void *)buf;
> +
> +		if (hsr_opt->magic == htonl(HSR_INLINE_HDR)) {
> +			printf("Error: Found HSR_INLINE_HDR\n");
> +			return -1;
> +		}
> +	}
> +	/* Assuming it is *our* network and nobody but the test here sends
> +	 * packets to p2p_dst_mac. Therefore any received packet needs to be
> +	 * ours and every mismatch is considered as error.
> +	 */
> +	if (memcmp(hsr_hdr->dst, p2p_dst_mac, MAC_LEN))
> +		goto again;
> +
> +	if (hsr_hdr->type != htons(ETH_P_HSR)) {
> +		printf("Error: Unexpected ether type: 0x%x\n", htons(hsr_hdr->type));
> +		return -1;
> +	}
> +
> +	if (hsr_hdr->encap_type != htons(ETH_P_1588)) {
> +		printf("Error: Unexpected encapsulated type: 0x%x\n",
> +		       htons(hsr_hdr->encap_type));
> +		return -1;
> +	}
> +
> +	if (cnt < sizeof(struct hsr_hdr) + sample_len) {
> +		printf("Error: Packet %d is too small for data check\n", cnt);
> +		return -1;
> +	}
> +
> +	if (!memcmp(&buf[sizeof(struct hsr_hdr)], sample, sample_len))
> +		return 1;
> +
> +	printf("Error: Packet did not match the sample\n");
> +	return -1;
> +}
> +
> +static int recv_verify(int port, void *sample, int sample_len)
> +{
> +	int32_t error = 0;
> +	int32_t ret;
> +
> +	ret = raw_recv(fd_slaveA, sample, sample_len);
> +	if (ret < 0)
> +		return ret;
> +	if (port == PORT_1 && ret == 0) {
> +		printf("Error: Missing packet on portA\n");
> +		error = 1;
> +	}
> +	if (port == PORT_2 && ret == 1) {
> +		printf("Error: Not expecting packet on portA\n");
> +		error = 1;
> +	}
> +
> +	ret = raw_recv(fd_slaveB, sample, sample_len);
> +	if (ret < 0)
> +		return ret;
> +	if (port == PORT_2 && ret == 0) {
> +		printf("Error: Missing packet on portB\n");
> +		error = 1;
> +	}
> +	if (port == PORT_1 && ret == 1) {
> +		printf("Error: Not expecting packet on portB\n");
> +		error = 1;
> +	}
> +	return error;
> +}
> +
> +static int32_t pkt_send(int port, bool hsr_hdr, void *data, int data_len)
> +{
> +	struct hsr_meta_header *hdr;
> +	uint8_t packet[200];
> +	ssize_t cnt;
> +	size_t len;
> +
> +	memset(packet, 0, sizeof(packet));
> +
> +	len = sizeof(struct hsr_inline_header);
> +	hdr = (struct hsr_meta_header *)packet;
> +	memset(&hdr->hsr_opt, 0, sizeof(hdr->hsr_opt));
> +	hdr->hsr_opt.magic = ntohl(HSR_INLINE_HDR);
> +	hdr->hsr_opt.eth_type = htons(ETH_P_1588);
> +	if (port != PORT_1 && port != PORT_2) {
> +		printf("Wrong port requested\n");
> +		return -1;
> +	}
> +	hdr->hsr_opt.tx_port = port;
> +
> +	if (hsr_hdr) {
> +		uint16_t pathid_size;
> +
> +		len += sizeof(struct hsr_hdr);
> +		memcpy(&packet[len], data, data_len);
> +
> +		hdr->hsr_opt.hsr_hdr = 1;
> +
> +		memcpy(&hdr->hsr_hdr.dst, p2p_dst_mac, MAC_LEN);
> +		memcpy(&hdr->hsr_hdr.src, slave_mac_addr, MAC_LEN);
> +		/* Flip a bit in SRC MAC addr so it does not look like hosts */
> +		hdr->hsr_hdr.src[3] ^= 0x21;
> +
> +		hdr->hsr_hdr.type = htons(ETH_P_HSR);
> +		hdr->hsr_hdr.sequence_nr = htons(hsr_seq++);
> +		hdr->hsr_hdr.encap_type = htons(ETH_P_1588);
> +
> +		/* The resulting packet must be alteast 66 bytes */
> +		if (data_len + sizeof(struct hsr_hdr) < 66)
> +			data_len = 66 - sizeof(struct hsr_hdr);
> +
> +		pathid_size = data_len + sizeof(struct hsr_hdr) - sizeof(struct eth_hdr);
> +		pathid_size |= (port - 1) << 12;
> +
> +		hdr->hsr_hdr.pathid_and_LSDU_size = htons(pathid_size);
> +	} else {
> +		len += sizeof(struct eth_hdr);
> +		hdr->hsr_opt.hsr_hdr = 0;
> +
> +		memcpy(&hdr->eth_hdr.dst, p2p_dst_mac, MAC_LEN);
> +		memcpy(&hdr->eth_hdr.src, slave_mac_addr, MAC_LEN);
> +		hdr->eth_hdr.type = htons(ETH_P_1588);
> +
> +		memcpy(&packet[len], data, data_len);
> +	}
> +
> +	cnt = send(fd_hsr, packet, len + data_len, 0);
> +	if (cnt < 1)
> +		return -1;
> +
> +	return cnt;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	char *slaveA = NULL, *slaveB = NULL, *hsr_dev = NULL;
> +	char *msg_mode;
> +	int opt;
> +
> +	while ((opt = getopt(argc, argv, "A:B:H:")) != -1) {
> +		switch (opt) {
> +		case 'A':
> +			slaveA = strdup(optarg);
> +			break;
> +
> +		case 'B':
> +			slaveB = strdup(optarg);
> +			break;
> +
> +		case 'H':
> +			hsr_dev = strdup(optarg);
> +			break;
> +		default: /* '?' */
> +			fprintf(stderr, "Usage: %s -A slaveA -B slaveB -H hsr_device\n",
> +				argv[0]);
> +			exit(EXIT_FAILURE);
> +		}
> +	}
> +
> +	if (!slaveA || !slaveB || !hsr_dev) {
> +		fprintf(stderr, "Missing network devices\n");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	if (raw_open(hsr_dev, slaveA, slaveB) < 0)
> +		return EXIT_FAILURE;
> +
> +	msg_mode = "PortA, no-hsr-header";
> +	if (pkt_send(PORT_1, false, ptp_packet, sizeof(ptp_packet)) < 0) {
> +		printf("Sending failed: %s\n", msg_mode);
> +		return EXIT_FAILURE;
> +	}
> +	if (recv_verify(PORT_1, ptp_packet, sizeof(ptp_packet))) {
> +		printf("Verify failed: %s\n", msg_mode);
> +		return EXIT_FAILURE;
> +	}
> +
> +	ptp_packet[16 + 4]++;
> +	msg_mode = "PortB, no-hsr-header";
> +	if (pkt_send(PORT_2, false, ptp_packet, sizeof(ptp_packet)) < 0) {
> +		printf("Sending failed: %s\n", msg_mode);
> +		return EXIT_FAILURE;
> +	}
> +	if (recv_verify(PORT_2, ptp_packet, sizeof(ptp_packet))) {
> +		printf("Sending failed: %s\n", msg_mode);
> +		return EXIT_FAILURE;
> +	}
> +
> +	ptp_packet[16 + 4]++;
> +	msg_mode = "PortA, hsr-header";
> +	if (pkt_send(PORT_1, true, ptp_packet, sizeof(ptp_packet)) < 0) {
> +		printf("Sending failed: %s\n", msg_mode);
> +		return EXIT_FAILURE;
> +	}
> +	if (recv_verify(PORT_1, ptp_packet, sizeof(ptp_packet))) {
> +		printf("Verify failed: %s\n", msg_mode);
> +		return EXIT_FAILURE;
> +	}
> +
> +	ptp_packet[16 + 4]++;
> +	msg_mode = "PortB, hsr-header";
> +	if (pkt_send(PORT_2, true, ptp_packet, sizeof(ptp_packet)) < 0) {
> +		printf("Sending failed: %s\n", msg_mode);
> +		return EXIT_FAILURE;
> +	}
> +	if (recv_verify(PORT_2, ptp_packet, sizeof(ptp_packet))) {
> +		printf("Verify failed: %s\n", msg_mode);
> +		return EXIT_FAILURE;
> +	}
> +
> +	return EXIT_SUCCESS;
> +}
>

[Severity: Low]
This isn't a bug, but open_socket() never uses its local_addr or
p2p_dst_mac parameters. All three callers in raw_open() pass
slave_mac_addr and the global p2p_dst_mac, and the function body
ignores both. The parameter named p2p_dst_mac also shadows the
file-scope array of the same name, which makes it harder to tell at a
glance which one is meant.

This looks like a leftover from linuxptp's raw.c. There, as far as I
remember, the MAC arguments are used to join the multicast groups on
the socket. Was that meant to be carried over here? If not, could the
two parameters be dropped so open_socket() takes only the interface
name?

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

Re: [PATCH net-next v6 0/8] hsr: Add additional info to send/ receive skbs

[Sebastian Andrzej Siewior]

On 2026-09-23 14:14:01 [+0200], To netdev@vger.kernel.org wrote:
> I am trying to extend linuxptp to support PTP over a HSR network.
…

sashiko:
- #2
| Does this manual offset adjustment corrupt the hardware checksum offload offset?

Nothing sets ->ip_summed here. But the bot seems to be right here.

| Does this overly strict pull check drop valid small Ethernet frames?

after revisiting I tend to replace it with
		 if (!pskb_may_pull(skb, HSR_HLEN + movelen))
with
	         if (!pskb_may_pull(skb, movelen))

since the memmove() below only moves movelen bytes, so there is no need
to check for the HSR_HLEN which are in the headroom.

| Does this hardcoded offset corrupt the network_header for VLAN frames?
okay, will add the VLAN header.

- #3 
| Does this code break UAPI backward compatibility for standard userspace
| applications?

complains about the in-line header. Intended.

- #4
| Will packets arriving from a Single Attached Node (SAN) via the interlink
| port bypass the new PTP drop logic?

Yes. I guess it is okay to drop the PTP packets from SAN port, too.

| Does this check miss PTP packets encapsulated in VLANs or UDP/IP?

Yes. The specification does not mention UDP/IP for PTP packets. I will
ignore VLAN filtering for now.

- #5
| Does this code unconditionally drop standard L2 PTP traffic?

Complains about the in-line header again.

Let me fix that.

Sebastian