Re: [PATCH net v8 1/4] net: hsr: keep GRO disabled on HSR/PRP ports

[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: 2 · 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] hsr_portdev_setup() (net/hsr/hsr_slave.c:210) calls
  hsr_portdev_role_set() on every non-master port without checking…
- [Medium] hsr_portdev_role_set() calls netdev_change_features() with
  dev->hsr_port set.

--

net: hsr: keep GRO disabled on HSR/PRP ports

This patch adds an hsr_port role bit to struct net_device. The bit is set
on HSR/PRP slave A/B and interlink ports. While it is set,
__netdev_update_features() drops software GRO and configurable GRO_HW
from both wanted_features and the computed features, and the attach
fails if software GRO stays active.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index df1a840b03343..2916b17f4169f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -11269,6 +11269,21 @@ int __netdev_update_features(struct net_device *dev)
>  
>  	features = netdev_get_wanted_features(dev);
>  
> +	/* A direct HSR/PRP port needs per-frame metadata. Drop software
> +	 * GRO and configurable GRO_HW from the request as well as from the
> +	 * result, as the core does for upper-disabled features: drivers
> +	 * such as veth derive state from wanted_features and need the
> +	 * requested and the active bit to agree. Fixed-on or
> +	 * driver-required GRO_HW is left to the driver.
> +	 */
> +	if (dev->hsr_port) {
> +		netdev_features_t gro = NETIF_F_GRO |
> +					(dev->hw_features & NETIF_F_GRO_HW);
> +
> +		dev->wanted_features &= ~gro;
> +		features &= ~gro;
> +	}
> +
>  	if (dev->netdev_ops->ndo_fix_features)
>  		features = dev->netdev_ops->ndo_fix_features(dev, features);
>  
> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> index a546f70f9cc8b..bc340ae0b52e1 100644
> --- a/net/hsr/hsr_slave.c
> +++ b/net/hsr/hsr_slave.c

[ ... ]

> +static void hsr_portdev_role_clear(struct net_device *dev)
> +{
> +	netdev_lock_ops(dev);
> +	dev->hsr_port = false;
> +	/* Recompute features without the role. GRO was dropped from
> +	 * the request while the role was set and stays off; the user
> +	 * can turn it back on with ethtool. A dying device just drops
> +	 * the role, a netns move stays registered and is recomputed.
> +	 */
> +	if (dev->reg_state == NETREG_REGISTERED)
> +		netdev_change_features(dev);
> +	netdev_unlock_ops(dev);
> +}

[ ... ]

