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

[PATCH net-next v8 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.

---
v7…v8: https://patch.msgid.link/20260928-hsr_ptp-v7-0-d55d304d9a7e@linutronix.de
- Use __u8 instead of uint8_t, add header file for in hsr_ptp.h
- Add header file to if_hsr.h for struct ethhdr
- Remove the inline keyword from hsr_ptp_test.c 

v6…v7: https://patch.msgid.link/20260923-hsr_ptp-v6-0-6ea07b3fb8a8@linutronix.de
- Change the logic in hsr_create_tagged_frame():
  - Don't update ->csum_start (the cloning takes care of this)
  - pskb_may_pull() does not need to include HSR_HLEN (it only copies
    'movelen')
  - Set network header to 'movelen' which takes VLAN into account
- Drop also PTP packets which were received on HSR_PT_INTERLINK (not
  just HSR A/B).
- Move include HSR header to include/uapi/linux/hsr_ptp.h, so it is also
  available in userland.
- selftests:
  - Add hsr_ptp.sh alphabetically ordered to the Makefile
  - Use "$0" in hsr_ptp.sh
  - Drop unnused arguments in open_socket()

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

 MAINTAINERS                                    |   1 +
 include/linux/if_hsr.h                         |   7 +
 include/uapi/linux/hsr_ptp.h                   |  20 +
 net/hsr/hsr_device.c                           |  67 +++-
 net/hsr/hsr_forward.c                          |  78 +++-
 net/hsr/hsr_forward.h                          |   3 +-
 net/hsr/hsr_framereg.h                         |   2 +
 net/hsr/hsr_main.h                             |   5 -
 net/hsr/hsr_slave.c                            |  34 +-
 tools/include/uapi/linux/hsr_ptp.h             |  20 +
 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 | 482 +++++++++++++++++++++++++
 14 files changed, 789 insertions(+), 45 deletions(-)
---
base-commit: 5a956dde5526a634dca7ccad27c051ebcc306089
change-id: 20260204-hsr_ptp-1f6380f1d35f

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

[PATCH net-next v8 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 v8 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 commit message for "hsr: Add header_ops::parse_protocol"
  (net/hsr/hsr_device.c, hsr_header_ops) is accurate but thin.

--

hsr: Add header_ops::parse_protocol

This commit sets eth_header_parse_protocol() as the parse_protocol
callback in hsr_header_ops in net/hsr/hsr_device.c. Without the callback,
dev_parse_header_protocol() returns 0 for hsr devices. As a result,
af_packet leaves skb->protocol at 0 for SOCK_RAW senders.

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

[Severity: Low]
This isn't a bug, but could the commit message also say why this is
needed?

The description is correct. In net/packet/af_packet.c,
packet_parse_headers() does:

    if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) &&
        sock->type == SOCK_RAW)
        skb->protocol = dev_parse_header_protocol(skb);

    skb_probe_transport_header(skb);

As far as I can tell, the reason for the change is the later patch in
this series, "hsr: Add a magic header for sending PTP packets". That
patch adds this check in hsr_dev_xmit():

    if (skb->protocol == htons(ETH_P_1588)) {

A SOCK_RAW sender bound to protocol 0 or ETH_P_ALL only passes that
check because of this callback. Should the message mention this
dependency?

Users of the hsr device will also see some changes. They now get the
real ethertype instead of 0 in these places:

  - skb_probe_transport_header()
  - the VLAN network header adjustment in packet_parse_headers()
  - tc/clsact protocol matching on the hsr device
  - taps on the hsr device

There is also no Fixes: tag. Could the message say whether this is a
standalone fix or a prerequisite for the PTP work, so backporters can
decide whether they need it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-hsr_ptp-v8-0-60dabc07e554%40linutronix.de

[PATCH net-next v8 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 | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 7734a521a96c2..6c0b1f467940d 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -342,33 +342,41 @@ 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 (skb->ip_summed == CHECKSUM_PARTIAL)
-		skb->csum_start += HSR_HLEN;
+	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;
 
 	movelen = ETH_HLEN;
 	if (frame->is_vlan)
 		movelen += VLAN_HLEN;
 
+	/* The entire header area needs to be linear */
+	if (!pskb_may_pull(skb, movelen))
+		goto err;
+
 	src = skb_mac_header(skb);
 	dst = skb_push(skb, HSR_HLEN);
 	memmove(dst, src, movelen);
+	skb_set_network_header(skb, HSR_HLEN + movelen);
 	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 v8 2/8] hsr: Use skb_clone() while adding the HSR header

[netdev-bot+sashiko]

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

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

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] Because of the switch from __pskb_copy() to skb_clone() +
  skb_cow_head(), skb_shared_info.meta_len (XDP metadata) of…
- [Low] The commit message does not mention that the patch changes the
  header layout of the software-tagged skb in hsr_create_tagged_frame()…
- [Low] In the software-tagging path, hsr_create_tagged_frame() now gives
  each slave an skb_clone() whose shinfo (shared, then copied by…

--

hsr: Use skb_clone() while adding the HSR header

This patch changes hsr_create_tagged_frame() to use skb_clone() and then
skb_cow_head() instead of __pskb_copy(). The goal is to keep
skb_shared_info, and with it any timestamp request, on the software
tagging path. The offloaded NETIF_F_HW_HSR_TAG_INS case now returns right
after the clone.

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 44f6d9c83180a..73bc4b8950071 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -342,33 +342,41 @@ 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);

