Re: [PATCH net-next v5 09/14] iavf: convert to ndo_set_rx_mode_async

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
iavf: convert to ndo_set_rx_mode_async

This commit converts the iavf driver from using ndo_set_rx_mode to
ndo_set_rx_mode_async. It updates iavf_set_rx_mode to take explicit
unicast and multicast list parameters and uses __hw_addr_sync_dev on
the snapshots instead of __dev_uc_sync and __dev_mc_sync.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1150,14 +1150,18 @@ bool iavf_promiscuous_mode_changed(struct iavf_adapter *adapter)
> -static void iavf_set_rx_mode(struct net_device *netdev)
> +static void iavf_set_rx_mode(struct net_device *netdev,
> +			     struct netdev_hw_addr_list *uc,
> +			     struct netdev_hw_addr_list *mc)
>  {
>  	struct iavf_adapter *adapter = netdev_priv(netdev);
>  
>  	spin_lock_bh(&adapter->mac_vlan_list_lock);
> -	__dev_uc_sync(netdev, iavf_addr_sync, iavf_addr_unsync);
> -	__dev_mc_sync(netdev, iavf_addr_sync, iavf_addr_unsync);
> +	__hw_addr_sync_dev(uc, netdev, iavf_addr_sync, iavf_addr_unsync);
> +	__hw_addr_sync_dev(mc, netdev, iavf_addr_sync, iavf_addr_unsync);
>  	spin_unlock_bh(&adapter->mac_vlan_list_lock);

[ ... ]

> @@ -1210,7 +1214,7 @@ static void iavf_configure(struct iavf_adapter *adapter)
>  	struct net_device *netdev = adapter->netdev;
>  	int i;
>  
> -	iavf_set_rx_mode(netdev);
> +	iavf_set_rx_mode(netdev, &netdev->uc, &netdev->mc);

Does this code iterate and mutate the live netdev address lists without
holding the required locks?

Previously, iavf_set_rx_mode() called __dev_uc_sync() and __dev_mc_sync(),
which internally acquire netif_addr_lock_bh() to protect the live netdev
address lists.

Now, iavf_configure() passes the live lists (&netdev->uc and &netdev->mc)
directly to iavf_set_rx_mode(), which calls __hw_addr_sync_dev() on them.
__hw_addr_sync_dev() iterates and modifies the lists by updating sync_cnt,
refcount, and removing stale entries.

Since iavf_configure() is called from contexts like iavf_open() and the
driver's watchdog task, it appears netif_addr_lock_bh() is not held here.

Could this cause list or RB-tree corruption if there are concurrent
modifications, such as IGMP/MLD timers adding multicast addresses in softirq
while holding netif_addr_lock_bh()?