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

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

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

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

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

--

hsr: Assign a socket for cloned skbs

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

[ ... ]

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

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

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

Here is the path when hsr_hdr is not set:

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

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

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

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

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

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

	if (!sk)
		return;

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

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

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

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

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

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

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

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

	enum hsr_port_type tx_port = HSR_PT_NONE;

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

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

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

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