[PATCH] nvme-pci: fix race between pci reset and nvme probe

[PATCH] nvme-pci: fix race between pci reset and nvme probe

[Ming Lei]

After nvme_probe() returns, device lock is released, and PCI reset
handler may come, meantime reset work is just scheduled and should
be in-progress.

When nvme_reset_prepare() is run, all NSs may not be allocated yet
and each NS's request queue won't be frozen by nvme_dev_disable().

But when nvme_reset_done() is called for resetting controller, all
NSs may have been scanned successfully, and nvme_wait_freeze() is
called on un-frozen request queues, then wait forever.

Fix the issue by holding device lock for resetting from nvme probe.

Reported-by: Yi Zhang <yi.zhang@redhat.com>
Link: https://lore.kernel.org/linux-block/CAHj4cs--KPTAGP=jj+7KMe=arDv=HeGeOgs1T8vbusyk=EjXow@mail.gmail.com/#r
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/host/pci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 4232192e10dd..d49b1a082983 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3075,9 +3075,14 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
 static void nvme_async_probe(void *data, async_cookie_t cookie)
 {
 	struct nvme_dev *dev = data;
+	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
+	pci_dev_lock(pdev);
+	nvme_reset_ctrl(&dev->ctrl);
 	flush_work(&dev->ctrl.reset_work);
 	flush_work(&dev->ctrl.scan_work);
+	pci_dev_unlock(pdev);
+
 	nvme_put_ctrl(&dev->ctrl);
 }
 
@@ -3154,7 +3159,6 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	dev_info(dev->ctrl.device, "pci function %s\n", dev_name(&pdev->dev));
 
-	nvme_reset_ctrl(&dev->ctrl);
 	async_schedule(nvme_async_probe, dev);
 
 	return 0;
-- 
2.31.1

Re: [PATCH] nvme-pci: fix race between pci reset and nvme probe

[Keith Busch]

On Mon, Aug 01, 2022 at 08:57:53PM +0800, Ming Lei wrote:
> After nvme_probe() returns, device lock is released, and PCI reset
> handler may come, meantime reset work is just scheduled and should
> be in-progress.
> 
> When nvme_reset_prepare() is run, all NSs may not be allocated yet
> and each NS's request queue won't be frozen by nvme_dev_disable().
> 
> But when nvme_reset_done() is called for resetting controller, all
> NSs may have been scanned successfully, and nvme_wait_freeze() is
> called on un-frozen request queues, then wait forever.
> 
> Fix the issue by holding device lock for resetting from nvme probe.
> 
> Reported-by: Yi Zhang <yi.zhang@redhat.com>
> Link: https://lore.kernel.org/linux-block/CAHj4cs--KPTAGP=jj+7KMe=arDv=HeGeOgs1T8vbusyk=EjXow@mail.gmail.com/#r
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/nvme/host/pci.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 4232192e10dd..d49b1a082983 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -3075,9 +3075,14 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
>  static void nvme_async_probe(void *data, async_cookie_t cookie)
>  {
>  	struct nvme_dev *dev = data;
> +	struct pci_dev *pdev = to_pci_dev(dev->dev);
>  
> +	pci_dev_lock(pdev);
> +	nvme_reset_ctrl(&dev->ctrl);
>  	flush_work(&dev->ctrl.reset_work);
>  	flush_work(&dev->ctrl.scan_work);
> +	pci_dev_unlock(pdev);
> +
>  	nvme_put_ctrl(&dev->ctrl);
>  }

When low on memory, async_schedule() falls back to calling the requested
function directly, so this would deadlock on taking the pci_dev_lock() the
second time within the probe context.

If it is successfully scheduled asynchronously, holding the lock blocks a hot
removal, which might be the only thing that can unblock the nvme reset_work
from forward progress.

If you are encountering a nvme_reset_prepare() condition during scanning, that
might indicate a failure to communicate with the end device. The scan work may
need the error handling to unblock it.

> @@ -3154,7 +3159,6 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	dev_info(dev->ctrl.device, "pci function %s\n", dev_name(&pdev->dev));
>  
> -	nvme_reset_ctrl(&dev->ctrl);
>  	async_schedule(nvme_async_probe, dev);
>  
>  	return 0;
> -- 
> 2.31.1
> 

