nvme: don't ignore tagset allocation failures
PATCH 1/1 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.
--- drivers/nvme/host/core.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/nvme/host/core.c +2 -2
@@ -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);
Signature · 1 line
1.7.1
Sagi Grimberg
maintainerPATCH 1/1 nvme: don't ignore tagset allocation failures
Quoted 18 lines
the
nvme_dev_add()function silently ignores failures. In caseblk_mq_alloc_tag_setfails, we hit NULL deref while callingblk_mq_init_queueduringnvme_alloc_nswith tagset == NULL. Instead, we'll not issue thescan_workin case tagset allocation failed and leave the ctrl functional.Signed-off-by Max Gurtovoy <maxg at mellanox.com>Reviewed-by Keith Busch <keith.busch at intel.com>--- drivers/nvme/host/core.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)drivers/nvme/host/core.c +0
-0
drivers/nvme/host/core.c void nvme_queue_scan(struct nvme_ctrl *ctrl) · line 2121
Quoted 9 lines
drivers/nvme/host/core.c +2
-2@@ -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);
This looks wrong...
Why is the controller LIVE without a tagset allocated? I think you should handle it in the pci driver.
PATCH 1/1 nvme: don't ignore tagset allocation failures
Quoted 30 lines · Sagi Grimberg
Quoted 18 lines
the
nvme_dev_add()function silently ignores failures. In caseblk_mq_alloc_tag_setfails, we hit NULL deref while callingblk_mq_init_queueduringnvme_alloc_nswith tagset == NULL. Instead, we'll not issue thescan_workin case tagset allocation failed and leave the ctrl functional.Signed-off-by Max Gurtovoy <maxg at mellanox.com>Reviewed-by Keith Busch <keith.busch at intel.com>--- drivers/nvme/host/core.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)drivers/nvme/host/core.c +0
-0drivers/nvme/host/core.c void nvme_queue_scan(struct nvme_ctrl *ctrl) · line 2121
Quoted 9 lines
drivers/nvme/host/core.c +2
-2@@ -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);This looks wrong...
Quoted 2 lines
Why is the controller LIVE without a tagset allocated? I think you should handle it in the pci driver.
Not having a tagset doesn't mean we can't go live; it just means we can't do IO, but the admin handle is still up for device management. Also, nvme_queue_scan can be triggered from places outside nvme pci's control, so I think Max's patch is an appropriate place to check.
Sagi Grimberg
maintainerPATCH 1/1 nvme: don't ignore tagset allocation failures
Quoted 3 lines
Not having a tagset doesn't mean we can't go live; it just means we can't do IO, but the admin handle is still up for device management.
So don't queue ns scanning...
Quoted 2 lines
Also,
nvme_queue_scancan be triggered from places outside nvme pci's control, so I think Max's patch is an appropriate place to check.
How is that not racy?
PATCH 1/1 nvme: don't ignore tagset allocation failures
Quoted 5 lines · Sagi Grimberg
Quoted 3 lines
Not having a tagset doesn't mean we can't go live; it just means we can't do IO, but the admin handle is still up for device management.
So don't queue ns scanning...
And what about the sysfs rescan or namespace notify async event? We have to fence those off too, so doing it in one place sounds better than three.
Quoted 4 lines
Quoted 2 lines
Also,
nvme_queue_scancan be triggered from places outside nvme pci's control, so I think Max's patch is an appropriate place to check.How is that not racy?
The tagset can only be allocated while the controller is not "LIVE", so that satisfies synchronizing this resource.
Sagi Grimberg
maintainerPATCH 1/1 nvme: don't ignore tagset allocation failures
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
Not having a tagset doesn't mean we can't go live; it just means we can't do IO, but the admin handle is still up for device management.
So don't queue ns scanning...
And what about the sysfs rescan or namespace notify async event? We have to fence those off too, so doing it in one place sounds better than three.
True, but it still feels wrong that the pci driver fails tagset allocation and continues to scan namespaces as if nothing happened, then queue namespaces checks for the existence of a tagset... It looks awkward...
Why is the controller moving to LIVE anyway? Obviously something went wrong, which is either a SW BUG (EINVAL), or a temporary failure that needs to be retried again (ENOMEM). So I think the proper solution would be to either retry again later for transient errors and for non-transient errors simply log the error remove the controller (its not a device issue so no point in keeping the controller around for error log query).
Thoughts?
PATCH 1/1 nvme: don't ignore tagset allocation failures
Quoted 24 lines · Sagi Grimberg
Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
Not having a tagset doesn't mean we can't go live; it just means we can't
do IO, but the admin handle is still up for device management.So don't queue ns scanning...
And what about the sysfs rescan or namespace notify async event? We have to fence those off too, so doing it in one place sounds better than three.
True, but it still feels wrong that the pci driver fails tagset allocation and continues to scan namespaces as if nothing happened, then queue namespaces checks for the existence of a tagset... It looks awkward...
Why is the controller moving to LIVE anyway? Obviously something went wrong, which is either a SW BUG (EINVAL), or a temporary failure that needs to be retried again (ENOMEM). So I think the proper solution would be to either retry again later for transient errors and for non-transient errors simply log the error remove the controller (its not a device issue so no point in keeping the controller around for error log query).
Quoted 1 line
Thoughts?
hi Sagi,
what do you think about this solution that we don't keep the ctrl:
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 remove the dead ctrl as it can't be functional anymore. Also remove old and wrong comment.
--- drivers/nvme/host/pci.c | 19 +++++++++---------- 1 files changed, 9 insertions(+), 10 deletions(-)
drivers/nvme/host/pci.c +0 -0
@@ -1545,14 +1545,10 @@ static void nvme_disable_io_queues(struct
nvme_dev *dev, int queues)
}
}
-/*
- * Return: error value if an error occurred setting up the queues or calling
- * Identify Device. 0 if these succeeded, even if adding some of the
- * namespaces failed. At the moment, these failures are silent. TBD which
- * failures should be reported.
- */
static int nvme_dev_add(struct nvme_dev *dev)
{
+ int ret = 0;
+
if (!dev->ctrl.tagset) {
dev->tagset.ops = &nvme_mq_ops; dev->tagset.nr_hw_queues = dev->online_queues - 1;
@@ -1564,8 +1560,9 @@ static int nvme_dev_add(struct nvme_dev *dev)
dev->tagset.flags = BLK_MQ_F_SHOULD_MERGE; dev->tagset.driver_data = dev;
- if (blk_mq_alloc_tag_set(&dev->tagset))
- return 0;
+ ret = blk_mq_alloc_tag_set(&dev->tagset);
+ if (ret)
+ return ret;
dev->ctrl.tagset = &dev->tagset;
} else {
blk_mq_update_nr_hw_queues(&dev->tagset, dev->online_queues - 1);
@@ -1574,7 +1571,7 @@ static int nvme_dev_add(struct nvme_dev *dev)
nvme_free_queues(dev, dev->online_queues);
}
- return 0;
+ return ret;
}
static int nvme_pci_enable(struct nvme_dev *dev)
@@ -1811,7 +1808,9 @@ static void nvme_reset_work(struct work_struct *work)
nvme_remove_namespaces(&dev->ctrl);
} else {
nvme_start_queues(&dev->ctrl);
- nvme_dev_add(dev);
+ result = nvme_dev_add(dev);
+ if (result)
+ goto out;
}
if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_LIVE)) {
Sagi Grimberg
maintainerPATCH 1/1 nvme: don't ignore tagset allocation failures
Quoted 4 lines
hi Sagi,
what do you think about this solution that we don't keep the ctrl:
I'd say that if you got a transient error, try reset again...