[PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Wenbo Wang]

If __nvme_submit_cmd races with nvme_dev_disable, nvmeq
could have been suspended and dev->bar could have been
unmapped. Do not touch sq door bell in this case.

Signed-off-by: Wenbo Wang <wenbo.wang@memblaze.com>
Reviewed-by: Wenwei Tao <wenwei.tao@memblaze.com>
CC: linux-nvme@lists.infradead.org
---
 drivers/nvme/host/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8b1a725..2288712 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -325,7 +325,8 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
 
 	if (++tail == nvmeq->q_depth)
 		tail = 0;
-	writel(tail, nvmeq->q_db);
+	if (likely(nvmeq->cq_vector >= 0))
+		writel(tail, nvmeq->q_db);
 	nvmeq->sq_tail = tail;
 }
 
-- 
1.8.3.1

RE: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Busch, Keith]

Does this ever happen? The queue should be stopped before the bar is unmapped. If that's insufficient to guard against this, we've another problem this patch does not cover. That command will just timeout since it was accepted by the driver, but not actually submitted to anything. The request needs to be requeued or return BLK_MQ_RQ_QUEUE_BUSY.

> -----Original Message-----
> From: Wenbo Wang [mailto:mail_weber_wang@163.com]
> Sent: Monday, February 01, 2016 8:42 AM
> To: axboe@fb.com; Busch, Keith
> Cc: linux-kernel@vger.kernel.org; Wenbo Wang; Wenbo Wang; linux-nvme@lists.infradead.org
> Subject: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended
> 
> If __nvme_submit_cmd races with nvme_dev_disable, nvmeq
> could have been suspended and dev->bar could have been
> unmapped. Do not touch sq door bell in this case.
> 
> Signed-off-by: Wenbo Wang <wenbo.wang@memblaze.com>
> Reviewed-by: Wenwei Tao <wenwei.tao@memblaze.com>
> CC: linux-nvme@lists.infradead.org
> ---
>  drivers/nvme/host/pci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 8b1a725..2288712 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -325,7 +325,8 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
> 
>  	if (++tail == nvmeq->q_depth)
>  		tail = 0;
> -	writel(tail, nvmeq->q_db);
> +	if (likely(nvmeq->cq_vector >= 0))
> +		writel(tail, nvmeq->q_db);
>  	nvmeq->sq_tail = tail;
>  }
> 
> --
> 1.8.3.1

RE: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Wenbo Wang]

No, this issue has not been seen yet. 

Since blk_mq_run_hw_queue tests queue stopped in the very beginning. It should be possible to race with nvme_dev_disable.

I agree that the request shall be re-queued.

-----Original Message-----
From: Busch, Keith [mailto:keith.busch@intel.com] 
Sent: Tuesday, February 2, 2016 12:00 AM
To: Wenbo Wang; axboe@fb.com
Cc: linux-kernel@vger.kernel.org; Wenbo Wang; linux-nvme@lists.infradead.org
Subject: RE: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

Does this ever happen? The queue should be stopped before the bar is unmapped. If that's insufficient to guard against this, we've another problem this patch does not cover. That command will just timeout since it was accepted by the driver, but not actually submitted to anything. The request needs to be requeued or return BLK_MQ_RQ_QUEUE_BUSY.

> -----Original Message-----
> From: Wenbo Wang [mailto:mail_weber_wang@163.com]
> Sent: Monday, February 01, 2016 8:42 AM
> To: axboe@fb.com; Busch, Keith
> Cc: linux-kernel@vger.kernel.org; Wenbo Wang; Wenbo Wang; 
> linux-nvme@lists.infradead.org
> Subject: [PATCH] NVMe: do not touch sq door bell if nvmeq has been 
> suspended
> 
> If __nvme_submit_cmd races with nvme_dev_disable, nvmeq could have 
> been suspended and dev->bar could have been unmapped. Do not touch sq 
> door bell in this case.
> 
> Signed-off-by: Wenbo Wang <wenbo.wang@memblaze.com>
> Reviewed-by: Wenwei Tao <wenwei.tao@memblaze.com>
> CC: linux-nvme@lists.infradead.org
> ---
>  drivers/nvme/host/pci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 
> 8b1a725..2288712 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -325,7 +325,8 @@ static void __nvme_submit_cmd(struct nvme_queue 
> *nvmeq,
> 
>  	if (++tail == nvmeq->q_depth)
>  		tail = 0;
> -	writel(tail, nvmeq->q_db);
> +	if (likely(nvmeq->cq_vector >= 0))
> +		writel(tail, nvmeq->q_db);
>  	nvmeq->sq_tail = tail;
>  }
> 
> --
> 1.8.3.1

Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Jens Axboe]