Re: [PATCH] nvme-pci: fix race between pci reset and nvme probe

[Ming Lei]

On Mon, Aug 01, 2022 at 08:33:01AM -0600, Keith Busch wrote:
> On Mon, Aug 01, 2022 at 08:57:53PM +0800, Ming Lei wrote:
> > After nvme_probe() returns, device lock is released, and PCI reset
> > handler may come, meantime reset work is just scheduled and should
> > be in-progress.
> > 
> > When nvme_reset_prepare() is run, all NSs may not be allocated yet
> > and each NS's request queue won't be frozen by nvme_dev_disable().
> > 
> > But when nvme_reset_done() is called for resetting controller, all
> > NSs may have been scanned successfully, and nvme_wait_freeze() is
> > called on un-frozen request queues, then wait forever.
> > 
> > Fix the issue by holding device lock for resetting from nvme probe.
> > 
> > Reported-by: Yi Zhang <yi.zhang@redhat.com>
> > Link: https://lore.kernel.org/linux-block/CAHj4cs--KPTAGP=jj+7KMe=arDv=HeGeOgs1T8vbusyk=EjXow@mail.gmail.com/#r
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/nvme/host/pci.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> > index 4232192e10dd..d49b1a082983 100644
> > --- a/drivers/nvme/host/pci.c
> > +++ b/drivers/nvme/host/pci.c
> > @@ -3075,9 +3075,14 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
> >  static void nvme_async_probe(void *data, async_cookie_t cookie)
> >  {
> >  	struct nvme_dev *dev = data;
> > +	struct pci_dev *pdev = to_pci_dev(dev->dev);
> >  
> > +	pci_dev_lock(pdev);
> > +	nvme_reset_ctrl(&dev->ctrl);
> >  	flush_work(&dev->ctrl.reset_work);
> >  	flush_work(&dev->ctrl.scan_work);
> > +	pci_dev_unlock(pdev);
> > +
> >  	nvme_put_ctrl(&dev->ctrl);
> >  }
> 
> When low on memory, async_schedule() falls back to calling the requested
> function directly, so this would deadlock on taking the pci_dev_lock() the
> second time within the probe context.

Good catch, maybe async_schedule() should provide one variation by
returning failure in case of OOM.

> 
> If it is successfully scheduled asynchronously, holding the lock blocks a hot
> removal, which might be the only thing that can unblock the nvme reset_work
> from forward progress.

OK, adding one reset or pci_reset lock may avoid to affect hot removal.

> 
> If you are encountering a nvme_reset_prepare() condition during scanning, that
> might indicate a failure to communicate with the end device. The scan work may
> need the error handling to unblock it.

Not sure I get your point. nvme_reset_prepare() is called from storing
to pci device's reset sysfs attr, so it can come any time, including
scanning.

Thanks,
Ming

Re: [PATCH] nvme-pci: fix race between pci reset and nvme probe

[Keith Busch]

On Tue, Aug 02, 2022 at 08:05:09AM +0800, Ming Lei wrote:
> > 
> > If you are encountering a nvme_reset_prepare() condition during scanning, that
> > might indicate a failure to communicate with the end device. The scan work may
> > need the error handling to unblock it.
> 
> Not sure I get your point. nvme_reset_prepare() is called from storing
> to pci device's reset sysfs attr, so it can come any time, including
> scanning.

Oh, I mean these .reset_prepare/done callbacks are also used by the PCIe AER
driver when the root port sees an uncorrectable error. In some cases, you won't
be able to successfuly use the PCIe bus downstream that root port until after
resetting it. I was just considering that scan_work may be stuck because of
such an error.

Re: [PATCH] nvme-pci: fix race between pci reset and nvme probe

[Keith Busch]

On Mon, Aug 01, 2022 at 08:57:53PM +0800, Ming Lei wrote:
> After nvme_probe() returns, device lock is released, and PCI reset
> handler may come, meantime reset work is just scheduled and should
> be in-progress.
> 
> When nvme_reset_prepare() is run, all NSs may not be allocated yet
> and each NS's request queue won't be frozen by nvme_dev_disable().
> 
> But when nvme_reset_done() is called for resetting controller, all
> NSs may have been scanned successfully, and nvme_wait_freeze() is
> called on un-frozen request queues, then wait forever.
> 
> Fix the issue by holding device lock for resetting from nvme probe.

