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

[netdev-bot+sashiko@kernel.org]

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] The per-port TX request (frame->req_tx_port) added by this
  patch does not take effect on slave devices with NETIF_F_HW_HSR_DUP.
- [Low] On PRP devices the has_foreign_header flag is only half honoured.
- [Low] Baseline decision: discard.

--

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

This commit makes hsr_forward_skb() use the tx port and header
information passed down from the upper layer. A frame with a requested
port is sent only on that port.

A frame that already carries an HSR header is sent as a clone of the
original skb. It is not given a sequence number, its h_source is not
replaced and no source node lookup is done.

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 14d793f00344f..a76bd02fe25f1 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c

[ ... ]

> @@ -570,6 +571,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 the per-port request still work when the slave devices have
NETIF_F_HW_HSR_DUP set?

hsr_forward_do() passes the frame only to the requested slave netdev.
Nothing tells the lower driver not to duplicate it.

For DSA, dsa_xmit_port_mask() in net/dsa/tag.h adds every HSR ring port
to the egress mask:

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

This covers ksz9477, xrs700x and the switches that use
dsa_port_simple_hsr_join().

For TI ICSSG, icssg_ndo_start_xmit() replaces the directed port:

    if (prueth->is_hsr_offload_mode &&
        (ndev->features & NETIF_F_HW_HSR_DUP))
        dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;

So a PTP frame requested for SLAVE_A, such as a per-link peer delay
message, would also go out on SLAVE_B. That doesn't match "If the port
set, deliver it only on the requested port" in the commit message.

The later commit "hsr: Move struct hsr_ethhdr to a global header" says
offloading drivers need to inspect PTP packets and suppress offloading.
No driver is changed in the series, though.

Should the directed case be handled here, or in the drivers, before this
is enabled?

> +		/* 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]
Should the clone take over the socket ownership here?

__skb_clone() leaves sk as NULL on the clone. The slave driver takes the
TX hardware timestamp on the clone, so the timestamp can't be delivered
to the sending PTP socket. The original skb, which owns the socket, is
freed in hsr_forward_skb() without being sent.

The next commit in the series, "hsr: Assign a socket for cloned skbs",
fixes this by adding skb_set_owner_w() to this branch. So this only
affects this intermediate commit. TX timestamps through hsr were not
delivered before the series either.

> +
>  		/* Don't send frame over port where it has been sent before.

[ ... ]

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

[Severity: Low]
This early return is only added to hsr_fill_frame_info(), the hsr_ops
callback. What happens on a PRP device, where proto_ops->fill_frame_info
is prp_fill_frame_info()?

hsr_dev_xmit() accepts hsr_hdr for any prot_version. fill_frame_info()
then sets has_foreign_header and skips hsr_get_node(), and
prp_fill_frame_info() runs its normal logic.

If the frame has no valid RCT, handle_std_frame() still uses up a
sequence number:

    frame->sequence_nr = hsr->sequence_nr;
    WRITE_ONCE(hsr->sequence_nr, hsr->sequence_nr + 1);

The commit message says no sequence number is assigned when a header is
already present.

If the frame has a valid RCT, skb_prp is set and skb_std stays NULL. The
"has_foreign_header && frame->skb_std" bypass in hsr_forward_do() is then
skipped. The frame goes through prp_create_tagged_frame(), which calls
prp_set_lan_id() and rewrites the trailer the user supplied.

At the end of the series, that skb_prp clone also gets no socket owner,
so its TX timestamp is lost.

hsr_dev_xmit() also always adds HSR_HLEN when hsr_hdr is set:

    if (has_header)
        hdr_len = ETH_HLEN + HSR_HLEN;

PRP uses a trailer, not a front tag. On PRP this places network_header
and mac_len 6 bytes too far into the frame.

The later commit "hsr: Assign a socket for cloned skbs" says "This does
not consider PRP part of the stack". Would it be better to reject the
inline header on PRP devices, or to handle has_foreign_header in
prp_fill_frame_info() as well?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-hsr_ptp-v9-0-2d20a2a0f816%40linutronix.de