Re: [PATCH net-next v5 05/14] fbnic: convert to ndo_set_rx_mode_async (1d5e76c60ed0)

[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.
> ---
> fbnic: convert to ndo_set_rx_mode_async
> 
> Convert fbnic from ndo_set_rx_mode to ndo_set_rx_mode_async, updating
> the driver to take explicit uc/mc lists and using __hw_addr_sync_dev()
> on the snapshots instead of __dev_uc_sync() on the netdev directly.
> 
> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> > index b4b396ca9bce3..c406a3b56b37f 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> [ ... ]
> > @@ -213,10 +215,10 @@ void __fbnic_set_rx_mode(struct fbnic_dev *fbd)
> >  	}
> >  
> >  	/* Synchronize unicast and multicast address lists */
> > -	err = __dev_uc_sync(netdev, fbnic_uc_sync, fbnic_uc_unsync);
> > +	err = __hw_addr_sync_dev(uc, netdev, fbnic_uc_sync, fbnic_uc_unsync);
> >  	if (err == -ENOSPC)
> >  		uc_promisc = true;
> > -	err = __dev_mc_sync(netdev, fbnic_mc_sync, fbnic_mc_unsync);
> > +	err = __hw_addr_sync_dev(mc, netdev, fbnic_mc_sync, fbnic_mc_unsync);
> 
> Since __fbnic_set_rx_mode() performs read-modify-write operations on the
> internal fbd->mac_addr array and updates the device TCAM, is there a need
> for internal synchronization?
>
> It appears that concurrent out-of-band callers like fbnic_bmc_rpc_check()
> (from an unlocked workqueue), RTNL paths like fbnic_set_mac(), and the async
> RX mode workqueue can race on the internal fbd->mac_addr array and interleave
> writes to the hardware, potentially breaking the RX filtering rules.

This is bogus, both paths are rntl/ops protested.

> 
> >  	if (err == -ENOSPC)
> >  		mc_promisc = true;
> [ ... ]
> > @@ -257,7 +262,8 @@ static int fbnic_set_mac(struct net_device *netdev, void *p)
> >  
> >  	eth_hw_addr_set(netdev, addr->sa_data);
> >  
> > -	fbnic_set_rx_mode(netdev);
> > +	if (netif_running(netdev))
> > +		__fbnic_set_rx_mode(fbn->fbd, &netdev->uc, &netdev->mc);
> 
> Is it safe to pass the live &netdev->uc and &netdev->mc address lists to
> __hw_addr_sync_dev() without holding netif_addr_lock_bh() or rtnl_lock()?
>
> This bypasses the core network stack's lock, which could lead to list
> corruption and use-after-free if the network stack concurrently modifies
> the address lists (e.g., via dev_mc_add()).
>
> Additionally, doesn't manually mutating sync_cnt on the live lists bypass
> the core's snapshot reconciliation state machine?

This is the same as before, nothing new added here (and, again, under
rtnl/ops lock).

> 
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> > index e3aebbe3656d5..6b139cf542567 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> [ ... ]
> > @@ -180,7 +180,7 @@ static int fbnic_fw_config_after_crash(struct fbnic_dev *fbd)
> >  	}
> >  
> >  	fbnic_rpc_reset_valid_entries(fbd);
> > -	__fbnic_set_rx_mode(fbd);
> > +	__fbnic_set_rx_mode(fbd, &fbd->netdev->uc, &fbd->netdev->mc);
> 
> Similarly here, fbnic_fw_config_after_crash() appears to be executed from the
> unlocked fbnic_service_task workqueue. Could this cause lockless data races on
> the live network stack lists?

Nothing is lockless here, everything is under rtnl at least.

> 
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> > index 42a186db43ea9..fe95b6f696463 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> [ ... ]
> > @@ -244,7 +244,7 @@ void fbnic_bmc_rpc_check(struct fbnic_dev *fbd)
> >  
> >  	if (fbd->fw_cap.need_bmc_tcam_reinit) {
> >  		fbnic_bmc_rpc_init(fbd);
> > -		__fbnic_set_rx_mode(fbd);
> > +		__fbnic_set_rx_mode(fbd, &fbd->netdev->uc, &fbd->netdev->mc);
> 
> And here as well, passing live address lists without synchronization.

Same here, nothing technically changed and this runs under rtnl.