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

[sdf.kernel@gmail.com]

> 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()?

Most of this is seems bogus:
- ndo_set_rx_mode_async path is fine because of the snapshot
- iavf_configure is fine because of the ops lock

What I can do is to wrap iavf_set_rx_mode in iavf_configure into
netif_addr_lock_bh to satisfy the bots?