From: Daniel Borkmann <daniel@iogearbox.net> Date: 2026-09-08 16:19:56
From: Bernardo Soares <redacted>
mlx5 registers the bridge offload switchdev notifiers once per eswitch
instance, i.e. once per PF, but the switchdev notifier chains are global.
Every registered instance is called for every switchdev event and has to
filter out the events that are not its own.
Since commit c358ea1741bc ("net/mlx5: Bridge, allow merged eswitch
connectivity") that filter is mlx5_esw_bridge_dev_same_hw(), which only
tells that the event netdevice and the eswitch of the instance sit on the
same HCA. This is intentional: with merged eswitch a bridge can span the
representors of several eswitches of one HCA, and each instance keeps the
ports of its sibling eswitches as peer ports in order to offload FDB
entries pointing at them.
However, the instance may not have the port at all. Peer ports are only
created from NETDEV_CHANGEUPPER events observed while the instance is
already registered, and only when merged_eswitch is supported. So when
more than one PF of the same HCA runs bridge offload and the PF that is
put in switchdev mode last has the uplink of an earlier configured PF
already enslaved to a bridge, that instance has no port for it.
The switchdev port object and attribute handlers do not check for this.
They claim the event via port_obj_info->handled and pass the
vport_num/esw_owner_vhca_id pair down, where the port lookup fails and
-EINVAL is returned. call_switchdev_blocking_notifiers() stops the chain
on the error and it is reported to user space, even though the instance
owning the port has already offloaded the request:
# bridge vlan add dev ens1f2np2 vid 999 master
RTNETLINK answers: Invalid argument
The same applies to bridge vlan del, mdb add/del and to the bridge
attributes (ageing time, vlan filtering, vlan protocol, mcast). Thus
fix by filter on the port instead. We tested that this fixes the issue
on ConnectX-7.
Fixes: c358ea1741bc ("net/mlx5: Bridge, allow merged eswitch connectivity")
Signed-off-by: Bernardo Soares <redacted>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Vlad Buslov <redacted>
Cc: Saeed Mahameed <saeedm@nvidia.com>
---
.../mellanox/mlx5/core/en/rep/bridge.c | 32 +++++++++++++++----
.../ethernet/mellanox/mlx5/core/esw/bridge.c | 6 ++++
.../ethernet/mellanox/mlx5/core/esw/bridge.h | 2 ++
3 files changed, 34 insertions(+), 6 deletions(-)
From: Mark Bloch <mbloch@nvidia.com> Date: 2026-09-10 12:48:11
On 08/09/2026 19:19, Daniel Borkmann wrote:
quoted hunk
From: Bernardo Soares <redacted>
mlx5 registers the bridge offload switchdev notifiers once per eswitch
instance, i.e. once per PF, but the switchdev notifier chains are global.
Every registered instance is called for every switchdev event and has to
filter out the events that are not its own.
Since commit c358ea1741bc ("net/mlx5: Bridge, allow merged eswitch
connectivity") that filter is mlx5_esw_bridge_dev_same_hw(), which only
tells that the event netdevice and the eswitch of the instance sit on the
same HCA. This is intentional: with merged eswitch a bridge can span the
representors of several eswitches of one HCA, and each instance keeps the
ports of its sibling eswitches as peer ports in order to offload FDB
entries pointing at them.
However, the instance may not have the port at all. Peer ports are only
created from NETDEV_CHANGEUPPER events observed while the instance is
already registered, and only when merged_eswitch is supported. So when
more than one PF of the same HCA runs bridge offload and the PF that is
put in switchdev mode last has the uplink of an earlier configured PF
already enslaved to a bridge, that instance has no port for it.
The switchdev port object and attribute handlers do not check for this.
They claim the event via port_obj_info->handled and pass the
vport_num/esw_owner_vhca_id pair down, where the port lookup fails and
-EINVAL is returned. call_switchdev_blocking_notifiers() stops the chain
on the error and it is reported to user space, even though the instance
owning the port has already offloaded the request:
# bridge vlan add dev ens1f2np2 vid 999 master
RTNETLINK answers: Invalid argument
The same applies to bridge vlan del, mdb add/del and to the bridge
attributes (ageing time, vlan filtering, vlan protocol, mcast). Thus
fix by filter on the port instead. We tested that this fixes the issue
on ConnectX-7.
Fixes: c358ea1741bc ("net/mlx5: Bridge, allow merged eswitch connectivity")
Signed-off-by: Bernardo Soares <redacted>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Vlad Buslov <redacted>
Cc: Saeed Mahameed <saeedm@nvidia.com>
---
.../mellanox/mlx5/core/en/rep/bridge.c | 32 +++++++++++++++----
.../ethernet/mellanox/mlx5/core/esw/bridge.c | 6 ++++
.../ethernet/mellanox/mlx5/core/esw/bridge.h | 2 ++
3 files changed, 34 insertions(+), 6 deletions(-)
v2 of "net/mlx5: Bridge, don't fail switchdev events of sibling
eswitch ports" (5f324c5d1b12), addressing the two follow-up issues
raised in review plus one additional issue found while auditing the
same code for the same bug class:
- Patch 1 is the original fix, unchanged.
- Patch 2 addresses the reviewer's comment that
mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get() picks the first
structurally-eligible rep found while walking a LAG bond's lower
devices, without verifying that rep is actually tracked by the
calling instance's br_offloads. On a merged-eswitch HCA with a
bond spanning reps of more than one eswitch instance, this could
cause a bridge attribute change (ageing time, vlan filtering/
protocol, mcast) to silently no-op on the correct instance.
- Patch 3 addresses the reviewer's comment asking whether the peer
unlink path also needed changes, plus a related idempotency gap
found in follow-up: mlx5_esw_bridge_vport_peer_unlink() lacked the
merged_eswitch capability guard that peer_link() already has, and
mlx5_esw_bridge_vport_unlink() itself returned -EINVAL rather than
treating an already-absent/untracked port as a no-op, which is
reachable on a replayed or duplicate NETDEV_CHANGEUPPER unlink.
Tested Patch 1 on a ConnectX-7 NIC (MT2910) on my single NIC system.
Patches 2 and 3 require a multiple eswitch instance setup, so I wasn't
able to exercise their code paths.
Bernardo Soares (3):
net/mlx5: Bridge, don't fail switchdev events of sibling eswitch ports
net/mlx5: LAG, check if vport exists on br_offloads
net/mlx5: Bridge, don't fail unlink of untracked/unsupported peer
ports
.../mellanox/mlx5/core/en/rep/bridge.c | 45 +++++++++++++++----
.../ethernet/mellanox/mlx5/core/esw/bridge.c | 15 +++++--
.../ethernet/mellanox/mlx5/core/esw/bridge.h | 2 +
3 files changed, 49 insertions(+), 13 deletions(-)
--
2.50.1
mlx5 registers the bridge offload switchdev notifiers once per eswitch
instance, but the notifier chains are global, so every instance sees
every event and must filter out the ones that aren't its own. The
existing filter, mlx5_esw_bridge_dev_same_hw(), only checks that the
event netdevice sits on the same HCA - intentional for merged eswitch,
where one bridge can span representors of several eswitches on one
HCA - but same-HCA doesn't mean the instance actually has that port:
peer ports are only created reactively from NETDEV_CHANGEUPPER, so an
instance brought up after a sibling PF's port was already enslaved has
none. The port object and attribute handlers claim the event anyway
once same-HW passes, then fail the port lookup and return -EINVAL,
which gets reported to user space even though the owning instance
already handled it (e.g. "bridge vlan add ... RTNETLINK answers:
Invalid argument"). Fix by filtering on the tracked port instead.
Fixes: c358ea1741bc ("net/mlx5: Bridge, allow merged eswitch connectivity")
Signed-off-by: Bernardo Soares <redacted>
Cc: Vlad Buslov <redacted>
Cc: Saeed Mahameed <saeedm@nvidia.com>
---
v2: unchanged, resending as part of the series alongside the two
follow-up fixes raised in review (patches 2 and 3).
---
.../mellanox/mlx5/core/en/rep/bridge.c | 32 +++++++++++++++----
.../ethernet/mellanox/mlx5/core/esw/bridge.c | 6 ++++
.../ethernet/mellanox/mlx5/core/esw/bridge.h | 2 ++
3 files changed, 34 insertions(+), 6 deletions(-)
mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get() walks a LAG bond's
lower devices and returns the first rep that's structurally eligible
(same HCA, is a rep), without checking it's tracked by the calling
instance's br_offloads. On a merged-eswitch HCA where a bond spans
reps of more than one eswitch instance, this can return a sibling's
rep instead of continuing to the one this instance actually owns -
reached via mlx5_esw_bridge_port_obj_attr_set(), so a bridge attribute
change on a bonded uplink can silently no-op on the right instance.
Fix by checking mlx5_esw_bridge_port_exists() at the point each rep is
picked, same as the previous commit did for the single-rep case.
Signed-off-by: Bernardo Soares <redacted>
---
v2: new patch, addresses review comment on v1 asking whether
mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get() should verify
the rep it picks while walking a LAG bond's lower devices is
actually tracked by the calling instance's br_offloads, since it
otherwise returns on the first structurally-eligible rep found,
which is not necessarily the one owned by this eswitch instance.
---
.../net/ethernet/mellanox/mlx5/core/en/rep/bridge.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
mlx5_esw_bridge_vport_unlink() returns -EINVAL when the port isn't
tracked by this instance's br_offloads, which is reachable in two
legitimate cases: a replayed/duplicate NETDEV_CHANGEUPPER unlink for a
port already cleaned up, or a peer-port unlink for a vport whose link
was never created because peer_link() skipped it. Return 0 instead in
both cases, and give mlx5_esw_bridge_vport_peer_unlink() the same
merged_eswitch capability guard peer_link() already has.
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.
Fixes: c358ea1741bc ("net/mlx5: Bridge, allow merged eswitch connectivity")
Signed-off-by: Bernardo Soares <redacted>
---
v2: new patch, addresses review comment on v1 asking whether the peer
unlink path also needed changes. mlx5_esw_bridge_vport_peer_unlink()
now mirrors mlx5_esw_bridge_vport_peer_link()'s merged_eswitch
capability guard, and mlx5_esw_bridge_vport_unlink() treats an
already-absent/untracked port as a no-op (return 0) instead of
-EINVAL, which is reachable on a replayed or duplicate
NETDEV_CHANGEUPPER unlink and was found while auditing the same
code for the ownership-check gap raised on v1.
---
drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
@@ -1649,10 +1649,8 @@ int mlx5_esw_bridge_vport_unlink(struct net_device *br_netdev, u16 vport_num,interr;port=mlx5_esw_bridge_port_lookup(vport_num,esw_owner_vhca_id,br_offloads);-if(!port){-NL_SET_ERR_MSG_MOD(extack,"Port is not attached to any bridge");-return-EINVAL;-}+if(!port)+return0;if(port->bridge->ifindex!=br_netdev->ifindex){NL_SET_ERR_MSG_MOD(extack,"Port is attached to another bridge");return-EINVAL;
@@ -1682,6 +1680,9 @@ int mlx5_esw_bridge_vport_peer_unlink(struct net_device *br_netdev, u16 vport_nustructmlx5_esw_bridge_offloads*br_offloads,structnetlink_ext_ack*extack){+if(!MLX5_CAP_ESW(br_offloads->esw->dev,merged_eswitch))+return0;+returnmlx5_esw_bridge_vport_unlink(br_netdev,vport_num,esw_owner_vhca_id,br_offloads,extack);}
From: Mark Bloch <mbloch@nvidia.com> Date: 2026-09-15 19:51:39
On 14/09/2026 11:54, Bernardo Soares wrote:
mlx5_esw_bridge_vport_unlink() returns -EINVAL when the port isn't
tracked by this instance's br_offloads, which is reachable in two
legitimate cases: a replayed/duplicate NETDEV_CHANGEUPPER unlink for a
port already cleaned up, or a peer-port unlink for a vport whose link
was never created because peer_link() skipped it. Return 0 instead in
both cases, and give mlx5_esw_bridge_vport_peer_unlink() the same
merged_eswitch capability guard peer_link() already has.
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.
Fixes: c358ea1741bc ("net/mlx5: Bridge, allow merged eswitch connectivity")
Signed-off-by: Bernardo Soares <redacted>
Commit message nit: NETDEV_CHANGEUPPER is not replayed, and
only listeners later in the chain are skipped. The code looks
good.
checkpatch warns the From address is gmail while the signed-off-by
uses the Isovalent address. Please make them match.
v2 was sent as a reply to v1. Please send v3 as a new thread.
Mark
quoted hunk
---
v2: new patch, addresses review comment on v1 asking whether the peer
unlink path also needed changes. mlx5_esw_bridge_vport_peer_unlink()
now mirrors mlx5_esw_bridge_vport_peer_link()'s merged_eswitch
capability guard, and mlx5_esw_bridge_vport_unlink() treats an
already-absent/untracked port as a no-op (return 0) instead of
-EINVAL, which is reachable on a replayed or duplicate
NETDEV_CHANGEUPPER unlink and was found while auditing the same
code for the ownership-check gap raised on v1.
---
drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
@@ -1649,10 +1649,8 @@ int mlx5_esw_bridge_vport_unlink(struct net_device *br_netdev, u16 vport_num,interr;port=mlx5_esw_bridge_port_lookup(vport_num,esw_owner_vhca_id,br_offloads);-if(!port){-NL_SET_ERR_MSG_MOD(extack,"Port is not attached to any bridge");-return-EINVAL;-}+if(!port)+return0;if(port->bridge->ifindex!=br_netdev->ifindex){NL_SET_ERR_MSG_MOD(extack,"Port is attached to another bridge");return-EINVAL;
@@ -1682,6 +1680,9 @@ int mlx5_esw_bridge_vport_peer_unlink(struct net_device *br_netdev, u16 vport_nustructmlx5_esw_bridge_offloads*br_offloads,structnetlink_ext_ack*extack){+if(!MLX5_CAP_ESW(br_offloads->esw->dev,merged_eswitch))+return0;+returnmlx5_esw_bridge_vport_unlink(br_netdev,vport_num,esw_owner_vhca_id,br_offloads,extack);}
From: Mark Bloch <mbloch@nvidia.com> Date: 2026-09-15 19:52:14
On 14/09/2026 11:54, Bernardo Soares wrote:
mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get() walks a LAG bond's
lower devices and returns the first rep that's structurally eligible
(same HCA, is a rep), without checking it's tracked by the calling
instance's br_offloads. On a merged-eswitch HCA where a bond spans
reps of more than one eswitch instance, this can return a sibling's
rep instead of continuing to the one this instance actually owns -
reached via mlx5_esw_bridge_port_obj_attr_set(), so a bridge attribute
change on a bonded uplink can silently no-op on the right instance.
Fix by checking mlx5_esw_bridge_port_exists() at the point each rep is
picked, same as the previous commit did for the single-rep case.
Signed-off-by: Bernardo Soares <redacted>
Please squash this into patch 1.
Mark> ---
quoted hunk
v2: new patch, addresses review comment on v1 asking whether
mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get() should verify
the rep it picks while walking a LAG bond's lower devices is
actually tracked by the calling instance's br_offloads, since it
otherwise returns on the first structurally-eligible rep found,
which is not necessarily the one owned by this eswitch instance.
---
.../net/ethernet/mellanox/mlx5/core/en/rep/bridge.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 1
- [Low] In this commit, mlx5_esw_bridge_lower_rep_port_lookup()
(drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c:116-126)…
--
net/mlx5: Bridge, don't fail switchdev events of sibling eswitch ports
The mlx5 bridge offload registers switchdev notifiers per eswitch instance
while the notifier chains are global, so each instance must filter events
that aren't its own. This patch replaces the same-HW filter in the port
object and attribute handlers with a check that the instance actually
tracks the port, so sibling eswitch events no longer return -EINVAL to
user space.
[Severity: Low]
Should the tracked-port test happen inside the lower device walk rather
than after it?
mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get() stops at the first
same-HCA representor it finds while walking the bridge's lower devices:
netdev_for_each_lower_dev(dev, lower_dev, iter) {
struct net_device *rep;
if (netif_is_bridge_master(lower_dev))
continue;
rep = mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get(lower_dev, esw, vport_num,
esw_owner_vhca_id);
if (rep)
return rep;
}
Bridge-level attributes are notified with dev == the bridge netdevice, so
on a merged-eswitch HCA whose bridge holds representors of two eswitch
instances, the walk can return the sibling's representor first. If the
calling instance has no peer port for that representor - the late-start
case described in the commit message - mlx5_esw_bridge_port_exists()
fails and the whole lookup returns false, even though a later lower
device is a port this instance does track.
quoted hunk
@@ -283,8 +303,8 @@ mlx5_esw_bridge_port_obj_attr_set(struct net_device *dev, u16 vport_num, esw_owner_vhca_id; int err = 0;- if (!mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get(dev, br_offloads->esw, &vport_num,- &esw_owner_vhca_id))+ if (!mlx5_esw_bridge_lower_rep_port_lookup(dev, br_offloads, &vport_num,+ &esw_owner_vhca_id)) return 0; port_attr_info->handled = true;
Can this turn the previous -EINVAL into a silently dropped attribute
update for that instance? mlx5_esw_bridge_port_obj_attr_set() returns 0
with port_attr_info->handled left false, so the ageing time, VLAN
filtering, VLAN protocol and MC-disabled updates never reach that
instance's own bridge object. Each instance resolves its bridge through
the port, for example:
drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c:mlx5_esw_bridge_vlan_filtering_set() {
bridge = mlx5_esw_bridge_from_port_lookup(vport_num, esw_owner_vhca_id, br_offloads);
if (!bridge)
return -EINVAL;
...
}
so after e.g. "ip link set br0 type bridge vlan_filtering 1" the software
bridge would have filtering enabled while that instance's hardware bridge
keeps MLX5_ESW_BRIDGE_VLAN_FILTERING_FLAG clear, with no error reported.
The follow-up patch in this series, "net/mlx5: LAG, check if vport exists
on br_offloads", moves the mlx5_esw_bridge_port_exists() test into the
leaf of the recursive walk so an untracked representor returns NULL and
netdev_for_each_lower_dev() continues to the next lower device, which
removes this behaviour at the end of the series. Would it make sense to
fold that change into this patch, or to note that this patch alone is not
suitable for backporting without it?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914085430.18694-1-bersoare%40isovalent.com
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 0 · Low: 1
- [High] The commit message describes a bug that the code cannot exhibit,
and the patch is a functional no-op for the scenario it names.
- [Low] The commit asserts a user-visible functional defect ('a bridge
attribute change on a bonded uplink can silently no-op on the right…
--
net/mlx5: LAG, check if vport exists on br_offloads
This adds a mlx5_esw_bridge_port_exists() check to the LAG-master/rep
branch of mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get(), so that a
rep which is not tracked by the calling instance's br_offloads is
rejected instead of returned.
[Severity: High]
Does this hunk change anything for the bonded-uplink case the changelog
describes?
The changelog says:
mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get() walks a LAG bond's
lower devices and returns the first rep that's structurally eligible
(same HCA, is a rep), without checking it's tracked by the calling
instance's br_offloads. On a merged-eswitch HCA where a bond spans
reps of more than one eswitch instance, this can return a sibling's
rep instead of continuing to the one this instance actually owns
For dev being the LAG master, the bond walk happens in
mlx5_esw_bridge_lag_rep_get(), reached via
mlx5_esw_bridge_rep_vport_num_vhca_id_get():
if (netif_is_lag_master(dev))
dev = mlx5_esw_bridge_lag_rep_get(dev, esw);
and that walk already filters on the eswitch instance:
netdev_for_each_lower_dev(dev, lower, iter) {
if (!mlx5e_eswitch_rep(lower))
continue;
if (mlx5_esw_bridge_dev_same_esw(lower, esw))
return lower;
}
So can it actually return a sibling instance's rep here? It looks like
only lowers satisfying mlx5_esw_bridge_dev_same_esw(lower, esw) are
accepted, which would make the "same HCA, is a rep" description apply to
mlx5_esw_bridge_rep_vport_num_vhca_id_get()'s non-LAG path rather than to
the bond walk.
The new branch is also terminal - on rejection it returns NULL and no
other bond member is examined - so what is "continued to" in the LAG case?
There is a second question about the result being identical before and
after. The only caller is mlx5_esw_bridge_lower_rep_port_lookup(), which
repeats the same test on the same br_offloads:
if (!mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get(dev, br_offloads->esw, vport_num,
esw_owner_vhca_id))
return false;
return mlx5_esw_bridge_port_exists(*vport_num, *esw_owner_vhca_id, br_offloads);
With esw->br_offloads and br_offloads being the same object, a dev that is
a LAG master ends up with the same verdict either way, and
mlx5_esw_bridge_port_obj_attr_set() still returns 0 without applying the
attribute:
if (!mlx5_esw_bridge_lower_rep_port_lookup(dev, br_offloads, &vport_num,
&esw_owner_vhca_id))
return 0;
The behaviour that does change is the recursive lower-device walk further
down, for a dev that is neither a LAG master nor a rep (the bridge
netdevice itself, for bridge-wide attributes such as ageing time, VLAN
filtering, VLAN protocol and MC-disabled): an untracked sibling lower rep
no longer terminates the walk, so the walk can reach the lower rep this
instance does track. Would it be clearer to describe that path in the
subject and changelog instead of the LAG bond?
[Severity: Low]
Should this patch carry a Fixes: tag?
The changelog claims a user-visible effect ("a bridge attribute change on
a bonded uplink can silently no-op on the right instance"), but there is
no Fixes: line, while the two sibling patches in the series both tag
Fixes: c358ea1741bc ("net/mlx5: Bridge, allow merged eswitch
connectivity").
The behaviour actually changed here (the recursive lower-device walk in
mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get() stopping on an untracked
sibling rep) is a merged-eswitch bug too, so either adding the matching
Fixes: tag or rewording the message to say this is follow-up hardening of
the preceding patch with no independent user-visible effect would remove
the ambiguity.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914085430.18694-1-bersoare%40isovalent.com
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