[Severity: Low]
Each per-slave skb now keeps SKBTX_HW_TSTAMP, SKBTX_SW_TSTAMP,
SKBTX_SCHED_TSTAMP and tskey in its shinfo. However, __skb_clone() leaves
it without an owning socket:

net/core/skbuff.c:__skb_clone() {
    ...
	n->sk = NULL;
	__copy_skb_header(n, skb);
    ...
	n->destructor = NULL;
    ...
}

Will a slave driver with hardware timestamping enabled now take a TX
timestamp for this clone, only for __skb_tstamp_tx() to discard it?

net/core/skbuff.c:__skb_tstamp_tx() {
    ...
	if (!sk)
		return;
    ...
}

Before this patch, __pskb_copy() did not carry these flags over on the
software path, so the slaves never saw these requests. Some NICs have only
a single in-flight TX timestamp slot. On those, a concurrent PTP request on
the same slave might be skipped.

The later patch "hsr: Assign a socket for cloned skbs" calls
skb_set_owner_w() only when frame->req_tx_port != HSR_PT_NONE. Ordinary
traffic sent over hsr0 by a socket with SOF_TIMESTAMPING_TX_HARDWARE would
still reach this through:

hsr_dev_xmit()->hsr_forward_do()->hsr_create_tagged_frame()->
    hsr_xmit()->dev_queue_xmit()

The NETIF_F_HW_HSR_TAG_INS path already sent sk-less clones like this
before the patch. Should the timestamp request flags be cleared on clones
that have no socket attached?

>  	if (!skb)
>  		return NULL;
> -	skb_reset_mac_header(skb);
>  
> -	if (skb->ip_summed == CHECKSUM_PARTIAL)
> -		skb->csum_start += HSR_HLEN;
> +	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;
>  
>  	movelen = ETH_HLEN;
>  	if (frame->is_vlan)
>  		movelen += VLAN_HLEN;
>  
> +	/* The entire header area needs to be linear */
> +	if (!pskb_may_pull(skb, movelen))
> +		goto err;
> +
>  	src = skb_mac_header(skb);
>  	dst = skb_push(skb, HSR_HLEN);
>  	memmove(dst, src, movelen);

[Severity: Medium]
Can skb_shared_info.meta_len end up describing the wrong bytes here?

skb_clone() shares the shinfo of frame->skb_std. pskb_expand_head() then
copies both the shinfo and the head contents, so the XDP metadata length is
kept. __pskb_copy() used a fresh shinfo, so meta_len used to be 0 on this
path.

skb_metadata_end() returns skb_mac_header(). This code pushes HSR_HLEN,
moves only the ETH(+VLAN) header 6 bytes earlier and resets mac_header. As
a result, the range [mac - meta_len, mac) begins with 6 bytes that are not
metadata, followed by only the first meta_len - 6 bytes of the original
metadata. The memmove() also overwrites the last 6 bytes of the original
metadata with the start of the Ethernet header.

The kernel-doc of pskb_expand_head() says:

net/core/skbuff.c:
 *	Note: If you skb_push() the start of the buffer after reallocating the
 *	header, call skb_postpush_data_move() first to move the metadata out of
 *	the way before writing to &sk_buff->data.

__vlan_insert_inner_tag() follows that rule.

In RedBox mode, an skb with XDP metadata received on the interlink port
(generic or native XDP calling skb_metadata_set()) takes this path for
each slave:

hsr_handle_frame()->hsr_forward_skb()->handle_std_frame()->
    hsr_forward_do()->hsr_create_tagged_frame()

A tc BPF program on slave egress that reads __sk_buff->data_meta would
then see misaligned and partly overwritten metadata.

Should this use skb_postpush_data_move(), or clear the metadata with
skb_metadata_clear() after the clone?

> +	skb_set_network_header(skb, HSR_HLEN + movelen);
>  	skb_reset_mac_header(skb);
> +	skb_reset_mac_len(skb);

[Severity: Low]
The commit message only covers preserving skb_shared_info for timestamps:

    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.

Could the changelog also mention the header layout changes made here?

With the old code, __pskb_copy() only ran skb_copy_header(), so the
head-relative offsets were copied unchanged into a buffer with HSR_HLEN
more headroom. Only mac_header and csum_start were fixed up afterwards.
That left network_header pointing at the HSR tag, transport_header
HSR_HLEN bytes too early, and mac_len at 14 (18 with VLAN).

With this patch:

- skb_cow_head()->pskb_expand_head()->skb_headers_offset_update() moves
  all offsets, which replaces the manual csum_start adjustment.
- network_header now points past the HSR tag.
- mac_len becomes 20 (24 with VLAN).
- A new pskb_may_pull() drop path is added.

So the patch fixes the transport_header offset and changes what
network_header and mac_len mean for these frames. Neither change is
described in the changelog.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-hsr_ptp-v8-0-60dabc07e554%40linutronix.de

