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

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

[Eduard Zateev]

nvme_dev_disable() skips nvme_disable_ctrl() when the controller reports
a fatal status or is not ready. PCI MMIO can still be accessible in this
state, allowing the controller to complete commands while the driver
cancels outstanding requests. This can race with blk-mq requeueing.

Separate PCI accessibility from controller health. Delete I/O queues
only for a healthy controller, but clear CC.EN whenever the PCI function
and channel remain accessible. Keep graceful shutdown and IRQ polling
limited to a healthy controller.

Reported-by: syzbot+f4353e96f7004ae5a09e@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=f4353e96f7004ae5a09e
Assisted-by: LLM
Signed-off-by: Eduard Zateev <hackerowskiy@gmail.com>
---
 drivers/nvme/host/pci.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 77317e5d00f..777615b4d25 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3296,14 +3296,19 @@ static void nvme_dev_unmap(struct nvme_dev *dev)
 	pci_release_mem_regions(to_pci_dev(dev->dev));
 }
 
-static bool nvme_pci_ctrl_is_dead(struct nvme_dev *dev)
+static bool nvme_pci_ctrl_is_accessible(struct nvme_dev *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
+
+	return pci_is_enabled(pdev) && pci_device_is_present(pdev) &&
+		pdev->error_state == pci_channel_io_normal;
+}
+
+static bool nvme_pci_ctrl_is_dead(struct nvme_dev *dev)
+{
 	u32 csts;
 
-	if (!pci_is_enabled(pdev) || !pci_device_is_present(pdev))
-		return true;
-	if (pdev->error_state != pci_channel_io_normal)
+	if (!nvme_pci_ctrl_is_accessible(dev))
 		return true;
 
 	csts = readl(dev->bar + NVME_REG_CSTS);
@@ -3331,10 +3336,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]);
 	}
 	nvme_suspend_io_queues(dev);
 	nvme_suspend_queue(dev, 0);
-- 
2.55.0

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

[sashiko-bot]

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