Re: [PATCH net v8 2/4] net: hsr: refuse forwarding devices as 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: 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 fix does not close the bug class it targets.
- [Medium] hsr_check_dev_ok() now refuses every macvlan and MACsec device
  as an HSR/PRP slave or interlink with -EINVAL, whatever its lower…

--

net: hsr: refuse forwarding devices as HSR/PRP ports

This patch makes hsr_check_dev_ok() refuse bridge, OVS, bond, team,
macvlan and MACsec devices as an HSR/PRP slave or interlink. It also
refuses devices with IFF_NO_RX_HANDLER or a busy RX handler before any
state is changed. The target is a syzbot recursive locking report: a
supervision frame sent with seqnr_lock held goes to a bridge slave, which
floods it into a second HSR master.

> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> index bc340ae0b52e1..26f16d98e4793 100644
> --- a/net/hsr/hsr_slave.c
> +++ b/net/hsr/hsr_slave.c

[ ... ]

> @@ -131,9 +147,19 @@ static int hsr_check_dev_ok(struct net_device *dev,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	/* HSR over bonded devices has not been tested, but I'm not sure it
> -	 * won't work...
> +	/* Forwarding devices feed HSR frames into another device's
> +	 * transmit path. A bridge, bond, team or OVS master or a macvlan
> +	 * or MACsec device makes no sense as a ring or LAN port and can
> +	 * nest seqnr_lock through a second HSR device. Refuse them by
> +	 * type: devices with lower devices in general stay valid, DSA
> +	 * user ports are uppers of their conduit.
>  	 */
> +	if (netif_is_any_bridge_master(dev) || netif_is_lag_master(dev) ||
> +	    netif_is_macvlan(dev) || netif_is_macsec(dev)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Cannot use a bridge, bond, team, OVS, macvlan or MACsec device as HSR port.");
> +		return -EINVAL;
> +	}

[Severity: High]
Does this type check close the seqnr_lock nesting, or only the bridge
reproducer from syzbot?

The lock is still held across the whole transmit path of the port. For
example, in hsr_dev_xmit():

net/hsr/hsr_device.c:hsr_dev_xmit() {
    ...
		spin_lock_bh(&hsr->seqnr_lock);
		hsr_forward_skb(skb, master);
		spin_unlock_bh(&hsr->seqnr_lock);
    ...
}

send_hsr_supervision_frame(), send_prp_supervision_frame() and the
interlink branch of hsr_handle_frame() follow the same pattern. The call
hsr_forward_skb()->hsr_xmit()->dev_queue_xmit(port) runs the port's tc,
BPF and ndo_start_xmit with the lock held.

spin_lock_init() is only called from hsr_dev_finalize(), so all HSR
instances share one lockdep class.

Any ARPHRD_ETHER port that passes these checks but whose egress reaches an
HSR master synchronously seems to hit the same splat:

- A plain veth or NIC slave with a clsact egress filter doing
  "mirred egress redirect dev hsr1". In that case
  tcf_mirred_act()->tcf_mirred_forward() calls dev_queue_xmit(hsr1). The
  mirred loop check only compares against devices already in
  sched_mirred_dev[], so it does not catch the first redirect.
  bpf_redirect() from tcx or cls_bpf behaves the same way.

- A virt_wifi device created on top of an HSR master. virt_wifi_newlink()
  accepts any lower device, and the virt_wifi device is set up with
  ether_setup() and none of the refused priv_flags.
  virt_wifi_start_xmit() then does:

	skb->dev = priv->lowerdev;
	return dev_queue_xmit(skb);

- Ethernet tunnels whose underlay routes through an HSR master. The
  commit message already mentions these.

Suppose the redirect targets the same HSR master: veth0 is a slave of
hsr0, and veth0's egress filter redirects to hsr0. Can this deadlock the
CPU?

hsr_announce() timer
  spin_lock_bh(&hsr0->seqnr_lock)
  hsr_forward_skb()->hsr_xmit()->dev_queue_xmit(veth0)
    sch_handle_egress()->tcf_mirred_act()->tcf_mirred_forward()
      dev_queue_xmit(hsr0)
        hsr_dev_xmit()
          spin_lock_bh(&hsr0->seqnr_lock)

hsr0 is lltx and IFF_NO_QUEUE, so __dev_queue_xmit() calls ndo_start_xmit
directly. Neither the dev_xmit_recursion() limit nor MIRRED_NEST_LIMIT is
reached before the second spin_lock_bh(), so the CPU spins on a lock it
already holds, with BH disabled. Setting this up only needs CAP_NET_ADMIN,
which a user namespace provides.

The commit message says:

    A chain of two devices does not deadlock, but such a port makes no
    sense

It lists only tunnels and batman-adv as not covered. Should it also
mention tc/BPF redirects, virt_wifi and the single-device self-deadlock?

The later commit "net: hsr: segment GSO before per-frame forwarding" moves
the lock into hsr_forward_frame(). It still holds the lock around
hsr_forward_skb(), and so around dev_queue_xmit(), so the problem appears
to remain at the end of the series.

Would it be better to change the locking itself? For example, the
sequence number could be assigned under seqnr_lock and the frame sent
after dropping the lock. At minimum, each instance could get its own
lockdep key. Either would replace this denylist rather than extend it, so
the Closes: tag for the syzbot report looks premature.

[Severity: Medium]
Is it intended to refuse every macvlan and MACsec device here, whatever
its lower device is?

Before this patch both types were accepted as a slave or interlink. Both
are ARPHRD_ETHER with ETH_ALEN, neither sets IFF_DONT_BRIDGE or
IFF_NO_RX_HANDLER, and their RX handler is on the lower device.

The commit message justifies the type rule with "ports can be added
later", but that only applies to bridge, bond and team masters. A
macvlan's lower and a MACsec real_dev are fixed when the device is
created. After that, macvlan_port or macsec holds the lower's RX handler,
so the lower cannot join a bridge or bond later either.

Could a single walk of the lower chain at attach time close the nesting
path through these two types, refusing an HSR master or a bridge/LAG
master found there? Configurations that cannot nest seqnr_lock would then
keep working. Examples are PRP LANs or HSR links over MACsec-protected
physical links, and PRP ports on macvlan in containers.

Because of the Fixes: tags, stable kernels would start rejecting these
previously valid rtnetlink requests with -EINVAL, for example
"ip link add ... type hsr slave1 macsec0 ...". Should the commit message
say that these configurations are no longer allowed?

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