Re: [PATCH net v2 3/3] net/mlx5: Bridge, don't fail unlink of untracked/unsupported peer ports

[netdev-bot+sashiko@kernel.org]

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

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

- [Low] Commit message overstates the notifier-chain impact.

--

net/mlx5: Bridge, don't fail unlink of untracked/unsupported peer ports

The patch makes mlx5_esw_bridge_vport_unlink() return 0 instead of -EINVAL
when the vport isn't present in this instance's br_offloads->ports xarray,
and adds a merged_eswitch capability guard to
mlx5_esw_bridge_vport_peer_unlink() mirroring the one in
mlx5_esw_bridge_vport_peer_link().

(No Link: tags in the commit header.)

Only the commit message is being commented on here, so the diff hunks are
omitted.

> This also matters beyond the -EINVAL itself:
> mlx5_esw_bridge_switchdev_port_event() runs on the per-netns
> netdev_chain, and notifier_from_errno(-EINVAL) sets NOTIFY_STOP_MASK,
> which call_netdevice_notifiers_info() checks to skip the global chain
> entirely - so the old -EINVAL silently dropped the event for every
> other listener on the system, including a sibling PF's own
> bridge-offload instance, even though none of it was visible to user
> space since __netdev_upper_dev_unlink() discards the return value.

[Severity: Low]
Is "dropped the event for every other listener on the system" accurate here?

notifier_call_chain() breaks at the callback that returned the stop bit, so
listeners ordered before mlx5's netdev_nb have already been invoked:

kernel/notifier.c:notifier_call_chain() {
	...
		ret = nb->notifier_call(nb, val, v);
	...
		if (ret & NOTIFY_STOP_MASK)
			break;
		nb = next_nb;
	...
}

The part about the global chain does hold:

net/core/dev.c:call_netdevice_notifiers_info() {
	ret = raw_notifier_call_chain(&net->netdev_chain, val, info);
	if (ret & NOTIFY_STOP_MASK)
		return ret;
	return raw_notifier_call_chain(&netdev_chain, val, info);
}

Regarding the sibling PF claim, mlx5 registers on the per-netns chain:

drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c:mlx5e_rep_bridge_init() {
	br_offloads->netdev_nb.notifier_call = mlx5_esw_bridge_switchdev_port_event;
	err = register_netdevice_notifier_net(&init_net, &br_offloads->netdev_nb);
	...
}

A sibling PF's br_offloads instance registers on that same per-netns chain,
so whether it misses the event depends on registration order rather than
being unconditional.

Would wording like "suppresses the remaining per-netns listeners
(potentially including a sibling PF's instance) and all global-chain
listeners" describe the behaviour more precisely?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914085430.18694-1-bersoare%40isovalent.com