Re: [PATCH net-next v5 05/14] fbnic: 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.
---
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.

>  	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?

>  
>  	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?

>  
>  	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.