Re: [PATCH] nvme-pci: disable accessible controller on error paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvme-pci: skipped nvme_dev_disable() in nvme_reset_work() after dead suspend
--

commit e486876c5c5632e71a9ede664c55c8b5d653c958
Author: Eduard Zateev <hackerowskiy@gmail.com>

nvme-pci: disable accessible controller on error paths

This commit separates PCI accessibility checks from controller health in
nvme_dev_disable() so that nvme_disable_ctrl() is called whenever the PCI
device remains accessible, even if the controller reports a fatal status
or is not ready.

Link: https://syzkaller.appspot.com/bug?extid=f4353e96f7004ae5a09e

> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index ea552ac5501f6..bb8e84f062db7 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c

[ ... ]

> @@ -3552,10 +3557,16 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>  
>  	nvme_quiesce_io_queues(&dev->ctrl);
>  
> -	if (!dead && dev->ctrl.queue_count > 0) {
> +	if (!dead && dev->ctrl.queue_count > 0)
>  		nvme_delete_io_queues(dev);
> -		nvme_disable_ctrl(&dev->ctrl, shutdown);
> -		nvme_poll_irqdisable(&dev->queues[0]);
> +	if (dev->ctrl.queue_count > 0 && nvme_pci_ctrl_is_accessible(dev)) {
> +		/*
> +		 * Stop an accessible controller even if it reports itself dead.
> +		 * Otherwise it may complete requests after they have been cancelled.
> +		 */
> +		nvme_disable_ctrl(&dev->ctrl, shutdown && !dead);
> +		if (!dead)
> +			nvme_poll_irqdisable(&dev->queues[0]);
>  	}

[Severity: High]
Can calling nvme_disable_ctrl(&dev->ctrl, shutdown && !dead) here leave
I/O queues unquiesced during nvme_reset_work() when resuming from a
suspend where the controller was dead?

During system suspend, nvme_suspend() and nvme_simple_suspend() call
nvme_disable_prepare_reset(ndev, true) -> nvme_dev_disable(ndev, true).
If the controller reports NVME_CSTS_CFS or !NVME_CSTS_RDY while PCI MMIO
is still accessible, dead is true and nvme_disable_ctrl(&dev->ctrl, false)
clears NVME_CC_ENABLE in dev->ctrl.ctrl_config:

drivers/nvme/host/core.c:nvme_disable_ctrl() {
	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
	if (shutdown)
		ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
	else
		ctrl->ctrl_config &= ~NVME_CC_ENABLE;
...
}

At the end of nvme_dev_disable(dev, true), the shutdown block still runs
because shutdown is true, which unquiesces the I/O and admin queues:

drivers/nvme/host/pci.c:nvme_dev_disable() {
...
	if (shutdown) {
		nvme_unquiesce_io_queues(&dev->ctrl);
		if (dev->ctrl.admin_q && !blk_queue_dying(dev->ctrl.admin_q))
			nvme_unquiesce_admin_queue(&dev->ctrl);
	}
...
}

When resuming via nvme_resume() or nvme_simple_resume() ->
nvme_try_sched_reset() -> nvme_reset_work(), nvme_reset_work() checks
NVME_CC_ENABLE before calling nvme_dev_disable(dev, false):

drivers/nvme/host/pci.c:nvme_reset_work() {
...
	if (dev->ctrl.ctrl_config & NVME_CC_ENABLE)
		nvme_dev_disable(dev, false);
	nvme_sync_queues(&dev->ctrl);
...
}

Previously, NVME_CC_ENABLE remained set in dev->ctrl.ctrl_config across
nvme_dev_disable(dev, true), so nvme_reset_work() always called
nvme_dev_disable(dev, false) and re-quiesced the I/O queues via
nvme_quiesce_io_queues(&dev->ctrl).

With NVME_CC_ENABLE cleared during suspend when dead is true, won't
nvme_reset_work() skip nvme_dev_disable(dev, false) and leave the I/O
queues unquiesced while resetting the controller and setting up queues,
allowing entered or requeued requests to be dispatched before queue setup
and nvme_dbbuf_set() complete?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023101825.139040-1-hackerowskiy@gmail.com?part=1