> @@ -169,6 +199,22 @@ static int hsr_portdev_setup(struct hsr_priv *hsr, struct net_device *dev,
>  	if (res)
>  		goto fail_upper_dev_link;
>  
> +	/* Enable the member role and recompute before the RX handler
> +	 * is published, so that software GRO on this device is off
> +	 * before frames can arrive. This covers the direct member
> +	 * only: a stacked member receives from a lower device that
> +	 * may still run GRO and can deliver merged frames as GSO skbs.
> +	 * A still-active software GRO rejects the port, a remaining
> +	 * GRO_HW alone does not.
> +	 */
> +	hsr_portdev_role_set(dev);
> +	if (dev->features & NETIF_F_GRO) {

[Severity: Medium]
The role is set here for every non-master port. Nothing checks
prot_version, port->type or hsr->fwd_offloaded first. Is GRO actually a
problem on all of these ports?

On PRP slave A/B, frames that carry an RCT trailer do not appear to be
merged. inet_gro_receive() flushes when the IP tot_len does not match
skb_gro_len():

net/ipv4/af_inet.c:inet_gro_receive() {
    ...
	flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (ntohl(*(__be32 *)&iph->id) & ~IP_DF));
    ...
}

prp_is_lan_dup() also drops A/B forwarding. Untagged LAN traffic goes to
the master or interlink untagged, the same way a bridge forwards GRO
skbs.

Some slaves have HW forwarding offload (NETIF_F_HW_HSR_FWD, for example
icssg, ksz9477 and xrs700x). For those, hsr_drop_frame() does:

net/hsr/hsr_forward.c:hsr_drop_frame() {
	if (port->dev->features & NETIF_F_HW_HSR_FWD)
		return prp_is_lan_dup(frame->port_rcv->type, port);
    ...
}

so merged skbs are never re-tagged towards the other slave.

There is also no GRO offload for ETH_P_HSR or ETH_P_PRP. Outside
net/hsr, the only references are in flow_dissector, so tagged frames
cannot be merged. The per-frame metadata problem seems to exist only
where untagged frames get tagged: the interlink, and SAN frames
forwarded between non-offloaded HSR slaves.

Because wanted_features is cleared and not restored when the port
leaves, PRP ports and HW-offloaded ports lose software GRO for good.
Host-bound receive then costs more CPU and gets less throughput. The
commit message does not mention this cost. The Fixes: tag would also
bring this behaviour change into stable kernels.

Could the role be limited to the ports where GRO actually breaks
per-frame tagging?

> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "software GRO still on after feature update, cannot join");
> +		res = -EBUSY;
> +		goto fail_role_policy;
> +	}
> +
>  	res = netdev_rx_handler_register(dev, hsr_handle_frame, port);
>  	if (res)
>  		goto fail_rx_handler;
> @@ -177,7 +223,12 @@ static int hsr_portdev_setup(struct hsr_priv *hsr, struct net_device *dev,
>  	return 0;
>  
>  fail_rx_handler:
> +fail_role_policy:
> +	/* Rollback order: unlink the upper, clear the role and
> +	 * recompute, then undo this setup's promiscuous increment.
> +	 */
>  	netdev_upper_dev_unlink(dev, hsr_dev);
> +	hsr_portdev_role_clear(dev);

[Severity: Medium]
Can a failed attach leave GRO disabled on a device that never became an
HSR member?

hsr_portdev_role_set() leads to __netdev_update_features(), which clears
NETIF_F_GRO (and configurable GRO_HW) from dev->wanted_features. This
happens before the attach is known to succeed. The previous request is
not saved anywhere. hsr_portdev_role_clear() only recomputes from the
reduced wanted_features.

This rollback undoes the upper link, the role bit and promiscuity, but
not the GRO request. The -EBUSY check above can fire, for example, when
ndo_set_features() fails. In that case __netdev_update_features()
returns early with dev->features unchanged, but wanted_features has
already been cleared.

A second path goes through hsr_del_port(). hsr_dev_finalize() attaches
the ports one at a time:

net/hsr/hsr_device.c:hsr_dev_finalize() {
    ...
	res = hsr_add_port(hsr, slave[0], HSR_PT_SLAVE_A, extack);
	if (res)
		goto err_unregister;

	res = hsr_add_port(hsr, slave[1], HSR_PT_SLAVE_B, extack);
	if (res)
		goto err_unregister;
    ...
}

Slave B or the interlink can fail, for example in hsr_check_dev_ok() or
in the new -EBUSY check. Then err_unregister runs
hsr_del_ports()->hsr_del_port()->hsr_portdev_role_clear() on slave A,
which had attached successfully, and slave A keeps GRO disabled. For
example:

  ip link add hsr0 type hsr slave1 eth0 slave2 lo

fails, but GRO stays off on eth0.

The commit message says:

  A failed attach does not restore the GRO request either; the next patch
  refuses the usual causes before any state is changed.

The next patch, "net: hsr: refuse forwarding devices as HSR/PRP ports",
only adds checks to hsr_check_dev_ok() for the port being attached.
Wouldn't that still leave out ports attached earlier in
hsr_dev_finalize(), and ndo_set_features() failures?

A stable backport selected through the Fixes: tag could also take this
patch without the follow-up.

>  fail_upper_dev_link:
>  	if (!port->hsr->fwd_offloaded || port->type == HSR_PT_INTERLINK)
>  		dev_set_promiscuity(dev, -1);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107220544.20-1-xiexinet%40gmail.com