Re: [net PATCH 1/4] fbnic: reset num_napi when the IRQ vectors are freed

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0

- [Medium] The patch encodes "datapath torn down" as `fbn->num_napi == 0`
  while deliberately leaving `netif_running(netdev)` true…

Pre-existing issues:
- [High] __fbnic_pm_attach()
  (drivers/net/ethernet/meta/fbnic/fbnic_pci.c) drops RTNL after…

--

fbnic: reset num_napi when the IRQ vectors are freed

This clears fbn->num_napi in fbnic_pm_suspend() after the datapath has
been torn down and the IRQs are about to be freed, and also clears it in
__fbnic_pm_resume() when __fbnic_open() fails, so the count-bounded
consumers such as fbnic_dbg_down() no longer walk freed napi vectors.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index 8b9bc9e8ea56..c6698e3002a1 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> @@ -434,6 +434,7 @@ static int fbnic_pm_suspend(struct device *dev)
>  {
>  	struct fbnic_dev *fbd = dev_get_drvdata(dev);
>  	struct net_device *netdev = fbd->netdev;
> +	struct fbnic_net *fbn;
>  
>  	if (fbnic_init_failure(fbd))
>  		goto null_uc_addr;
> @@ -441,11 +442,16 @@ static int fbnic_pm_suspend(struct device *dev)
>  	rtnl_lock();
>  	netdev_lock(netdev);
>  
> +	fbn = netdev_priv(netdev);
> +
>  	netif_device_detach(netdev);
>  
>  	if (netif_running(netdev))
>  		netdev->netdev_ops->ndo_stop(netdev);
>  
> +	/* The IRQs are about to be freed, so drop the napi vector count */
> +	fbn->num_napi = 0;
> +

[Severity: Medium]
This makes num_napi == 0 the marker for "datapath torn down" while
netif_running(netdev) is deliberately left true (ndo_stop is called
directly, so __LINK_STATE_START stays set).  Do the netdev queue
management ops cope with that combination?

fbnic_queue_mem_alloc() in fbnic_txrx.c uses netif_running() as the test
for "the napi vectors exist" and then divides by the count:

	if (!netif_running(dev))
		return fbnic_alloc_qt_page_pools(fbn, qt, idx);

	real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
	nv = fbn->napi[idx % fbn->num_napi];

fbnic_queue_start() and fbnic_queue_stop() do the same modulo with no
guard at all:

	real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
	nv = fbn->napi[idx % fbn->num_napi];

With num_napi == 0, is idx % fbn->num_napi a divide error oops?

These ops look reachable in that state.  fbnic_netdev_alloc() sets
netdev->queue_mgmt_ops = &fbnic_queue_mgmt_ops, so a memory provider can
be bound to an fbnic Rx queue (io_uring zcrx via netif_mp_open_rxq(), or
devmem).  On teardown of the io_uring fd or the netlink socket,
__netif_mp_close_rxq() -> netdev_rx_queue_reconfig() in
net/core/netdev_rx_queue.c calls ndo_queue_mem_alloc unconditionally and
ndo_queue_stop/ndo_queue_start under netif_running(dev), holding only the
netdev instance lock and doing no netif_device_present() check, unlike
the netdev-genl bind entry points.  real_num_rx_queues is not reset by
ndo_stop, so the index checks still pass.

The window is not short: userspace is not frozen during PCIe error
recovery, and the state persists indefinitely when __fbnic_open() fails
on resume or when fbnic_err_slot_reset() returns
PCI_ERS_RESULT_DISCONNECT so .resume and __fbnic_pm_attach() never run.

For completeness, the same sequence already oopsed before this patch,
since fbnic_free_napi_vector() sets fbn->napi[i] = NULL and
fbnic_remove_rx_ring() sets fbn->rx[q_idx] = NULL, so the pre-patch
crash was a NULL dereference rather than a divide error.  The question is
whether the torn-down state can be made unambiguous for all consumers,
for example by gating the queue management ops on
netif_device_present() or on an explicit datapath-up flag, rather than
only fixing the count-bounded consumers.

>  	netdev_unlock(netdev);
>  	rtnl_unlock();
>  
> @@ -508,16 +514,20 @@ static int __fbnic_pm_resume(struct device *dev)
>  	if (fbnic_init_failure(fbd))
>  		return 0;
>  
> +	rtnl_lock();
> +	netdev_lock(netdev);
> +
>  	fbn = netdev_priv(netdev);
>  
>  	/* Reset the queues if needed */
>  	fbnic_reset_queues(fbn, fbn->num_tx_queues, fbn->num_rx_queues);
>  
> -	rtnl_lock();
> -	netdev_lock(netdev);
> -
> -	if (netif_running(netdev))
> +	if (netif_running(netdev)) {
>  		err = __fbnic_open(fbn);
> +		/* On failure the vectors are freed, so drop the count */
> +		if (err)
> +			fbn->num_napi = 0;
> +	}
>  
>  	netdev_unlock(netdev);
>  	rtnl_unlock();

[Severity: High]
This isn't a bug introduced by this patch, but while the resume path is
being reworked to take rtnl_lock() plus netdev_lock() around the
netif_running() test, __fbnic_pm_attach() in the same file still does the
equivalent test with neither lock held:

	rtnl_lock();
	fbnic_reset_hw_stats(fbd);
	rtnl_unlock();
	...
	fbn = netdev_priv(netdev);

	if (netif_running(netdev))
		fbnic_up(fbn);

	netif_device_attach(netdev);

Can a close slip in between the netif_running() test and fbnic_up()?
__dev_close_many() in net/core/dev.c explicitly allows ndo_stop after a
detach:

		/*
		 *	Call the device specific close. This cannot fail.
		 *	Only if device is UP
		 *
		 *	We allow it to be called even after a DETACH hot-plug
		 *	event.
		 */
		if (ops->ndo_stop)
			ops->ndo_stop(dev);

so fbnic_stop() -> fbnic_free_napi_vectors() can NULL and kfree() every
entry of fbn->napi[] while fbn->num_napi keeps the normal queue count.
fbnic_up() -> fbnic_napi_enable() then walks that array:

	for (i = 0; i < fbn->num_napi; i++) {
		struct fbnic_napi_vector *nv = fbn->napi[i];

		napi_enable_locked(&nv->napi);

Is that a NULL dereference, or a use-after-free of the vector, when the
close won the race?

The path is fbnic_err_slot_reset() succeeding -> fbnic_err_resume() ->
__fbnic_pm_attach(), which runs from the PCI recovery context with
userspace not frozen, and that context takes only device_lock, never
rtnl or netdev->lock, so an "ip link set dev X down" on another CPU is
free to run concurrently.  Would it make sense to serialize the
netif_running() check and the dependent fbnic_up() under the same
rtnl_lock() and netdev_lock() pair now used by suspend and resume?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/178838795612.11136.15512516427273655916.stgit%40ahduyck-xeon-server.home.arpa