Re: [PATCH net-next v5 08/14] bnxt: use snapshot in bnxt_cfg_rx_mode

[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: use snapshot in bnxt_cfg_rx_mode

With the introduction of ndo_set_rx_mode_async we can call bnxt_cfg_rx_mode
directly. Convert bnxt_cfg_rx_mode to use uc/mc snapshots and move its
call in bnxt_sp_task to the section that resets BNXT_STATE_IN_SP_TASK.

Link: https://lore.kernel.org/netdev/CACKFLi=5vj8hPqEUKDd8RTw3au5G+zRgQEqjF+6NZnyoNm90KA@mail.gmail.com/

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
[ ... ]
> -static int bnxt_cfg_rx_mode(struct bnxt *bp)
> +static int bnxt_cfg_rx_mode(struct bnxt *bp, struct netdev_hw_addr_list *uc,
> +			    struct netdev_hw_addr_list *mc)

This isn't a bug, but is the mc parameter used in this function? The
multicast configuration appears to be handled synchronously prior to this call
by bnxt_mc_list_updated().

>  {
>  	struct net_device *dev = bp->dev;
>  	struct bnxt_vnic_info *vnic = &bp->vnic_info[BNXT_VNIC_DEFAULT];
>  	struct netdev_hw_addr *ha;
>  	int i, off = 0, rc;
> -	bool uc_update;
> -
> -	netif_addr_lock_bh(dev);
> -	uc_update = bnxt_uc_list_updated(bp, &dev->uc);
> -	netif_addr_unlock_bh(dev);
> -
> -	if (!uc_update)
> -		goto skip_uc;

By removing this check, will the driver now unconditionally delete and recreate
all secondary hardware unicast MAC filters via HWRM commands on any RX mode
change?

For example, if an unprivileged user joins a multicast group via
IP_ADD_MEMBERSHIP, does this create a time window where the hardware drops
valid unicast traffic destined for secondary MACs?

Also, if any of the firmware commands to re-add a filter fail, will the
function return early and skip programming the RX mask entirely?

[ ... ]
> @@ -14684,6 +14675,12 @@ static void bnxt_sp_task(struct work_struct *work)
>  	/* These functions below will clear BNXT_STATE_IN_SP_TASK.  They
>  	 * must be the last functions to be called before exiting.
>  	 */
> +	if (test_and_clear_bit(BNXT_RX_MASK_SP_EVENT, &bp->sp_event)) {
> +		bnxt_lock_sp(bp);

Could this introduce a race condition with bnxt_close()?

bnxt_lock_sp() temporarily clears the BNXT_STATE_IN_SP_TASK bit before
blocking on netdev_lock(bp->dev). If bnxt_close() is called concurrently, it
holds netdev_lock() and loops waiting for BNXT_STATE_IN_SP_TASK to clear.

When bnxt_lock_sp() clears the bit, bnxt_close() could unblock, shut down the
device, and free memory including bp->vnic_info via bnxt_free_vnics().

After bnxt_close() completes and releases the lock, bnxt_sp_task() would
acquire it and unconditionally call bnxt_cfg_rx_mode(), which dereferences the
now-freed bp->vnic_info.

Would it be safer to check test_bit(BNXT_STATE_OPEN, &bp->state) after
acquiring the lock, similar to what neighboring handlers do?

> +		bnxt_cfg_rx_mode(bp, &dev->uc, &dev->mc);
> +		bnxt_unlock_sp(bp);
> +	}