On 02/01/2016 08:42 AM, Wenbo Wang wrote:
> If __nvme_submit_cmd races with nvme_dev_disable, nvmeq
> could have been suspended and dev->bar could have been
> unmapped. Do not touch sq door bell in this case.
>
> Signed-off-by: Wenbo Wang <wenbo.wang@memblaze.com>
> Reviewed-by: Wenwei Tao <wenwei.tao@memblaze.com>
> CC: linux-nvme@lists.infradead.org
> ---
>   drivers/nvme/host/pci.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 8b1a725..2288712 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -325,7 +325,8 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
>
>   	if (++tail == nvmeq->q_depth)
>   		tail = 0;
> -	writel(tail, nvmeq->q_db);
> +	if (likely(nvmeq->cq_vector >= 0))
> +		writel(tail, nvmeq->q_db);
>   	nvmeq->sq_tail = tail;

What Keith said (this should not happen), and additionally, this won't 
work for a polled CQ without a vector.

-- 
Jens Axboe

Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Sagi Grimberg]

> Jens,
>
> I did the following test to validate the issue.
>
> 1. Modify code as below to increase the chance of races.
> 	Add 10s delay after nvme_dev_unmap() in nvme_dev_disable()
> 	Add 10s delay before __nvme_submit_cmd()
> 2. Run dd and at the same time, echo 1 to reset_controller to trigger device reset. Finally kernel crashes due to accessing unmapped door bell register.
>
> Following is the execution order of the two code paths:
> __blk_mq_run_hw_queue
>    Test BLK_MQ_S_STOPPED
> 					nvme_dev_disable()
> 					     nvme_stop_queues()  <-- set BLK_MQ_S_STOPPED
> 					     nvme_dev_unmap(dev)  <-- unmap door bell
>    nvme_queue_rq()
>        Touch door bell	<-- panic here

First of all, I think we need to cancel all
inflight requests before nvme_dev_unmap.

With my patches that move I/O termination to the
nvme core ([PATCH v1 0/3] Move active IO termination to the core)
the change needed is:
--
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e921165..2288bdb 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1890,10 +1890,11 @@ static void nvme_dev_shutdown(struct nvme_dev *dev)
                 nvme_shutdown_ctrl(&dev->ctrl);
                 nvme_disable_queue(dev, 0);
         }
-       nvme_dev_unmap(dev);

         blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_io, dev);
         blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_io, dev);
+
+       nvme_dev_unmap(dev);
  }

  static int nvme_setup_prp_pools(struct nvme_dev *dev)
--

But still we need a way to wait out for all active
queue_rq to end. It seems like we need to maintain
the request_fn_active for blk-mq and provide an
API (blk_mq_wait_for_active_requests ?) that waits for
it to drop to zero.

Thoughts?

Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Keith Busch]

On Tue, Feb 02, 2016 at 02:41:37PM +0200, Sagi Grimberg wrote:
> First of all, I think we need to cancel all
> inflight requests before nvme_dev_unmap.

IO cancelling is where it is because it protects against host memory
corruption. If you're going to mess with the ordering, just make sure
the PCI device is disabled from bus mastering first.

Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Sagi Grimberg]

Hey Keith,

>> First of all, I think we need to cancel all
>> inflight requests before nvme_dev_unmap.
>
> IO cancelling is where it is because it protects against host memory
> corruption. If you're going to mess with the ordering, just make sure
> the PCI device is disabled from bus mastering first.

Little help? :)

What corruption is the ordering protecting against?

Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Keith Busch]

On Tue, Feb 02, 2016 at 04:33:10PM +0200, Sagi Grimberg wrote:
> Hey Keith,
> 
> >>First of all, I think we need to cancel all
> >>inflight requests before nvme_dev_unmap.
> >
> >IO cancelling is where it is because it protects against host memory
> >corruption. If you're going to mess with the ordering, just make sure
> >the PCI device is disabled from bus mastering first.
> 
> Little help? :)
> 
> What corruption is the ordering protecting against?

Sure thing. :)

We free the transfer buffers when a command is cancelled. The controller,
however, may still own the command and may try to write to them. We
have to fence the controller off from being able to do that, so we can't
cancel inflight commands while the PCI device is still bus master enabled.

In a perfect world, we could trust in disabling with NVMe registers,
but sometimes we can't rely on that.

This was commit 07836e659c81ec6b0d683dfbf7958339a22a7b69, which might
explain the scenario a little better, and was reported by end user.

Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Sagi Grimberg]

> We free the transfer buffers when a command is cancelled. The controller,
> however, may still own the command and may try to write to them. We
> have to fence the controller off from being able to do that, so we can't
> cancel inflight commands while the PCI device is still bus master enabled.
>
> In a perfect world, we could trust in disabling with NVMe registers,
> but sometimes we can't rely on that.

