Re: [PATCH net v3 2/2] net: sparx5: fix sleep in atomic context in MAC table access

[sashiko-bot]

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

Pre-existing issues:
- [High] The switchdev bridge join/leave handlers manually call `__dev_mc_sync` and `__dev_mc_unsync` without holding `netif_addr_lock_bh`, corrupting `dev->mc` list.
- [High] The driver destroys its workqueue prior to unregistering the notifiers that submit work to it, causing a use-after-free.
- [High] Dynamic FDB MAC entries use `devm_kzalloc` and `devm_kfree`, causing severe O(N) devres list traversal overhead leading to lockups.
--

--- Patch [2]: [PATCH net v3 2/2] net: sparx5: fix sleep in atomic context in MAC table access ---
commit 0271557455a8033457131b5edf4c50d634d7c1d7
Author: Daniel Machon <daniel.machon@xxxxxxxxxxxxx>

net: sparx5: fix sleep in atomic context in MAC table access

This commit converts the sparx5 driver to use the ndo_set_rx_mode_async
callback to avoid sleeping in atomic context. The previous ndo_set_rx_mode
implementation polled the MAC access command register under a mutex, which
is not allowed from atomic context.

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
> index 1d34af78166a3..4dedf5d926416 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
> @@ -162,13 +162,18 @@ static int sparx5_port_stop(struct net_device *ndev)
>  	return 0;
>  }
>  
> -static void sparx5_set_rx_mode(struct net_device *dev)
> +static int sparx5_set_rx_mode(struct net_device *dev,
> +			      struct netdev_hw_addr_list *uc,
> +			      struct netdev_hw_addr_list *mc)
>  {
>  	struct sparx5_port *port = netdev_priv(dev);
>  	struct sparx5 *sparx5 = port->sparx5;
>  
>  	if (!test_bit(port->portno, sparx5->bridge_mask))
> -		__dev_mc_sync(dev, sparx5_mc_sync, sparx5_mc_unsync);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the
__dev_mc_sync changes, I noticed a pre-existing issue in the switchdev code.
The bridge join/leave handlers manually call __dev_mc_sync() and
__dev_mc_unsync() without holding netif_addr_lock_bh:

drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c:sparx5_port_bridge_join() {
    ...
    __dev_mc_unsync(ndev, sparx5_mc_unsync);
    ...
}

drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c:sparx5_port_bridge_leave() {
    ...
    __dev_mc_sync(port->ndev, sparx5_mc_sync, sparx5_mc_unsync);
    ...
}

Since these are called under rtnl_lock but without netif_addr_lock_bh, could
this corrupt the dev->mc linked list if there are concurrent unprivileged
multicast mutations (like IGMP/MLD)?

> +		return __hw_addr_sync_dev(mc, dev, sparx5_mc_sync,
> +					  sparx5_mc_unsync);
> +
> +	return 0;
>  }

[ ... ]

[Severity: High]
This is a pre-existing issue, but does module removal destroy the workqueue
while the switchdev notifier is still active?

drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c:sparx5_unregister_notifier_blocks() {
    ...
    destroy_workqueue(sparx5_owq);
    
    unregister_switchdev_blocking_notifier(&s5->switchdev_blocking_nb);
    unregister_switchdev_notifier(&s5->switchdev_nb);
    ...
}

If a concurrent switchdev FDB event occurs after destroy_workqueue() but
before unregister_switchdev_notifier(), sparx5_switchdev_event() might
queue work on the destroyed workqueue. Can this sequence result in a
use-after-free and kernel panic?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver use devres
allocations for highly dynamic network state?

drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c:alloc_mact_entry() {
    ...
    mact_entry = devm_kzalloc(sparx5->dev,
                              sizeof(*mact_entry), GFP_ATOMIC);
    ...
}

drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c:sparx5_del_mact_entry() {
    ...
    devm_kfree(sparx5->dev, mact_entry);
    ...
}

Because devm_kfree() linearly scans the entire devres list under the
dev->devres_lock spinlock with IRQs disabled, could this cause severe O(N)
traversal overhead and lockups when handling thousands of dynamic FDB MAC
entries?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-misc-fixes-sparx5-lan969x-v3-0-c7c7fef723a8@xxxxxxxxxxxxx?part=2