Correcting erroneous patch
Irvin Cote
irvincoteg at gmail.com
Mon Apr 24 21:44:07 PDT 2023
Following the error comment in the previous e-mail thread, I have identified the cause of the error.
nvme_free_ctrl which is the ctrl's release method, and nvme_init_ctrl's
teardown path, both call ida_free on ctrl->instance.
To properly decrement the ref-count while avoiding such issues:
-The final nvme_put_ctrl was placed in nvme_pci_alloc_dev at the end of the teardown path
so that nvme_free_ctrl executes last.
-I have addressed the broader issue of double-freeing between the teardown-path and the release method,
by marking the resources as freed in the former and implementing checks in the latter.
This is the object of the last two patches, while the first one is just a cleanup.
The patches were written one on top of the other.
-------------------------------------------------------
>From ff763a79d3746e3fd034e067e2fad798e95ff9a7 Mon Sep 17 00:00:00 2001
From: Irvin Cote <irvincoteg at gmail.com>
Date: Mon, 24 Apr 2023 23:46:48 -0300
Subject: [PATCH 1/3] nvme-core: nvme_init_ctrl cleanup
Removed an incoherent check at the last exit label
and renamed it for better clarity.
Signed-off-by: Irvin Cote <irvincoteg at gmail.com>
---
drivers/nvme/host/core.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d6a9bac91a4c..353443250d48 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5149,14 +5149,12 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
BUILD_BUG_ON(NVME_DSM_MAX_RANGES * sizeof(struct nvme_dsm_range) >
PAGE_SIZE);
ctrl->discard_page = alloc_page(GFP_KERNEL);
- if (!ctrl->discard_page) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!ctrl->discard_page)
+ return -ENOMEM;
ret = ida_alloc(&nvme_instance_ida, GFP_KERNEL);
if (ret < 0)
- goto out;
+ goto out_free_discard_page;
ctrl->instance = ret;
device_initialize(&ctrl->ctrl_device);
@@ -5204,9 +5202,8 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
kfree_const(ctrl->device->kobj.name);
out_release_instance:
ida_free(&nvme_instance_ida, ctrl->instance);
-out:
- if (ctrl->discard_page)
- __free_page(ctrl->discard_page);
+out_free_discard_page:
+ __free_page(ctrl->discard_page);
return ret;
}
EXPORT_SYMBOL_GPL(nvme_init_ctrl);
--
2.39.2
-------------------------------------------------------
>From 9bd93f3ae34a2f095d9a4687b453ef2bbc598f22 Mon Sep 17 00:00:00 2001
From: Irvin Cote <irvincoteg at gmail.com>
Date: Mon, 24 Apr 2023 23:58:11 -0300
Subject: [PATCH 2/3] nvme-pci: Adjusting ctrl deref count
If there is a failure in nvme_init_ctrl
the dereference counting is not properly
handled and the code exits with a non-zero
ctrl ref-count. This is because nvme_init_ctrl
takes two refs but only accounts for one deref
in its teardown path. Instead of adding another deref
there in the core layer, we take care of it here and
we make sure that it is the last thing we do so that
all teardown logic executes before the release of
the controller as the two have some identical instructions.
Signed-off-by: Irvin Cote <irvincoteg at gmail.com>
---
drivers/nvme/host/pci.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index cd7873de3121..65e4b9f1b632 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2975,6 +2975,7 @@ static struct nvme_dev *nvme_pci_alloc_dev(struct pci_dev *pdev,
kfree(dev->queues);
out_free_dev:
kfree(dev);
+ nvme_put_ctrl(&dev->ctrl);
return ERR_PTR(ret);
}
--
2.39.2
-------------------------------------------------------
>From 4ac05d1742c2b7e5bcb84dde1a64052ede071796 Mon Sep 17 00:00:00 2001
From: Irvin Cote <irvincoteg at gmail.com>
Date: Tue, 25 Apr 2023 00:44:51 -0300
Subject: [PATCH 3/3] nvme-core: preventing double freeing in ctrl release
The teardown logic and the release method for
the controller both free the same resources.
This introduces double-freeing issues.
Since the release of the ctrl is the last step
of the teardown, we can solve this by setting the freed
pointers to NULL in the teardown path and add checks
in the release method.
The only issue is with ida_free as ida doesn't offer any way to
indicate that an ID has already been freed.
In our specific case we can set the ID to -1 to indicate that.
We can do that because, the way we've setup ida to manage ctrl instances,
it always returns non-negative IDS.
Signed-off-by: Irvin Cote <irvincoteg at gmail.com>
---
drivers/nvme/host/core.c | 7 +++++--
drivers/nvme/host/pci.c | 8 ++++++--
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 353443250d48..15df7846fde8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5091,14 +5091,15 @@ static void nvme_free_ctrl(struct device *dev)
container_of(dev, struct nvme_ctrl, ctrl_device);
struct nvme_subsystem *subsys = ctrl->subsys;
- if (!subsys || ctrl->instance != subsys->instance)
+ if ((!subsys || ctrl->instance != subsys->instance) && ctrl->instance != -1)
ida_free(&nvme_instance_ida, ctrl->instance);
nvme_free_cels(ctrl);
nvme_mpath_uninit(ctrl);
nvme_auth_stop(ctrl);
nvme_auth_free(ctrl);
- __free_page(ctrl->discard_page);
+ if(ctrl->discard_page)
+ __free_page(ctrl->discard_page);
free_opal_dev(ctrl->opal_dev);
if (subsys) {
@@ -5202,8 +5203,10 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
kfree_const(ctrl->device->kobj.name);
out_release_instance:
ida_free(&nvme_instance_ida, ctrl->instance);
+ ctrl->instance = -1;
out_free_discard_page:
__free_page(ctrl->discard_page);
+ ctrl->discard_page = NULL;
return ret;
}
EXPORT_SYMBOL_GPL(nvme_init_ctrl);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 65e4b9f1b632..ff5c876739bc 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2681,8 +2681,10 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
nvme_free_tagset(dev);
put_device(dev->dev);
- kfree(dev->queues);
- kfree(dev);
+ if(dev->queues)
+ kfree(dev->queues);
+ if(dev)
+ kfree(dev);
}
static void nvme_reset_work(struct work_struct *work)
@@ -2973,8 +2975,10 @@ static struct nvme_dev *nvme_pci_alloc_dev(struct pci_dev *pdev,
out_put_device:
put_device(dev->dev);
kfree(dev->queues);
+ dev->queues = NULL;
out_free_dev:
kfree(dev);
+ dev = NULL;
nvme_put_ctrl(&dev->ctrl);
return ERR_PTR(ret);
}
--
2.39.2
More information about the Linux-nvme
mailing list