[PATCH net-next v8 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>
---
 MAINTAINERS                  |  1 +
 include/linux/if_hsr.h       |  1 +
 include/uapi/linux/hsr_ptp.h | 20 ++++++++++++++
 net/hsr/hsr_device.c         | 66 ++++++++++++++++++++++++++++++++++++--------
 net/hsr/hsr_forward.c        |  3 +-
 net/hsr/hsr_forward.h        |  3 +-
 net/hsr/hsr_slave.c          |  4 +--
 7 files changed, 82 insertions(+), 16 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 72294ddfa5b75..88a3926238d62 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12022,6 +12022,7 @@ F:	drivers/net/usb/hso.c
 HSR NETWORK PROTOCOL
 L:	netdev@vger.kernel.org
 S:	Orphan
+F:	include/uapi/linux/hsr_ptp.h
 F:	net/hsr/
 F:	tools/testing/selftests/net/hsr/
 
diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
index f4cf2dd36d193..7b6b60ef592db 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 <uapi/linux/hsr_ptp.h>
 
 struct net_device;
 
diff --git a/include/uapi/linux/hsr_ptp.h b/include/uapi/linux/hsr_ptp.h
new file mode 100644
index 0000000000000..a0b104a3df569
--- /dev/null
+++ b/include/uapi/linux/hsr_ptp.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+#ifndef __UAPI_HSR_PTP_H
+#define __UAPI_HSR_PTP_H
+
+#include <linux/types.h>
+
+#define HSR_INLINE_HDR  0xaf485352
+#define HSR_INLINE_HDR_PORT_A	1
+#define HSR_INLINE_HDR_PORT_B	2
+
+struct hsr_inline_header {
+	__u8 tx_port;
+	__u8 hsr_hdr;
+	__u8 __pad0[4];
+	__be32 magic;
+	__u8 __pad1[2];
+	__be16 eth_type;
+} __attribute__ ((packed));
+
+#endif
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 6c0b1f467940d..bf5b8c7fad306 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -773,7 +773,8 @@ static int fill_frame_info(struct hsr_frame_info *frame,
 }
 
 /* Must be called holding rcu read lock (because of the port parameter) */
-void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port)
+void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port,
+		     enum hsr_port_type tx_port, bool has_hsr_header)
 {
 	struct hsr_frame_info frame;
 
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 v8 3/8] hsr: Add a magic header for sending PTP packets

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 2

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] hsr_dev_xmit() (net/hsr/hsr_device.c) now drops every skb that
  has skb->protocol == htons(ETH_P_1588) unless the first 14 bytes form…
- [Medium] hsr_dev_xmit() treats the first 14 bytes of any skb with
  skb->protocol == ETH_P_1588 as trusted in-band control metadata.
- [Low] At this commit, the new uAPI fields tx_port and hsr_hdr
  (include/uapi/linux/hsr_ptp.h) are parsed and checked in…
- [Low] At this commit, the user-supplied hsr_hdr flag sets mac_len to 20
  without checking that the inner frame really has an HSR tag at offset…

--

hsr: Add a magic header for sending PTP packets

This patch adds a uAPI struct, hsr_inline_header. It must sit in front
of every ETH_P_1588 frame sent through the HSR master device.
hsr_dev_xmit() checks the magic, reads the requested tx port and the
"already has HSR header" flag, strips the inline header and passes both
values to hsr_forward_skb(), which will use them in a later patch.

> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index 59df647d2fd45..72347477f4f21 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,

[ ... ]

