Re: [PATCH net-next v5 07/14] bnxt: convert to ndo_set_rx_mode_async (c1776bbe53ec)

[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.
> ---
> bnxt: convert to ndo_set_rx_mode_async
> 
> This commit converts the bnxt driver to use ndo_set_rx_mode_async, updating
> the rx mode callbacks to take explicit list parameters and iterate over them.
> 
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index fe8b886ff82ea..569c7de7c5e44 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> 
> [ ... ]
> 
> > @@ -13523,17 +13524,17 @@ void bnxt_get_ring_drv_stats(struct bnxt *bp,
> >  		bnxt_get_one_ring_drv_stats(bp, stats, &bp->bnapi[i]->cp_ring);
> >  }
> >  
> > -static bool bnxt_mc_list_updated(struct bnxt *bp, u32 *rx_mask)
> > +static bool bnxt_mc_list_updated(struct bnxt *bp, u32 *rx_mask,
> > +				 const struct netdev_hw_addr_list *mc)
> >  {
> >  	struct bnxt_vnic_info *vnic = &bp->vnic_info[BNXT_VNIC_DEFAULT];
> > -	struct net_device *dev = bp->dev;
> >  	struct netdev_hw_addr *ha;
> >  	u8 *haddr;
> >  	int mc_count = 0;
> >  	bool update = false;
> >  	int off = 0;
> >  
> > -	netdev_for_each_mc_addr(ha, dev) {
> > +	netdev_hw_addr_list_for_each(ha, mc) {
> 
> When iterating over the list, bnxt_mc_list_updated() performs a memcpy()
> into vnic->mc_list.
> 
> If this executes asynchronously while bnxt_cfg_rx_mode() is mapping
> vnic->mc_list for DMA to the firmware via bnxt_hwrm_cfa_l2_set_rx_mask(),
> could the NIC hardware read torn or corrupted multicast MAC addresses?
> 
> Even if a later patch addresses the explicit list TOCTOU issue, is a
> driver-level spinlock needed here to serialize vnic state access between
> the two concurrent workqueues?

This is not something new I think? The same split was already happening
prior to conversion: bnxt_set_rx_mode updates mc_list and queues
BNXT_RX_MASK_SP_EVENT while bnxt_sp_task does bnxt_cfg_rx_mode.

After the conversion, both paths should be under ops lock, so we should
be in a better place.

> 
> [ ... ]
> 
> > @@ -15881,7 +15884,7 @@ static const struct net_device_ops bnxt_netdev_ops = {
> >  	.ndo_start_xmit		= bnxt_start_xmit,
> >  	.ndo_stop		= bnxt_close,
> >  	.ndo_get_stats64	= bnxt_get_stats64,
> > -	.ndo_set_rx_mode	= bnxt_set_rx_mode,
> > +	.ndo_set_rx_mode_async	= bnxt_set_rx_mode,
> 
> Since this callback now runs asynchronously without the netif_addr_lock_bh()
> that the core networking stack previously held, does this introduce data races
> on the driver state?
> 
> With bnxt_set_rx_mode() executing locklessly in the netdev_rx_mode_work
> workqueue, and bnxt_cfg_rx_mode() concurrently modifying vnic->uc_list,
> vnic->uc_filter_count, and vnic->rx_mask in the driver's bnxt_sp_task
> workqueue, can they race when accessing the vnic state?
> 
> For example, could lockless assignments to vnic->rx_mask in bnxt_set_rx_mode()
> temporarily drop hardware fallback states (like the PROMISCUOUS bit) set by
> bnxt_cfg_rx_mode()?

Hmm, this is completely bogus? I don't see "lockless assignments". After
async conversion all these paths run fully under ops lock.