Re: [PATCH net-next v5 07/14] bnxt: 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.
---
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?

[ ... ]

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

>  	.ndo_eth_ioctl		= bnxt_ioctl,
>  	.ndo_validate_addr	= eth_validate_addr,
>  	.ndo_set_mac_address	= bnxt_change_mac_addr,