> +	skb->dev = master->dev;
> +	if (skb->protocol == htons(ETH_P_1588)) {

[Severity: Medium]
Does this drop every PTP frame that lacks the new inline header?

The gate looks only at skb->protocol, so it also catches senders that
know nothing about struct hsr_inline_header. With a normal Ethernet
header, the magic check below reads bytes 0-3 of the source MAC. These
frames then go to the drop label, and only tx_dropped is incremented.

Before this patch, hsr_forward_skb() forwarded these frames to both
slaves. Some cases that look affected:

  - AF_PACKET SOCK_DGRAM senders. eth_header() builds the header, so the
    device MAC sits where the magic is expected.

  - PTP frames that an L2 forwarder sends into hsr0, for example with
    hsr0 as a bridge port via br_add_if(). eth_type_trans() has already
    set skb->protocol to ETH_P_1588.

  - PTP sent through a VLAN upper of hsr0. hsr_dev_setup() advertises
    NETIF_F_HW_VLAN_CTAG_TX, so the tag stays in skb metadata and
    skb->protocol is still ETH_P_1588.

The commit message says:

    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.

That covers local senders that ask for TX timestamps. Does it also hold
for transit PTP traffic, or for PTP traffic that doesn't use
timestamps?

The commit message calls the header mandatory. It doesn't mention that
existing senders without the header now lose their traffic.
hsr_dev_xmit() isn't changed later in the series, so this stays in place.

> +		struct hsr_inline_header *hsr_opt;
> +		struct ethhdr *eth_hdr;
> +		unsigned int hdr_len;
> +
> +		BUILD_BUG_ON(sizeof(struct hsr_inline_header) != sizeof(struct ethhdr));
> +
> +		/* need to access the magic header */
> +		if (!pskb_may_pull(skb, sizeof(struct hsr_inline_header)))
> +			goto drop;
> +
> +		hsr_opt = (struct hsr_inline_header *)skb_mac_header(skb);
> +		if (hsr_opt->magic != htonl(HSR_INLINE_HDR))
> +			goto drop;

[Severity: Medium]
Can a remote host provide this control header?

The only check is a constant magic in the source MAC position, and the
struct maps directly onto dst MAC, src MAC and ethertype.

Some frames are received from the wire and forwarded into hsr0 by tc
mirred (ingress to egress) or by an OVS output action. They keep
skb->protocol = ETH_P_1588 from eth_type_trans(). Their first 14 bytes
are the Ethernet header that the remote sender chose.

Consider a frame with:

  - dst MAC {01 or 02, hsr_hdr, 00, 00, 00, 00}
  - src MAC {af:48:53:52, 00, 00}
  - ethertype 0x88f7

It passes this check. The bridge rejects it because the source is
multicast, but tc mirred and OVS don't. OVS and promiscuous NICs accept
the crafted destination MAC.

hsr_dev_xmit() then strips those 14 bytes. It sends the inner frame on
the port the sender picked, with the hsr_hdr flag the sender chose.

The later commit "hsr: Use the port and header information in
hsr_forward_skb()" makes has_hsr_header skip several steps:

  - hsr_get_node()
  - hsr_register_frame_in()
  - sequence number assignment
  - source MAC substitution in hsr_xmit()

As a result, the frame goes out unchanged with a spoofed source MAC,
HSR sequence number and path id.

Without this patch, forwarded frames are always re-tagged with the local
node's sequence number and source MAC. Forwarded ETH_P_HSR frames are
rejected by the mac_len < sizeof(struct hsr_ethhdr) check, because
hsr_dev_xmit() sets mac_len to 14.

Could this be used to poison the duplicate-discard tables of other ring
nodes, so that legitimate traffic from the spoofed node gets dropped?

> +
> +		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;

[Severity: Low]
Should hsr_hdr be checked against the inner frame before it is trusted
here?

With hsr_hdr set, mac_len becomes ETH_HLEN + HSR_HLEN. Nothing checks
that eth_hdr->h_proto is really an HSR ethertype.

Take a VLAN-tagged frame with an encapsulated ETH_P_HSR and hsr_hdr = 1.
It gets past this guard in hsr_fill_frame_info(), which would otherwise
reject it:

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

Then hsr_get_skb_sequence_nr() and hsr_set_path_id() read and write the
tag at mac_header + 14, which is inside the VLAN header, instead of at
mac_header + 18.

The pskb_may_pull() above keeps these accesses within the linear data.

The later commit "hsr: Use the port and header information in
hsr_forward_skb()" makes hsr_fill_frame_info() return early when
has_foreign_header is set. hsr_forward_do() then sends a clone of
skb_std without calling hsr_set_path_id(). So this affects only this
intermediate commit.

[ ... ]

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 73bc4b8950071..4afe9e1d65b7e 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -773,7 +773,8 @@ static int fill_frame_info(struct hsr_frame_info *frame,
>  }
>  
>  /* Must be called holding rcu read lock (because of the port parameter) */
> -void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port)
> +void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port,
> +		     enum hsr_port_type tx_port, bool has_hsr_header)
>  {
>  	struct hsr_frame_info frame;
>  

[Severity: Low]
At this commit, hsr_forward_skb() accepts tx_port and has_hsr_header but
doesn't use them. The body still does:

	if (fill_frame_info(&frame, skb, port) < 0)
		goto out_drop;

hsr_forward_do() still sends to both slaves. This patch already exports
struct hsr_inline_header as uAPI. At this point in the series, a frame
that asks for a single port is still sent on both ports.

The commit message says the values "will be used later". The follow-up
commit "hsr: Use the port and header information in hsr_forward_skb()"
adds frame->req_tx_port and frame->has_foreign_header. It acts on them in
fill_frame_info(), hsr_fill_frame_info(), hsr_forward_do() and
hsr_xmit(). So this only matters when bisecting onto this commit.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-hsr_ptp-v8-0-60dabc07e554%40linutronix.de

[PATCH net-next v8 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.
VLAN tagged packets are not considered.

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

diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index 5274ba6dd36e6..a5b47f016abd7 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -23,6 +23,7 @@ bool hsr_invalid_dan_ingress_frame(__be16 protocol)
 
 static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
 {
+	struct hsr_ethhdr *hsr_ethhdr;
 	struct sk_buff *skb = *pskb;
 	struct hsr_port *port;
 	struct hsr_priv *hsr;
@@ -44,8 +45,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,15 +64,28 @@ 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);
 	}
 	skb_reset_mac_len(skb);
 
+	/* PTP packets are not supposed to be forwarded via HSR as-is. The
+	 * latency introduced by forwarding renders the time information
+	 * useless. Userland needs to capture the packet on the original
+	 * interface instead of hsr.
+	 */
+	if ((!hsr->prot_version && protocol == htons(ETH_P_PRP)) ||
+	    protocol == htons(ETH_P_HSR)) {
+		hsr_ethhdr = (struct hsr_ethhdr *)skb_mac_header(skb);
+		if (hsr_ethhdr->hsr_tag.encap_proto == htons(ETH_P_1588))
+			goto finish_free_consume;
+	} else {
+		if (protocol == htons(ETH_P_1588))
+			goto finish_free_consume;
+	}
+
 	/* Only the frames received over the interlink port will assign a
 	 * sequence number and require synchronisation vs other sender.
 	 */
@@ -84,7 +97,10 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
 		hsr_forward_skb(skb, port, HSR_PT_NONE, false);
 	}
 
-finish_consume:
+	return RX_HANDLER_CONSUMED;
+
+finish_free_consume:
+	kfree_skb(skb);
 	return RX_HANDLER_CONSUMED;
 
 finish_pass:

-- 
2.55.0

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

[netdev-bot+sashiko]

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

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

- [Medium] The commit message says "Drop all received PTP packets", but
  hsr_handle_frame() (net/hsr/hsr_slave.c) only matches Layer-2 PTP.

--

hsr: Drop received PTP packets

