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