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

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

[Keith Busch]

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(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index cef5ce851a92..b110eac1d0e6 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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
-- 
2.14.3

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

[Alex G.]

Hi Keith,

> 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(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index cef5ce851a92..b110eac1d0e6 100644

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

> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -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

>  		/*
>  		 * If the controller is still alive tell it to stop using the
>  		 * host memory buffer.  In theory the shutdown / reset should
> 

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

[Keith Busch]

On Wed, Mar 28, 2018@03:13:37PM -0500, Alex G. wrote:
> 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:

---
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index b6f43b738f03..2459067d208b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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);
 
--

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

[Alex G.]

On 03/28/2018 03:22 PM, Keith Busch wrote:
> On Wed, Mar 28, 2018@03:13:37PM -0500, Alex G. wrote:
>> 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

> 
> A 4.16 port would would like this:
> 
> ---
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index b6f43b738f03..2459067d208b 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -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);
>  
> --
> 

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

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>