This patch changes hsr_handle_frame() so that PTP frames received on the
HSR/PRP slave ports are dropped instead of forwarded. It matches either an
outer ethertype of ETH_P_1588 or an HSR-encapsulated protocol of
ETH_P_1588. It also merges the kfree_skb() error paths into a single
finish_free_consume label.

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

[ ... ]

> @@ -64,15 +64,28 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)

[ ... ]

>  	skb_reset_mac_len(skb);
>  
> +	/* PTP packets are not supposed to be forwarded via HSR as-is. The
> +	 * latency introduced by forwarding renders the time information
> +	 * useless. Userland needs to capture the packet on the original
> +	 * interface instead of hsr.
> +	 */
> +	if ((!hsr->prot_version && protocol == htons(ETH_P_PRP)) ||
> +	    protocol == htons(ETH_P_HSR)) {
> +		hsr_ethhdr = (struct hsr_ethhdr *)skb_mac_header(skb);
> +		if (hsr_ethhdr->hsr_tag.encap_proto == htons(ETH_P_1588))
> +			goto finish_free_consume;
> +	} else {
> +		if (protocol == htons(ETH_P_1588))
> +			goto finish_free_consume;
> +	}
> +

[Severity: Medium]
The commit message says:

    Drop all received PTP packets.

Do these checks catch PTP carried over UDP? Both branches only match
ETH_P_1588, which is the Layer 2 mapping.

PTP over UDP/IPv4 or IPv6 (ports 319/320) has ETH_P_IP or ETH_P_IPV6 in
two places. For untagged, PRP, interlink or HW-tag-removed frames it is in
h_proto. For HSR-tagged frames it is in hsr_tag.encap_proto. Either way,
the frame takes neither drop branch.

These frames then go into hsr_forward_skb(). There they are de-duplicated,
delivered to the hsr master and forwarded around the ring. That causes the
same unaccounted forwarding latency and loss of per-port timestamps that
the commit message describes. linuxptp uses network_transport UDPv4 by
default, so this is a common configuration.

UDP PTP behaves the same as it did before this patch, and the IEC 62439-3
PTP profile for HSR/PRP uses the L2 mapping. Still, the commit message
names only VLAN-tagged packets as out of scope. The new comment also
states the policy for PTP in general.

Should the commit message and the comment say that only L2 (ETH_P_1588)
PTP is dropped? Or should the UDP case be handled as well?

>  	/* Only the frames received over the interlink port will assign a
>  	 * sequence number and require synchronisation vs other sender.
>  	 */

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-hsr_ptp-v8-0-60dabc07e554%40linutronix.de

[PATCH net-next v8 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 bf5b8c7fad306..2ce31b1da7b35 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -428,7 +428,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
@@ -549,11 +549,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;
@@ -572,6 +573,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.
 		 */
@@ -599,6 +611,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;
@@ -663,6 +676,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)) {
@@ -704,7 +724,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;
@@ -727,10 +748,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;
@@ -779,10 +805,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 v8 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: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] hsr_forward_do()'s new per-port delivery (`frame->req_tx_port`
  filter, line 577) and pre-tagged bypass (line 582) ignore HSR hardware…
- [Low] The new foreign-header branch in hsr_forward_do() sends
  `skb_clone(frame->skb_std, GFP_ATOMIC)` without moving socket…

--

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

This patch makes hsr_forward_skb() use the tx port and the "has HSR header"
information passed in by the upper layer. Frames can be limited to a single
slave port. Frames that already carry an HSR header are cloned and sent
unchanged, without sequence number assignment, source address substitution
or a node_db lookup.

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 4afe9e1d65b7e..562802ddff003 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c

[ ... ]

> @@ -572,6 +573,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;

[Severity: Medium]
Does "deliver it only on the requested port" still hold when the slaves
have HSR hardware offload enabled?

This check selects the port in software only. Nothing tells a slave with
NETIF_F_HW_HSR_DUP not to duplicate the frame. DSA switches (ksz9477,
xrs700x, and switches using dsa_port_simple_hsr_join()) add every HSR port
to the TX mask in the tagger:

net/dsa/tag.h:dsa_xmit_port_mask() {
    ...
	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);
    ...
}

icssg in HSR offload mode marks every skb as undirected and asks the
firmware to insert a tag:

drivers/net/ethernet/ti/icssg/icssg_common.c:icssg_ndo_start_xmit() {
    ...
	if (prueth->is_hsr_offload_mode &&
	    (ndev->features & NETIF_F_HW_HSR_DUP))
		dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;

	if (prueth->is_hsr_offload_mode &&
	    (ndev->features & NETIF_F_HW_HSR_TAG_INS))
		epib[1] |= PRUETH_UNDIRECTED_PKT_TAG_INS;
    ...
}

So a PTP frame sent with tx_port set to SLAVE_A goes through
hsr_dev_xmit()->hsr_forward_skb()->hsr_forward_do()->hsr_xmit(). On these
setups, wouldn't it still leave on SLAVE_B as well?

The has_foreign_header branch below has a related issue. It skips
create_tagged_frame(), which is where NETIF_F_HW_HSR_TAG_INS is normally
taken into account. It then passes an already tagged frame to hardware
(xrs700x, icssg) that may insert its own HSR tag. Can that produce a
double tagged frame?

The later commit "hsr: Move struct hsr_ethhdr to a global header" says
that "in the offloading case the network driver needs to inspect the HSR
header ... where it needs suppress offloading of the packet". No driver or
DSA tagger change in the series seems to do that.

