[BUG] nvme driver crash

[BUG] nvme driver crash

[Shaohua Li]

Disable CONIFG_SMP, kernel crashes at boot time, here is the log.

[    9.593590] nvme 0000:00:03.0: can't allocate MSI-X affinity masks for 1 vectors
[    9.595189] ==================================================================
[    9.595834] BUG: KASAN: null-ptr-deref in blk_mq_init_queue+0x1c/0x70
[    9.596010] Read of size 4 at addr 0000000000000020 by task kworker/u2:0/5
[    9.596010]
[    9.596010] CPU: 0 PID: 5 Comm: kworker/u2:0 Not tainted 4.13.0-rc1+ #1
[    9.596010] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.1-5.el7_3.3 04/01/2014
[    9.596010] Workqueue: nvme-wq nvme_scan_work
[    9.596010] Call Trace:
[    9.596010]  dump_stack+0x19/0x24
[    9.596010]  kasan_report+0xe8/0x350
[    9.596010]  __asan_load4+0x78/0x80
[    9.596010]  blk_mq_init_queue+0x1c/0x70
[    9.596010]  nvme_validate_ns+0x17c/0x620
[    9.596010]  ? nvme_revalidate_disk+0xf0/0xf0
[    9.596010]  ? nvme_submit_sync_cmd+0x30/0x30

Re: [BUG] nvme driver crash

[Christoph Hellwig]

On Tue, Jul 25, 2017 at 03:24:08PM -0700, Shaohua Li wrote:
> Disable CONIFG_SMP, kernel crashes at boot time, here is the log.

I can reproduce the issue.  Unfortunately the addresss in the bug
doesn't make any sense to me when resolving it using gdb, as t just
points to the line where blk_mq_init_queue calls
blk_alloc_queue_node.

Can you check if you get better results in your build?

Re: [BUG] nvme driver crash

[Omar Sandoval]

On Wed, Jul 26, 2017 at 10:34:43AM -0700, Christoph Hellwig wrote:
> On Tue, Jul 25, 2017 at 03:24:08PM -0700, Shaohua Li wrote:
> > Disable CONIFG_SMP, kernel crashes at boot time, here is the log.
> 
> I can reproduce the issue.  Unfortunately the addresss in the bug
> doesn't make any sense to me when resolving it using gdb, as t just
> points to the line where blk_mq_init_queue calls
> blk_alloc_queue_node.
> 
> Can you check if you get better results in your build?

It's crashing on that line because ctrl->tagset is NULL, which is
because...

irq_create_affinity_masks() returns NULL on !CONFIG_SMP
-> pci_irq_get_affinity() returns NULL
-> blk_mq_pci_map_queues() returns -EINVAL
-> blk_mq_alloc_tag_set() returns -EINVAL
-> nvme_dev_add() doesn't set ctrl->tagset

The two-fold fix would be to make the nvme driver handle
blk_mq_alloc_tag_set() failing and to fall back to a dumb mapping in
blk_mq_pci_map_queues(), but I don't know what the best way to do those
is.

Re: [BUG] nvme driver crash

[Shaohua Li]

On Wed, Jul 26, 2017 at 12:12:21PM -0700, Omar Sandoval wrote:
> On Wed, Jul 26, 2017 at 10:34:43AM -0700, Christoph Hellwig wrote:
> > On Tue, Jul 25, 2017 at 03:24:08PM -0700, Shaohua Li wrote:
> > > Disable CONIFG_SMP, kernel crashes at boot time, here is the log.
> > 
> > I can reproduce the issue.  Unfortunately the addresss in the bug
> > doesn't make any sense to me when resolving it using gdb, as t just
> > points to the line where blk_mq_init_queue calls
> > blk_alloc_queue_node.
> > 
> > Can you check if you get better results in your build?
> 
> It's crashing on that line because ctrl->tagset is NULL, which is
> because...
> 
> irq_create_affinity_masks() returns NULL on !CONFIG_SMP
> -> pci_irq_get_affinity() returns NULL
> -> blk_mq_pci_map_queues() returns -EINVAL
> -> blk_mq_alloc_tag_set() returns -EINVAL
> -> nvme_dev_add() doesn't set ctrl->tagset
> 
> The two-fold fix would be to make the nvme driver handle
> blk_mq_alloc_tag_set() failing and to fall back to a dumb mapping in
> blk_mq_pci_map_queues(), but I don't know what the best way to do those
> is.

irq_create_affinity_masks returns null and prints error, could we fix that?

Re: [BUG] nvme driver crash

