[PATCH 0/3] Adjusting deref count in nvme_init_ctrl

[PATCH 0/3] Adjusting deref count in nvme_init_ctrl

[Irvin Cote]

The following three patches aim to solve the following issues:

-In case the teardown path of nvme_init_ctrl is triggered the driver
 exits with a non-zero ref count on the controller. 

-Teardown paths and the ctrl's release method perform similar tasks
 leading to double-freeing issues.

In details:

-The first patch is just a ceanup of nvme_init_ctrl teardown logic.

-The second patch adds an nvme_put_ctrl at the end of nvme_init_ctrl's
teardown path to ensure the controller releases all references upon exit.

-The third one addresses the double-freeing issues 

Irvin Cote (3):
  nvme-core: nvme_init_ctrl cleanup
  nvme-pci: Adjusting ctrl deref count
  nvme-core: preventing double freeing in ctrl release

 drivers/nvme/host/core.c | 20 ++++++++++----------
 drivers/nvme/host/pci.c  |  9 +++++++--
 2 files changed, 17 insertions(+), 12 deletions(-)

-- 
2.39.2

[PATCH 1/3] nvme-core: nvme_init_ctrl cleanup

[Irvin Cote]

Removed an incoherent check at the last exit label
and renamed it for better clarity.

Signed-off-by: Irvin Cote <irvincoteg@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

[PATCH 2/3] nvme-pci: Adjusting ctrl deref count

[Irvin Cote]

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@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

[PATCH 3/3] nvme-core: preventing double freeing in ctrl release

[Irvin Cote]

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@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

Re: [PATCH 3/3] nvme-core: preventing double freeing in ctrl release

[Keith Busch]

On Tue, Apr 25, 2023 at 06:18:36PM -0300, Irvin Cote wrote:
> 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.

Your line wrap setting is way too short. 72 characters is recommended.
 
> @@ -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;

Isn't the problem that the nvme_put_ctrl() in this function's 'goto' error is
doubling the rest of the unwinding, which means your ida_free() is happening
twice?

I think instead the nvme_get_ctrl() should be the last thing done in the
init_ctrl() function, then the rest of the cleanup on error would be simpler.

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

kfree() already checks for NULL, so these are redundant.

>  }
>  
>  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);
>  }
> -- 

Re: [PATCH 3/3] nvme-core: preventing double freeing in ctrl release

[irvin cote]

On Tue, 25 Apr 2023 at 18:45, Keith Busch <kbusch@kernel.org> wrote:

> Isn't the problem that the nvme_put_ctrl() in this function's 'goto' error is
> doubling the rest of the unwinding, which means your ida_free() is happening
> twice?

The nvme_put_ctrl at the label "out_free_name" in nvme_init_ctrl,
doesn't decrement the ref-count to 0 (thus doesn't trigger anything).
This is because by the time we get to it device_initialize and
nvme_get_ctrl have each taken 1 reference.
However the problem indeed arises from the fact that both the teardown
path and the release method (nvme_free_ctrl, triggered by the last
nvme_put_ctrl)
call ida_free.

> I think instead the nvme_get_ctrl() should be the last thing done in the
> init_ctrl() function, then the rest of the cleanup on error would be simpler.
If we were to put nvme_get_ctrl at the end of nvme_init_ctrl, we would
still be facing the problem.
This is because, ultimately the teardown path must clear the ref-count
of the controller before exiting the driver
and this will lead to the release method being called and doubling
some of the unwinding done by the teardown path,
among which ida_free.