Should hsr_forward_do() reject these frames on offloading slaves, or work
around the offload, until the drivers handle it?

> +		/* 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: Low]
Does this clone lose the socket ownership of the original skb?

__skb_clone() sets n->sk and n->destructor to NULL on the clone. After
that, hsr_forward_skb() frees the socket-owned original:

	kfree_skb(frame.skb_std);

That runs sock_wfree() while the clone may still be queued on the slave.
The socket's wmem accounting then stops covering the in-flight packet.
Also, skb_tstamp_tx() sees a NULL sk, so the PTP TX timestamp never reaches
the sender.

A later commit in the series, "hsr: Assign a socket for cloned skbs",
seems to fix this. It adds the following to this branch, and does the same
for the req_tx_port case in hsr_create_tagged_frame():

	if (skb && frame->skb_std->sk)
		skb_set_owner_w(skb, frame->skb_std->sk);

The old path through hsr_create_tagged_frame() also cloned without
restoring the socket, so this patch doesn't make things worse.

Could that fix be folded into this patch?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-hsr_ptp-v8-0-60dabc07e554%40linutronix.de

[PATCH net-next v8 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 2ce31b1da7b35..eb696e33bd739 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;
 
@@ -581,6 +585,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 v8 6/8] hsr: Assign a socket for cloned skbs

[netdev-bot+sashiko]

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

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

- [High] The fix covers only the HSR implementation of the shared
  `create_tagged_frame` callback.
- [Medium] The patch calls `skb_set_owner_w(clone, frame->skb_std->sk)`
  in both `hsr_create_tagged_frame()` and `hsr_forward_do()`.
- [Low] The commit message says, without qualification, that the cloned
  skb loses the original's socket and that the patch assigns the…

--

hsr: Assign a socket for cloned skbs

This commit gives the cloned skb that is actually transmitted the socket
of the original skb, so that requested TX timestamps reach the user. It
does this in hsr_create_tagged_frame() for frames that request a TX port,
and in the foreign header path of hsr_forward_do().

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

[Severity: Low]
Could the commit message say that this only covers port-directed PTP
frames?

hsr_create_tagged_frame() only assigns the socket when
frame->req_tx_port != HSR_PT_NONE. hsr_dev_xmit() only moves tx_port away
from HSR_PT_NONE for ETH_P_1588 frames that carry the inline HSR header.
The has_foreign_header path in hsr_forward_do() also needs a port.

Other traffic that asks for TX timestamps still reaches the slaves as
clones with skb->sk == NULL. One example is a UDP socket using
SO_TIMESTAMPING over hsr0. For those clones, __skb_tstamp_tx() returns
early:

	if (!sk)
		return;

The restriction looks deliberate, since it avoids one timestamp per slave
and double charging. The message, though, reads as if it applies to all
cloned skbs.

The message also leaves out the side effects of skb_set_owner_w(). The
clone is now charged to the sender's sk_wmem_alloc, and its skb->hash and
ooo_okay now come from the socket.

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 562802ddff003..46f9bd5039821 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);
> +
>  	if (port->dev->features & NETIF_F_HW_HSR_TAG_INS)
>  		return skb;
>  

[Severity: High]
Does the timestamp still get lost on PRP devices?

HSR and PRP masters share hsr_dev_xmit(). Both accept the ETH_P_1588
inline header, and tx_port is set without checking prot_version. On a
PRP master the frame takes this path:

hsr_dev_xmit()
  hsr_forward_skb()
    prp_fill_frame_info()
      handle_std_frame()      /* sets frame->skb_std */
    hsr_forward_do()
      hsr->proto_ops->create_tagged_frame()  /* prp_create_tagged_frame */

prp_create_tagged_frame() is not changed by this patch:

	} 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);

With NETIF_F_HW_HSR_TAG_INS, the clone keeps the timestamp request flags
in the shared skb_shinfo. Its skb->sk is NULL, though, so
__skb_tstamp_tx() returns early.

In the skb_copy_expand() case, skb_copy_header() copies only the gso
fields from shinfo. Both tx_flags and the socket are lost.

hsr_forward_skb() just frees the original skb_std. A PTP application
sending through a PRP master with the inline header would therefore still
not get its TX timestamp.

None of the later patches in this series seem to touch
prp_create_tagged_frame(). Should it get the same change? Or should
hsr_dev_xmit() reject the inline header on PRP?

[ ... ]

> @@ -581,6 +585,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;
>  		}

[Severity: Medium]
Is it safe to call skb_set_owner_w() here, and in
hsr_create_tagged_frame(), without checking how the original skb holds
skb_std->sk?

skb_set_owner_w() adds the clone's truesize to sk->sk_wmem_alloc. It does
not take an sk_refcnt reference. That is fine when the original skb is
owned through sock_wfree, which is the usual AF_PACKET case, because that
charge keeps the socket alive. For comparison, skb_clone_sk() calls
refcount_inc_not_zero() on sk_refcnt.

An egress skb can carry a socket that nothing pins, though.
bpf_sk_assign() at TC ingress does:

	skb->sk = sk;
	skb->destructor = sock_pfree;

It takes no reference for SOCK_RCU_FREE sockets. The tcx ingress redirect
path and __bpf_tx_skb() do not orphan the skb before dev_queue_xmit().
hsr_dev_xmit() accepts any ETH_P_1588 skb with the inline magic,
whatever its destructor.

