Re: [PATCH net 2/3] ionic: detach the netdev in the PCI reset handler

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ionic: detach the netdev in the PCI reset handler

Adds a netif_device_detach() call to ionic_reset_prepare() right after
IONIC_LIF_F_FW_RESET is set, so the netdev is marked not-present before the
PCI reset path tears down the queues, the BARs and the device state. The
change mirrors what ionic_lif_handle_fw_down() does on the firmware recovery
path, and relies on ionic_restart_lif() to re-attach afterwards.

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> index 05f19489ec5c8..c15c4c705155e 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> @@ -447,6 +447,7 @@ static void ionic_reset_prepare(struct pci_dev *pdev)
>  	dev_dbg(ionic->dev, "%s: device stopping\n", __func__);
>  
>  	set_bit(IONIC_LIF_F_FW_RESET, lif->state);
> +	netif_device_detach(lif->netdev);

[Severity: Medium]
Is the premise in the changelog accurate?  The message says
ionic_reset_prepare() "leaves the netdev attached and registered for the
whole reset window", but ionic_reset_prepare() already reaches
ionic_stop_queues_reconfig(), whose first statement is a detach:

drivers/net/ethernet/pensando/ionic/ionic_lif.c:
void ionic_stop_queues_reconfig(struct ionic_lif *lif)
{
	/* Stop and clean the queues before reconfiguration */
	netif_device_detach(lif->netdev);
	ionic_stop_queues(lif);
	ionic_txrx_deinit(lif);
}

That call runs before every destructive step in ionic_reset_prepare()
(ionic_txrx_free(), ionic_lif_deinit(), ionic_qcqs_free(),
ionic_debugfs_del_lif(), ionic_dev_teardown(), ionic_clear_pci()), and well
before ionic_reset_done() -> ionic_setup_one() -> ionic_port_init() runs.

netif_device_detach() clears __LINK_STATE_PRESENT unconditionally:

net/core/dev.c:
	if (test_and_clear_bit(__LINK_STATE_PRESENT, &dev->state) &&
	    netif_running(dev)) {
		netif_tx_stop_all_queues(dev);
	}

so the presence gate was already closed before the teardown even for an
interface that is not up.

The only window the new call additionally covers is:

	set_bit(IONIC_LIF_F_FW_RESET, lif->state);
	netif_device_detach(lif->netdev);
	timer_delete_sync(&ionic->watchdog_timer);
	cancel_work_sync(&lif->deferred.work);
	ionic_auxbus_unregister(ionic->lif);
	mutex_lock(&lif->queue_lock);

None of those steps free idev->port_info or unmap the BARs, so the
ionic_port_init() free named in the message does not appear to be the
exposure that moves here.  Would it be more accurate to describe this as
making ionic_reset_prepare() consistent with
ionic_lif_handle_fw_down() (detach up front, before the auxbus unregister
and the queue_lock section), and to reconsider the
Fixes: a79b559e99be tag so stable does not pick this up as the fix for the
described use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but since the changelog presents
the patch as closing the port_info use-after-free, is the added detach
sufficient to close it?

The ethtool core checks presence once, before dispatching to the driver:

net/ethtool/ioctl.c:dev_ethtool_locked() {
	if (!netif_device_present(dev)) {
		rc = -ENODEV;
		goto out;
	}
	...
}

ethnl_ops_begin() does the same on the netlink path.  netif_device_detach()
is only a test_and_clear_bit() plus netif_tx_stop_all_queues(), with no wait
or drain, so a caller already past that check keeps running.

ionic_reset_prepare() is called from the PCI core holding only device_lock;
it takes neither rtnl_lock() nor the netdev instance/ops lock that the
ethtool callbacks run under.  Can the following interleaving still happen?

	ethtool GLINKSETTINGS			PCI reset
	dev_ethtool_locked()
	  netif_device_present() -> true
	  ionic_get_link_ksettings()
	    reads idev->port_info		ionic_reset_prepare()
						  netif_device_detach()
						  ionic_clear_pci()
						  ionic_dev_teardown()
						ionic_reset_done()
						  ionic_setup_one()
						    ionic_port_init()
	    (still dereferencing)		      dma_free_coherent(...
						        idev->port_info)

ionic_clear_pci() also drops the register pointers an in-flight op may be
using:

drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c:ionic_clear_pci() {
	if (ionic->num_bars) {
		ionic->idev.dev_info_regs = NULL;
		ionic->idev.dev_cmd_regs = NULL;
		ionic->idev.intr_status = NULL;
		ionic->idev.intr_ctrl = NULL;

		ionic_unmap_bars(ionic);
		pci_release_regions(ionic->pdev);
	}
	...
}

and ionic_dev_teardown() frees idev->cmb_inuse, destroys ionic->wq and
mutex_destroy()s idev->cmb_inuse_lock while the netdev is still registered.

The NULL test on idev->port_info in ionic_get_link_ksettings(),
ionic_get_pauseparam() and ionic_get_fecparam(), and the
IONIC_LIF_F_FW_RESET test in the set-ops, are check-then-use against a
teardown path that holds no lock in common with them, so the new detach adds
a second advisory gate with the same property.

Would serializing the detach and the whole teardown/re-setup under the lock
the ethtool and ndo callbacks already hold (rtnl_lock() /
netdev_lock_ops()), or protecting idev->port_info with a lock taken on both
sides, be needed to actually close this?

>  
>  	timer_delete_sync(&ionic->watchdog_timer);
>  	cancel_work_sync(&lif->deferred.work);