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

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.

[ ... ]

[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?

[ ... ]

[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?

[ ... ]

[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