Suppose that socket is being closed at the same time and sk_wmem_alloc
has already reached zero. Could skb_set_owner_w() then add to a zero
refcount? After the clone leaves the RCU section through the slave qdisc
or driver, could its sock_wfree() touch the freed socket?

Triggering this needs CAP_BPF or CAP_NET_ADMIN and a narrow close race,
and the full race has not been shown in practice.

Would it make sense to transfer ownership only when
frame->skb_std->destructor == sock_wfree, like the copy_dtor check in
UDP GSO? ip_frag_next() calls skb_set_owner_w(skb2, skb->sk)
unconditionally, so similar exposure exists elsewhere in the kernel.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-hsr_ptp-v8-0-60dabc07e554%40linutronix.de

[PATCH net-next v8 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 | 6 ++++++
 net/hsr/hsr_main.h     | 5 -----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
index 7b6b60ef592db..f81323f10eecc 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/if_ether.h>
 #include <uapi/linux/hsr_ptp.h>
 
 struct net_device;
@@ -39,6 +40,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 v8 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/include/uapi/linux/hsr_ptp.h             |  20 +
 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 | 482 +++++++++++++++++++++++++
 5 files changed, 617 insertions(+)

diff --git a/tools/include/uapi/linux/hsr_ptp.h b/tools/include/uapi/linux/hsr_ptp.h
new file mode 100644
index 0000000000000..a0b104a3df569
--- /dev/null
+++ b/tools/include/uapi/linux/hsr_ptp.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+#ifndef __UAPI_HSR_PTP_H
+#define __UAPI_HSR_PTP_H
+
+#include <linux/types.h>
+
+#define HSR_INLINE_HDR  0xaf485352
+#define HSR_INLINE_HDR_PORT_A	1
+#define HSR_INLINE_HDR_PORT_B	2
+
+struct hsr_inline_header {
+	__u8 tx_port;
+	__u8 hsr_hdr;
+	__u8 __pad0[4];
+	__be32 magic;
+	__u8 __pad1[2];
+	__be16 eth_type;
+} __attribute__ ((packed));
+
+#endif
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..512ccf61b6652 100644
--- a/tools/testing/selftests/net/hsr/Makefile
+++ b/tools/testing/selftests/net/hsr/Makefile
@@ -2,9 +2,12 @@
 
 top_srcdir = ../../../../..
 
+CFLAGS += -Wall -Wl,--no-as-needed -O2 -g $(TOOLS_INCLUDES)
+
 TEST_PROGS := \
 	hsr_ping.sh \
 	hsr_prp_redbox.sh \
+	hsr_ptp.sh \
 	hsr_redbox.sh \
 	link_faults.sh \
 	prp_ping.sh \
@@ -12,4 +15,6 @@ 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..23379b1ce7a56
--- /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="h"
+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..5b69d4594768b
--- /dev/null
+++ b/tools/testing/selftests/net/hsr/hsr_ptp_test.c
@@ -0,0 +1,482 @@
+// 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
+ * prepend 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/types.h>
+#include <linux/hsr_ptp.h>
+#include <linux/if_ether.h>
+#include <net/if.h>
+#include <netinet/in.h>
+#include <netpacket/packet.h>
+#include <sys/ioctl.h>
+
+#define MSEC_TO_NSEC(_x)	(_x * 1000000)
+#define USEC_PER_SEC		1000000
+#define NSEC_PER_SEC		1000000000
+
+#ifndef __packed
+#define __packed __attribute__((packed))
+#endif
+
+#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 void tsnorm(struct timespec *ts)
+{
+	while (ts->tv_nsec >= NSEC_PER_SEC) {
+		ts->tv_nsec -= NSEC_PER_SEC;
+		ts->tv_sec++;
+	}
+}
+
+static 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 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)
+{
+	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);
+	if (fd_slaveA < 0)
+		goto err;
+
+	fd_slaveB = open_socket(slaveB_dev);
+	if (fd_slaveB < 0)
+		goto err;
+
+	fd_hsr = open_socket(hsr_dev);
+	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", ntohs(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 == HSR_INLINE_HDR_PORT_A && ret == 0) {
+		printf("Error: Missing packet on portA\n");
+		error = 1;
+	}
+	if (port == HSR_INLINE_HDR_PORT_B && 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 == HSR_INLINE_HDR_PORT_B && ret == 0) {
+		printf("Error: Missing packet on portB\n");
+		error = 1;
+	}
+	if (port == HSR_INLINE_HDR_PORT_A && 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 = htonl(HSR_INLINE_HDR);
+	hdr->hsr_opt.eth_type = htons(ETH_P_1588);
+	if (port != HSR_INLINE_HDR_PORT_A && port != HSR_INLINE_HDR_PORT_B) {
+		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(HSR_INLINE_HDR_PORT_A, false, ptp_packet, sizeof(ptp_packet)) < 0) {
+		printf("Sending failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+	if (recv_verify(HSR_INLINE_HDR_PORT_A, 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(HSR_INLINE_HDR_PORT_B, false, ptp_packet, sizeof(ptp_packet)) < 0) {
+		printf("Sending failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+	if (recv_verify(HSR_INLINE_HDR_PORT_B, ptp_packet, sizeof(ptp_packet))) {
+		printf("Verify failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+
+	ptp_packet[16 + 4]++;
+	msg_mode = "PortA, hsr-header";
+	if (pkt_send(HSR_INLINE_HDR_PORT_A, true, ptp_packet, sizeof(ptp_packet)) < 0) {
+		printf("Sending failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+	if (recv_verify(HSR_INLINE_HDR_PORT_A, 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(HSR_INLINE_HDR_PORT_B, true, ptp_packet, sizeof(ptp_packet)) < 0) {
+		printf("Sending failed: %s\n", msg_mode);
+		return EXIT_FAILURE;
+	}
+	if (recv_verify(HSR_INLINE_HDR_PORT_B, 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 v8 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] The 'PortA, hsr-header' and 'PortB, hsr-header' cases of
  hsr_ptp_test do not check that the kernel preserves a supplied…

--

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

This adds a selftest for the inline header the HSR stack uses with ether
type ETH_P_1588. The test sends four sample PTP packets through the HSR
device: one to each port, each sent with and without an HSR header. It
checks that each packet shows up only on the requested slave port and
that the received payload matches what was sent.

> 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..5b69d4594768b
> --- /dev/null
> +++ b/tools/testing/selftests/net/hsr/hsr_ptp_test.c
> @@ -0,0 +1,482 @@
> +// 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
> + * prepend its own header.

[ ... ]

> +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", ntohs(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]
Do the "PortA, hsr-header" and "PortB, hsr-header" cases check that the
kernel keeps the HSR header supplied by the caller?

pkt_send() fills the foreign header with values that would identify it:

	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. It never
compares src, sequence_nr or pathid_and_LSDU_size with the values that
were sent.

On the kernel side, you can only see the foreign header handling on the
wire through these two paths in net/hsr/hsr_forward.c:

hsr_xmit() {
	if (frame->port_rcv->type == HSR_PT_MASTER && !frame->has_foreign_header) {
		hsr_addr_subst_dest(frame->node_src, skb, port);
		...
		ether_addr_copy(eth_hdr(skb)->h_source, port->dev->dev_addr);
	}
	...
}

hsr_forward_do() {
	...
		if (frame->has_foreign_header && frame->skb_std) {
			skb = skb_clone(frame->skb_std, GFP_ATOMIC);
	...
}

Suppose the !frame->has_foreign_header check in hsr_xmit() were dropped.
The source MAC would then be rewritten to the slave's address, and
hsr_addr_subst_dest() returns early for the multicast 01:80:c2:00:00:0e
destination. Would every check in raw_recv() still pass in that case?

Could raw_recv() also compare the source MAC, sequence_nr and path id
against the values pkt_send() used for the hsr-header cases?

Also, the file header comment says the interface needs to "accept a
foreign HSR header and prepend its own header". Is that accurate?
hsr_forward_do() seems to keep the supplied header ("PTP TX packets may
already have a HSR header which needs to be preserved") and does not
prepend a new one.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-hsr_ptp-v8-0-60dabc07e554%40linutronix.de

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

[Daniel Machon]

Sebastian,

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


Do any drivers need to be updated as part of this series?

A number of drivers offload HSR duplication using NETIF_F_HW_HSR_DUP. For a
directed PTP frame the HSR layer only sends it to the requested tx_port, but
that is undone further down the stack (e.g. dsa_xmit_port_mask() adds the HSR
partner port back, and icssg sends it as undirected), so e.g. a SYNC frame sent
to port B would also leave on port A. Or am I missing something?

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

We support PTP over HSR downstream on lan969x and lan9645x (with a different
uAPI). Maybe I can be of assistance testing this. Let me know.

> 
> ---
> v7…v8: https://patch.msgid.link/20260928-hsr_ptp-v7-0-d55d304d9a7e@linutronix.de
> - Use __u8 instead of uint8_t, add header file for in hsr_ptp.h
> - Add header file to if_hsr.h for struct ethhdr
> - Remove the inline keyword from hsr_ptp_test.c
> 
> v6…v7: https://patch.msgid.link/20260923-hsr_ptp-v6-0-6ea07b3fb8a8@linutronix.de
> - Change the logic in hsr_create_tagged_frame():
>   - Don't update ->csum_start (the cloning takes care of this)
>   - pskb_may_pull() does not need to include HSR_HLEN (it only copies
>     'movelen')
>   - Set network header to 'movelen' which takes VLAN into account
> - Drop also PTP packets which were received on HSR_PT_INTERLINK (not
>   just HSR A/B).
> - Move include HSR header to include/uapi/linux/hsr_ptp.h, so it is also
>   available in userland.
> - selftests:
>   - Add hsr_ptp.sh alphabetically ordered to the Makefile
>   - Use "$0" in hsr_ptp.sh
>   - Drop unnused arguments in open_socket()
> 
> 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
> 
>  MAINTAINERS                                    |   1 +
>  include/linux/if_hsr.h                         |   7 +
>  include/uapi/linux/hsr_ptp.h                   |  20 +
>  net/hsr/hsr_device.c                           |  67 +++-
>  net/hsr/hsr_forward.c                          |  78 +++-
>  net/hsr/hsr_forward.h                          |   3 +-
>  net/hsr/hsr_framereg.h                         |   2 +
>  net/hsr/hsr_main.h                             |   5 -
>  net/hsr/hsr_slave.c                            |  34 +-
>  tools/include/uapi/linux/hsr_ptp.h             |  20 +
>  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 | 482 +++++++++++++++++++++++++
>  14 files changed, 789 insertions(+), 45 deletions(-)
> ---
> base-commit: 5a956dde5526a634dca7ccad27c051ebcc306089
> change-id: 20260204-hsr_ptp-1f6380f1d35f
> 
> Best regards,
> --
> Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 

/Daniel