[Christoph Hellwig]

On Wed, Jul 26, 2017 at 03:29:52PM -0700, Shaohua Li wrote:
> irq_create_affinity_masks returns null and prints error, could we fix that?

I've already sent a fix for that to the PCI list yesterday.

Re: [BUG] nvme driver crash

[Max Gurtovoy]

On 7/26/2017 10:12 PM, Omar Sandoval wrote:
> On Wed, Jul 26, 2017 at 10:34:43AM -0700, Christoph Hellwig wrote:
>> On Tue, Jul 25, 2017 at 03:24:08PM -0700, Shaohua Li wrote:
>>> Disable CONIFG_SMP, kernel crashes at boot time, here is the log.
>>
>> I can reproduce the issue.  Unfortunately the addresss in the bug
>> doesn't make any sense to me when resolving it using gdb, as t just
>> points to the line where blk_mq_init_queue calls
>> blk_alloc_queue_node.
>>
>> Can you check if you get better results in your build?
>
> It's crashing on that line because ctrl->tagset is NULL, which is
> because...
>
> irq_create_affinity_masks() returns NULL on !CONFIG_SMP
> -> pci_irq_get_affinity() returns NULL
> -> blk_mq_pci_map_queues() returns -EINVAL
> -> blk_mq_alloc_tag_set() returns -EINVAL
> -> nvme_dev_add() doesn't set ctrl->tagset
>
> The two-fold fix would be to make the nvme driver handle
> blk_mq_alloc_tag_set() failing and to fall back to a dumb mapping in
> blk_mq_pci_map_queues(), but I don't know what the best way to do those
> is.
>

Adding Sagi and Keith.

Christoph,
I've send some fix few months ago to that but haven't got a green light:


nvme: don't ignore tagset allocation failures

the nvme_dev_add() function silently ignores failures.
In case blk_mq_alloc_tag_set fails, we hit NULL deref while
calling blk_mq_init_queue during nvme_alloc_ns with tagset == NULL.
Instead, we'll not issue the scan_work in case tagset allocation
failed and leave the ctrl functional.

Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
Reviewed-by: Keith Busch <keith.busch@intel.com>
---
  drivers/nvme/host/core.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9b3b57f..493722a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2115,9 +2115,9 @@ void nvme_queue_scan(struct nvme_ctrl *ctrl)
  {
  	/*
  	 * Do not queue new scan work when a controller is reset during
-	 * removal.
+	 * removal or if the tagset doesn't exist.
  	 */
-	if (ctrl->state == NVME_CTRL_LIVE)
+	if (ctrl->state == NVME_CTRL_LIVE && ctrl->tagset)
  		schedule_work(&ctrl->scan_work);
  }
  EXPORT_SYMBOL_GPL(nvme_queue_scan);


maybe we can rebase and consider it again ?

Re: [BUG] nvme driver crash

[Sagi Grimberg]

> Adding Sagi and Keith.
> 
> Christoph,
> I've send some fix few months ago to that but haven't got a green light:
> 
> 
> nvme: don't ignore tagset allocation failures
> 
> the nvme_dev_add() function silently ignores failures.
> In case blk_mq_alloc_tag_set fails, we hit NULL deref while
> calling blk_mq_init_queue during nvme_alloc_ns with tagset == NULL.
> Instead, we'll not issue the scan_work in case tagset allocation
> failed and leave the ctrl functional.
> 

IIRC I argued that the core should not check the tagset existence,

Regardless that its the wrong layer to check it, it means that
all drivers need to make sure to take care of getting this dereference
correct.

Perhaps we need to come up with a new state for this stage (something
like CTRL_ADMIN_READY), and state transition is:

NEW -> ADMIN_READY (admin queue configured) -> LIVE (if we have
one or more IO queues ready).

> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> Reviewed-by: Keith Busch <keith.busch@intel.com>
> ---
>   drivers/nvme/host/core.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 9b3b57f..493722a 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2115,9 +2115,9 @@ void nvme_queue_scan(struct nvme_ctrl *ctrl)
>   {
>       /*
>        * Do not queue new scan work when a controller is reset during
> -     * removal.
> +     * removal or if the tagset doesn't exist.
>        */
> -    if (ctrl->state == NVME_CTRL_LIVE)
> +    if (ctrl->state == NVME_CTRL_LIVE && ctrl->tagset)
>           schedule_work(&ctrl->scan_work);
>   }
>   EXPORT_SYMBOL_GPL(nvme_queue_scan);
> 
> 
> maybe we can rebase and consider it again ?
>