Beyond the issues with locking the pci device, I think the problem you're
describing is generic to any rescan that alters the namespace inventory, so we
can't resolve it only on probe.

Is it enough to track the frozen status within the namespace flags? Something
like the following:

---
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 2429b11eb9a8..befd156d57c1 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5027,8 +5027,11 @@ void nvme_unfreeze(struct nvme_ctrl *ctrl)
 	struct nvme_ns *ns;
 
 	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
+		if (!test_and_clear_bit(NVME_NS_FREEZE, &ns->flags))
+			continue;
 		blk_mq_unfreeze_queue(ns->queue);
+	}
 	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_unfreeze);
@@ -5039,6 +5042,8 @@ int nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout)
 
 	down_read(&ctrl->namespaces_rwsem);
 	list_for_each_entry(ns, &ctrl->namespaces, list) {
+		if (!test_bit(NVME_NS_FREEZE, &ns->flags))
+			continue;
 		timeout = blk_mq_freeze_queue_wait_timeout(ns->queue, timeout);
 		if (timeout <= 0)
 			break;
@@ -5053,8 +5058,11 @@ void nvme_wait_freeze(struct nvme_ctrl *ctrl)
 	struct nvme_ns *ns;
 
 	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
+		if (!test_bit(NVME_NS_FREEZE, &ns->flags))
+			continue;
 		blk_mq_freeze_queue_wait(ns->queue);
+	}
 	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_wait_freeze);
@@ -5064,8 +5072,10 @@ void nvme_start_freeze(struct nvme_ctrl *ctrl)
 	struct nvme_ns *ns;
 
 	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
+		set_bit(NVME_NS_FREEZE, &ns->flags);
 		blk_freeze_queue_start(ns->queue);
+	}
 	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_start_freeze);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bdc0ff7ed9ab..872237b4b65d 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -484,6 +484,7 @@ struct nvme_ns {
 #define NVME_NS_FORCE_RO	3
 #define NVME_NS_READY		4
 #define NVME_NS_STOPPED		5
+#define NVME_NS_FREEZE		6
 
 	struct cdev		cdev;
 	struct device		cdev_device;
--

Re: [PATCH] nvme-pci: fix race between pci reset and nvme probe

[Ming Lei]

On Thu, Aug 04, 2022 at 03:55:14PM -0600, Keith Busch wrote:
> On Mon, Aug 01, 2022 at 08:57:53PM +0800, Ming Lei wrote:
> > After nvme_probe() returns, device lock is released, and PCI reset
> > handler may come, meantime reset work is just scheduled and should
> > be in-progress.
> > 
> > When nvme_reset_prepare() is run, all NSs may not be allocated yet
> > and each NS's request queue won't be frozen by nvme_dev_disable().
> > 
> > But when nvme_reset_done() is called for resetting controller, all
> > NSs may have been scanned successfully, and nvme_wait_freeze() is
> > called on un-frozen request queues, then wait forever.
> > 
> > Fix the issue by holding device lock for resetting from nvme probe.
> 
> Beyond the issues with locking the pci device, I think the problem you're
> describing is generic to any rescan that alters the namespace inventory, so we
> can't resolve it only on probe.

Yeah, just it is easier to trigger during the 1st scan when no any NS
exits.

> 
> Is it enough to track the frozen status within the namespace flags? Something
> like the following:

I guess the patch could avoid the issue.

Another way might be to remove nvme_wait_freeze() from nvme_reset_work(). No
see it is necessary:

- nvme_dev_add():

if (!dev->ctrl.tagset) {
	...
} else {
	blk_mq_update_nr_hw_queues(&dev->tagset, dev->online_queues - 1);
	...
} 

If there isn't tagset, there can't be IO; and if nr_hw_queues is to be
changed really, blk_mq_update_nr_hw_queues() will freeze queue, otherwise
it isn't necessary to nvme_wait_freeze() before calling
blk_mq_update_nr_hw_queues.



Thanks,
Ming