Re: [PATCH net-next 3/4] bnge: add ndo_set_rx_mode_async support

[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.
---
--- Patch [3]: [PATCH net-next 3/4] bnge: add ndo_set_rx_mode_async support ---
commit 1fc0ed8b56a0fd5a87f788e357261a7f2693625b
Author: Vikas Gupta <vikas.gupta@broadcom.com>

bnge: add ndo_set_rx_mode_async support

This commit registers bnge_set_rx_mode() as ndo_set_rx_mode_async to
handle filter updates asynchronously. It computes UC list updates directly
from the snapshot, allowing sleepable HWRM commands without holding the
address lock.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index 1e3cdaeaa03dc..e67536a8c4304 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c

[ ... ]

> @@ -2272,6 +2269,48 @@ static int bnge_cfg_rx_mode(struct bnge_net *bn, struct netdev_hw_addr_list *uc,
>  	return rc;
>  }
>  
> +static int bnge_set_rx_mode(struct net_device *dev,
> +			    struct netdev_hw_addr_list *uc,
> +			    struct netdev_hw_addr_list *mc)
> +{
> +	struct bnge_net *bn = netdev_priv(dev);
> +	struct bnge_vnic_info *vnic;

[ ... ]

> +	if (dev->flags & IFF_ALLMULTI) {
> +		mask |= CFA_L2_SET_RX_MASK_REQ_MASK_ALL_MCAST;
> +		vnic->mc_list_count = 0;
> +	} else if (dev->flags & IFF_MULTICAST) {
> +		mc_update = bnge_mc_list_updated(bn, &mask, mc);
> +	}
> +
> +	if (mask != vnic->rx_mask || uc_update || mc_update) {
> +		vnic->rx_mask = mask;
> +		return bnge_cfg_rx_mode(bn, uc, uc_update, true);
> +	}

[Severity: High]
If bnge_cfg_rx_mode() returns -EAGAIN, the network core schedules a retry 
timer. However, because vnic->rx_mask is eagerly updated here (and 
vnic->mc_list is mutated in-place inside bnge_mc_list_updated()), won't the 
subsequent retry falsely detect no state changes and return 0 without 
programming the hardware?

Could this leave the hardware filter state permanently desynchronized from 
the network stack?

> +
> +	return 0;
> +}

[ ... ]

> @@ -2704,7 +2743,7 @@ static int bnge_init_chip(struct bnge_net *bn)
>  		vnic->rx_mask |= mask;
>  	}
>  
> -	rc = bnge_cfg_rx_mode(bn, &bn->netdev->uc, false);
> +	rc = bnge_cfg_rx_mode(bn, &bn->netdev->uc, true, false);
>  	if (rc == -EAGAIN) {
>  		netif_rx_mode_schedule_retry(bn->netdev);
>  		rc = 0;

[Severity: High]
This is a pre-existing issue, but does iterating the live netdev multicast
list earlier in bnge_init_chip() require holding netif_addr_lock_bh()?

Just above this call in bnge_init_chip(), the code evaluates:
bnge_mc_list_updated(bn, &mask, &bn->netdev->mc)

Since this happens without netif_addr_lock_bh() held, could it race with 
concurrent multicast updates (such as dev_mc_add() from IGMP/MLD timers), 
potentially leading to a use-after-free or infinite loop during list 
traversal?