Skip to contentSkip to conversation

nvme-pci: Skip queue deletion if there are no queues

PATCH New Keith Busch 3 participants 5 messages Mar 28, 2018 – Apr 4, 2018 1 message not in the archive NVMe

lore
Share & tools
lore

Keith Busch

author
#0 lore raw

PATCH 1/1 nvme-pci: Skip queue deletion if there are no queues

User reported controller always retains CSTS.RDY to 1, which fails controller disabling when resetting the controller. This is also before the admin queue is allocated, and trying to disable an unallocated queue results in a NULL dereference.

Reported-by Alex Gagniuc <Alex_Gagniuc at Dellteam.com>
Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/pci.c +1 -1
index cef5ce851a92..b110eac1d0e6 100644
@@ -2200,7 +2200,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 
 	nvme_stop_queues(&dev->ctrl);
 
-	if (!dead) {
+	if (!dead && dev->ctrl.queue_count > 0) {
 		/*
 		 * If the controller is still alive tell it to stop using the
 		 * host memory buffer.  In theory the shutdown / reset should
Signature · 1 line
2.14.3

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw

PATCH 1/1 nvme-pci: Skip queue deletion if there are no queues

Hi Keith,

drivers/nvme/host/pci.c

Quoted 13 lines

User reported controller always retains CSTS.RDY to 1, which fails controller disabling when resetting the controller. This is also before the admin queue is allocated, and trying to disable an unallocated queue results in a NULL dereference.

Reported-by Alex Gagniuc <Alex_Gagniuc at Dellteam.com>
Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/pci.c +0 -0
index cef5ce851a92..b110eac1d0e6 100644

Which branch am I supposed to be using to test this? It doesn't apply to mainline.

drivers/nvme/host/pci.c static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown) · line 2203

Quoted 8 lines
drivers/nvme/host/pci.c +1 -1
@@ -2200,7 +2200,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 
 	nvme_stop_queues(&dev->ctrl);
 
-	if (!dead) {
+	if (!dead && dev->ctrl.queue_count > 0) {

Is this around the calls to nvme_disable_io_queues() and nvme_disable_admin_queue()?
Coming as an outsider with limited experience regarding the nvme driver it seems it might be more robust to move this check to nvme_disable_admin_queue().

Alex

Quoted 4 lines
drivers/nvme/host/pci.c +0 -0
 		/*
 		 * If the controller is still alive tell it to stop using the
 		 * host memory buffer.  In theory the shutdown / reset should
author
#3 lore raw

PATCH 1/1 nvme-pci: Skip queue deletion if there are no queues

Quoted 2 lines · Alex G.

Which branch am I supposed to be using to test this? It doesn't apply to mainline.

This is targeted to the 4.17 staging trees (sorry, this doesn't seem to be a good 4.16 candidate this late in the game). This should apply on linux-block for-4.17/block here:

https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/log/?h=for-4.17/block

A 4.16 port would would like this:

---

drivers/nvme/host/pci.c +1 -1
index b6f43b738f03..2459067d208b 100644
@@ -2194,7 +2194,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	 * Give the controller a chance to complete all entered requests if
 	 * doing a safe shutdown.
 	 */
-	if (!dead) {
+	if (!dead && dev->ctrl.queue_count > 0) {
 		if (shutdown)
 			nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);

--

#4 lore raw

PATCH 1/1 nvme-pci: Skip queue deletion if there are no queues

Quoted 9 lines · Keith Busch
Quoted 2 lines · Alex G.

Which branch am I supposed to be using to test this? It doesn't apply to mainline.

This is targeted to the 4.17 staging trees (sorry, this doesn't seem to be a good 4.16 candidate this late in the game). This should apply on linux-block for-4.17/block here:

https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/log/?h=for-4.17/block

Then I should probably test with that.
Tested-by: Alex Gagniuc <Alex_Gagniuc at Dellteam.com>

So I see the check is really meant to protect dereferencing &dev->queues[0] in nvme_disable_admin_queue(). My previous comment is still valid then. Successfully tested this approach as well.

Thanks for the quick turn-around.

Alex

Quoted 19 lines

A 4.16 port would would like this:

---

drivers/nvme/host/pci.c +1 -2
index b6f43b738f03..2459067d208b 100644
@@ -2194,7 +2194,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	 * Give the controller a chance to complete all entered requests if
 	 * doing a safe shutdown.
 	 */
-	if (!dead) {
+	if (!dead && dev->ctrl.queue_count > 0) {
 		if (shutdown)
 			nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);
 
--