OK, I wasn't aware that we cannot rely on that.

So it looks like we cannot change the ordering. So this leaves us with
the need to guarantee that no queue_rq is inflight before we unmap.

Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Keith Busch]

On Tue, Feb 02, 2016 at 07:15:57AM +0000, Wenbo Wang wrote:
> Jens,
> 
> I did the following test to validate the issue.
> 
> 1. Modify code as below to increase the chance of races.
> 	Add 10s delay after nvme_dev_unmap() in nvme_dev_disable()
> 	Add 10s delay before __nvme_submit_cmd()

If running sync IO, preempt is disabled. You can't just put a 10 second
delay there. Wouldn't you hit a "scheduling while atomic" bug instead?

If blk-mq is running the h/w context from its work queue, that might
be a different issue. Maybe we can change the "cancel_delayed_work" to
"cancel_delayed_work_sync" in blk_mq_stop_hw_queues.

If there's still a window where blk-mq can insert a request after the
driver requested to stop queues, I think we should try to close it with
the block layer.

Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Keith Busch]

On Tue, Feb 02, 2016 at 07:15:57AM +0000, Wenbo Wang wrote:
> I did the following test to validate the issue.
> 
> 1. Modify code as below to increase the chance of races.
> 	Add 10s delay after nvme_dev_unmap() in nvme_dev_disable()
> 	Add 10s delay before __nvme_submit_cmd()
> 2. Run dd and at the same time, echo 1 to reset_controller to trigger device reset. Finally kernel crashes due to accessing unmapped door bell register.
> 
> Following is the execution order of the two code paths:
> __blk_mq_run_hw_queue
>   Test BLK_MQ_S_STOPPED
> 					nvme_dev_disable()
> 					     nvme_stop_queues()  <-- set BLK_MQ_S_STOPPED
> 					     nvme_dev_unmap(dev)  <-- unmap door bell
>   nvme_queue_rq()
>       Touch door bell	<-- panic here

Does the following force the first to complete before the unmap?

---
@@ -1415,10 +1421,21 @@ void nvme_stop_queues(struct nvme_ctrl *ctrl)
 
 		blk_mq_cancel_requeue_work(ns->queue);
 		blk_mq_stop_hw_queues(ns->queue);
+		blk_sync_queue(ns->queue);
 	}
 	mutex_unlock(&ctrl->namespaces_mutex);
 }
--

Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Keith Busch]

On Wed, Feb 03, 2016 at 04:35:03PM +0000, Wenbo Wang wrote:
> For async io (executed by run_work worker) it should work. However for sync io in blk_mq_run_hw_queue, this seems not help, there is still a window.

Alright, for lack of a better way to sync a stopped queue, the driver's
indication looks like the best we can do at this point.

Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Sagi Grimberg]

> Keith,
>
> Is the following solution OK?
> synchronize_rcu guarantee that no queue_rq is running concurrently with device disable code.
> Together with your another patch (adding blk_sync_queue), both sync/async path shall be handled correctly.

This can be acceptable I think.

> Do you think synchronize_rcu shall be added to blk_sync_queue?

Or, we'll add it to blk_mq_stop_hw_queues() and then scsi
will enjoy it as well.

> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 4c0622f..bfe9132 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -865,7 +865,9 @@ void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
>          if (!async) {
>                  int cpu = get_cpu();
>                  if (cpumask_test_cpu(cpu, hctx->cpumask)) {
> +                       rcu_read_lock();
>                          __blk_mq_run_hw_queue(hctx);
> +                       rcu_read_unlock();

I think the rcu is better folded into __blk_mq_run_hw_queue
to cover all the call sites.

Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Keith Busch]

On Sat, Feb 06, 2016 at 02:32:24PM +0000, Wenbo Wang wrote:
> Keith,
> 
> Is the following solution OK?
> synchronize_rcu guarantee that no queue_rq is running concurrently with device disable code. Together with your another patch (adding blk_sync_queue), both sync/async path shall be handled correctly.
> 
> Do you think synchronize_rcu shall be added to blk_sync_queue?

I was nearly going to suggest the same last week, but it feels wrong since
no one takes rcu_read_lock in the path we're trying to sychronoize. Is
this safe if the task is interrupted?

Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

[Keith Busch]

On Tue, Feb 09, 2016 at 11:22:04AM +0000, Wenbo Wang wrote:
> In most cases, rcu read lock is just a preempt_disable, which is what get_cpu does. I don't see any risk.

Yes, many rcu_read_lock cases expand similarly to get_cpu. What about
the other cases?

FWIW, I don't think we'll hit the problem with the proposed rcu sync.
Heck, we don't even have a synchronize today and we don't hit the
theoretical problem.