[PATCH] nvme: Fix memory leak in nvme_init_ctrl error path

[PATCH] nvme: Fix memory leak in nvme_init_ctrl error path

[Sagi Grimberg]

nvme_init_ctrl may fail before creating the misc device or after creating
the misc device.

If we fail before creating the misc device, we just need to deallocate what
was allocated before and return (as usually done).

If we fail after we create the misc device, we must put
the final reference on the device in order to make sure that internal
device resources are cleaned up.

The device release also triggers nvme_free_ctrl method so we need to make
sure to identify that we failed during the initialization itself and skip
cleaning because nvme_init_ctrl error path cleaned up on its own (we do
that by marking the ctrl->instance to UNINITIALIZED).

We also drop the explicit dev_name deallocation because it is freed in
the device release sequence.

This addresses a memory leak triggered by blktests nvme/044 which happens
to fail exactly after misc device initialization:
--
unreferenced object 0xffff95678a54cd00 (size 256):
  comm "nvme", pid 1941, jiffies 4294761594 (age 10.010s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 08 cd 54 8a 67 95 ff ff  ..........T.g...
    08 cd 54 8a 67 95 ff ff e0 b5 7b 8b ff ff ff ff  ..T.g.....{.....
  backtrace:
    [<ffffffff8b349205>] kmalloc_trace+0x25/0x90
    [<ffffffff8b7c0463>] device_add+0x303/0x690
    [<ffffffff8b4103c4>] cdev_device_add+0x44/0x90
    [<ffffffffc0de1c42>] 0xffffffffc0de1c42
    [<ffffffffc0d788b3>] 0xffffffffc0d788b3
    [<ffffffffc0d8c66d>] 0xffffffffc0d8c66d
    [<ffffffffc0d8c831>] 0xffffffffc0d8c831
    [<ffffffff8b40a8b2>] vfs_write+0xc2/0x3c0
    [<ffffffff8b40aeff>] ksys_write+0x5f/0xe0
    [<ffffffff8bc0eb58>] do_syscall_64+0x38/0x90
    [<ffffffff8be000aa>] entry_SYSCALL_64_after_hwframe+0x72/0xdc
--

Reported-by: Irvin Cote <irvincoteg@gmail.com>
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c | 28 ++++++++++++++++------------
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ea508e9a1de7..b374e6007553 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5083,6 +5083,9 @@ static void nvme_free_ctrl(struct device *dev)
 		container_of(dev, struct nvme_ctrl, ctrl_device);
 	struct nvme_subsystem *subsys = ctrl->subsys;
 
+	if (ctrl->instance == NVME_CTRL_INSTANCE_UNINITIALIZED)
+		return;
+
 	if (!subsys || ctrl->instance != subsys->instance)
 		ida_free(&nvme_instance_ida, ctrl->instance);
 
@@ -5137,18 +5140,19 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	INIT_DELAYED_WORK(&ctrl->failfast_work, nvme_failfast_work);
 	memset(&ctrl->ka_cmd, 0, sizeof(ctrl->ka_cmd));
 	ctrl->ka_cmd.common.opcode = nvme_admin_keep_alive;
+	ctrl->instance = NVME_CTRL_INSTANCE_UNINITIALIZED;
 
 	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;
+	if (ret < 0) {
+		__free_page(ctrl->discard_page);
+		return ret;
+	}
 	ctrl->instance = ret;
 
 	device_initialize(&ctrl->ctrl_device);
@@ -5172,7 +5176,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	ctrl->cdev.owner = ops->module;
 	ret = cdev_device_add(&ctrl->cdev, ctrl->device);
 	if (ret)
-		goto out_free_name;
+		goto out_put_ctrl;
 
 	/*
 	 * Initialize latency tolerance controls.  The sysfs files won't
@@ -5193,14 +5197,14 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	nvme_fault_inject_fini(&ctrl->fault_inject);
 	dev_pm_qos_hide_latency_tolerance(ctrl->device);
 	cdev_device_del(&ctrl->cdev, ctrl->device);
-out_free_name:
+out_put_ctrl:
+	/* pairs with nvme_get_ctrl */
 	nvme_put_ctrl(ctrl);
-	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);
+	ctrl->instance = NVME_CTRL_INSTANCE_UNINITIALIZED;
+	/* pairs with device_initialize .release method will cleanup */
+	nvme_put_ctrl(ctrl);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(nvme_init_ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bf46f122e9e1..920403589670 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -255,6 +255,7 @@ struct nvme_ctrl {
 	struct request_queue *connect_q;
 	struct request_queue *fabrics_q;
 	struct device *dev;
+#define NVME_CTRL_INSTANCE_UNINITIALIZED (-1)
 	int instance;
 	int numa_node;
 	struct blk_mq_tag_set *tagset;
-- 
2.34.1

Re: [PATCH] nvme: Fix memory leak in nvme_init_ctrl error path

[Chaitanya Kulkarni]

On 5/3/23 08:09, Sagi Grimberg wrote:
> nvme_init_ctrl may fail before creating the misc device or after creating
> the misc device.
>
> If we fail before creating the misc device, we just need to deallocate what
> was allocated before and return (as usually done).
>
> If we fail after we create the misc device, we must put
> the final reference on the device in order to make sure that internal
> device resources are cleaned up.
>
> The device release also triggers nvme_free_ctrl method so we need to make
> sure to identify that we failed during the initialization itself and skip
> cleaning because nvme_init_ctrl error path cleaned up on its own (we do
> that by marking the ctrl->instance to UNINITIALIZED).
>
> We also drop the explicit dev_name deallocation because it is freed in
> the device release sequence.
>
> This addresses a memory leak triggered by blktests nvme/044 which happens
> to fail exactly after misc device initialization:
> --
> unreferenced object 0xffff95678a54cd00 (size 256):
>    comm "nvme", pid 1941, jiffies 4294761594 (age 10.010s)
>    hex dump (first 32 bytes):
>      00 00 00 00 00 00 00 00 08 cd 54 8a 67 95 ff ff  ..........T.g...
>      08 cd 54 8a 67 95 ff ff e0 b5 7b 8b ff ff ff ff  ..T.g.....{.....
>    backtrace:
>      [<ffffffff8b349205>] kmalloc_trace+0x25/0x90
>      [<ffffffff8b7c0463>] device_add+0x303/0x690
>      [<ffffffff8b4103c4>] cdev_device_add+0x44/0x90
>      [<ffffffffc0de1c42>] 0xffffffffc0de1c42
>      [<ffffffffc0d788b3>] 0xffffffffc0d788b3
>      [<ffffffffc0d8c66d>] 0xffffffffc0d8c66d
>      [<ffffffffc0d8c831>] 0xffffffffc0d8c831
>      [<ffffffff8b40a8b2>] vfs_write+0xc2/0x3c0
>      [<ffffffff8b40aeff>] ksys_write+0x5f/0xe0
>      [<ffffffff8bc0eb58>] do_syscall_64+0x38/0x90
>      [<ffffffff8be000aa>] entry_SYSCALL_64_after_hwframe+0x72/0xdc
> --
>
> Reported-by: Irvin Cote <irvincoteg@gmail.com>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   

I try to apply this patch on linux-nvme nvme-6.4 it is not applying,
which tree I should use for this to test it ?

nvme (nvme-6.4) # git apply init-ctrl-fix.patch
error: patch failed: drivers/nvme/host/core.c:5137
error: drivers/nvme/host/core.c: patch does not apply

-ck

Re: [PATCH] nvme: Fix memory leak in nvme_init_ctrl error path

[Sagi Grimberg]

> I try to apply this patch on linux-nvme nvme-6.4 it is not applying,
> which tree I should use for this to test it ?
> 
> nvme (nvme-6.4) # git apply init-ctrl-fix.patch
> error: patch failed: drivers/nvme/host/core.c:5137
> error: drivers/nvme/host/core.c: patch does not apply

It is on top of your series for the other memleaks

Re: [PATCH] nvme: Fix memory leak in nvme_init_ctrl error path

[Chaitanya Kulkarni]

On 5/4/23 01:18, Sagi Grimberg wrote:
>
>> I try to apply this patch on linux-nvme nvme-6.4 it is not applying,
>> which tree I should use for this to test it ?
>>
>> nvme (nvme-6.4) # git apply init-ctrl-fix.patch
>> error: patch failed: drivers/nvme/host/core.c:5137
>> error: drivers/nvme/host/core.c: patch does not apply
>
> It is on top of your series for the other memleaks

With this applied on the top of [1] on linux-nvme:nvme-6.4,
blktests nvme/044 and nvme/045 are passing without producing
kmemleak [2].

without this patch I'm still getting :-

   backtrace:
     [<0000000003b3ab89>] kmalloc_trace+0x25/0x90
     [<00000000ee6a62bc>] device_add+0x4cf/0x850
     [<000000006959d6db>] cdev_device_add+0x44/0x90
     [<00000000a9254393>] nvme_init_ctrl+0x352/0x410 [nvme_core]
     [<00000000a8a801a7>] 0xffffffffc05158b3
     [<00000000a566bd92>] 0xffffffffc03734cb
     [<000000001b88fc1d>] vfs_write+0xc5/0x3c0
     [<0000000021d472df>] ksys_write+0x5f/0xe0
     [<0000000001cad18e>] do_syscall_64+0x3b/0x90
     [<000000001fc6210f>] entry_SYSCALL_64_after_hwframe+0x72/0xdc


Feel free to add :-
Tested-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[1] http://lists.infradead.org/pipermail/linux-nvme/2023-April/039496.html

Re: [PATCH] nvme: Fix memory leak in nvme_init_ctrl error path

[Yi Zhang]

Verified the kmemleak issue I reported:
https://lore.kernel.org/linux-nvme/CAHj4cs-nDaKzMx2txO4dbE+Mz9ePwLtU0e3egz+StmzOUgWUrA@mail.gmail.com/

Tested-by: Yi Zhang <yi.zhang@redhat.com>

On Wed, May 3, 2023 at 11:13 PM Sagi Grimberg <sagi@grimberg.me> wrote:
>
> nvme_init_ctrl may fail before creating the misc device or after creating
> the misc device.
>
> If we fail before creating the misc device, we just need to deallocate what
> was allocated before and return (as usually done).
>
> If we fail after we create the misc device, we must put
> the final reference on the device in order to make sure that internal
> device resources are cleaned up.
>
> The device release also triggers nvme_free_ctrl method so we need to make
> sure to identify that we failed during the initialization itself and skip
> cleaning because nvme_init_ctrl error path cleaned up on its own (we do
> that by marking the ctrl->instance to UNINITIALIZED).
>
> We also drop the explicit dev_name deallocation because it is freed in
> the device release sequence.
>
> This addresses a memory leak triggered by blktests nvme/044 which happens
> to fail exactly after misc device initialization:
> --
> unreferenced object 0xffff95678a54cd00 (size 256):
>   comm "nvme", pid 1941, jiffies 4294761594 (age 10.010s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 08 cd 54 8a 67 95 ff ff  ..........T.g...
>     08 cd 54 8a 67 95 ff ff e0 b5 7b 8b ff ff ff ff  ..T.g.....{.....
>   backtrace:
>     [<ffffffff8b349205>] kmalloc_trace+0x25/0x90
>     [<ffffffff8b7c0463>] device_add+0x303/0x690
>     [<ffffffff8b4103c4>] cdev_device_add+0x44/0x90
>     [<ffffffffc0de1c42>] 0xffffffffc0de1c42
>     [<ffffffffc0d788b3>] 0xffffffffc0d788b3
>     [<ffffffffc0d8c66d>] 0xffffffffc0d8c66d
>     [<ffffffffc0d8c831>] 0xffffffffc0d8c831
>     [<ffffffff8b40a8b2>] vfs_write+0xc2/0x3c0
>     [<ffffffff8b40aeff>] ksys_write+0x5f/0xe0
>     [<ffffffff8bc0eb58>] do_syscall_64+0x38/0x90
>     [<ffffffff8be000aa>] entry_SYSCALL_64_after_hwframe+0x72/0xdc
> --
>
> Reported-by: Irvin Cote <irvincoteg@gmail.com>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/nvme/host/core.c | 28 ++++++++++++++++------------
>  drivers/nvme/host/nvme.h |  1 +
>  2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index ea508e9a1de7..b374e6007553 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -5083,6 +5083,9 @@ static void nvme_free_ctrl(struct device *dev)
>                 container_of(dev, struct nvme_ctrl, ctrl_device);
>         struct nvme_subsystem *subsys = ctrl->subsys;
>
> +       if (ctrl->instance == NVME_CTRL_INSTANCE_UNINITIALIZED)
> +               return;
> +
>         if (!subsys || ctrl->instance != subsys->instance)
>                 ida_free(&nvme_instance_ida, ctrl->instance);
>
> @@ -5137,18 +5140,19 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
>         INIT_DELAYED_WORK(&ctrl->failfast_work, nvme_failfast_work);
>         memset(&ctrl->ka_cmd, 0, sizeof(ctrl->ka_cmd));
>         ctrl->ka_cmd.common.opcode = nvme_admin_keep_alive;
> +       ctrl->instance = NVME_CTRL_INSTANCE_UNINITIALIZED;
>
>         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;
> +       if (ret < 0) {
> +               __free_page(ctrl->discard_page);
> +               return ret;
> +       }
>         ctrl->instance = ret;
>
>         device_initialize(&ctrl->ctrl_device);
> @@ -5172,7 +5176,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
>         ctrl->cdev.owner = ops->module;
>         ret = cdev_device_add(&ctrl->cdev, ctrl->device);
>         if (ret)
> -               goto out_free_name;
> +               goto out_put_ctrl;
>
>         /*
>          * Initialize latency tolerance controls.  The sysfs files won't
> @@ -5193,14 +5197,14 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
>         nvme_fault_inject_fini(&ctrl->fault_inject);
>         dev_pm_qos_hide_latency_tolerance(ctrl->device);
>         cdev_device_del(&ctrl->cdev, ctrl->device);
> -out_free_name:
> +out_put_ctrl:
> +       /* pairs with nvme_get_ctrl */
>         nvme_put_ctrl(ctrl);
> -       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);
> +       ctrl->instance = NVME_CTRL_INSTANCE_UNINITIALIZED;
> +       /* pairs with device_initialize .release method will cleanup */
> +       nvme_put_ctrl(ctrl);
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(nvme_init_ctrl);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index bf46f122e9e1..920403589670 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -255,6 +255,7 @@ struct nvme_ctrl {
>         struct request_queue *connect_q;
>         struct request_queue *fabrics_q;
>         struct device *dev;
> +#define NVME_CTRL_INSTANCE_UNINITIALIZED (-1)
>         int instance;
>         int numa_node;
>         struct blk_mq_tag_set *tagset;
> --
> 2.34.1
>
>


-- 
Best Regards,
  Yi Zhang

Re: [PATCH] nvme: Fix memory leak in nvme_init_ctrl error path

[Christoph Hellwig]

On Wed, May 03, 2023 at 06:09:25PM +0300, Sagi Grimberg wrote:
> -	if (!ctrl->discard_page) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> +	if (!ctrl->discard_page)
> +		return -ENOMEM;

Can we please pre-load these cleanups in a separate patch?

> -out:
> -	if (ctrl->discard_page)
> -		__free_page(ctrl->discard_page);
> +	ctrl->instance = NVME_CTRL_INSTANCE_UNINITIALIZED;
> +	/* pairs with device_initialize .release method will cleanup */
> +	nvme_put_ctrl(ctrl);

Err, no.  We should not go through .release with a partial 
initialization.  Please do proper unwinding before the device
is added, and make sure everything is in a proper state by the
time ->release can be called.

>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(nvme_init_ctrl);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index bf46f122e9e1..920403589670 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -255,6 +255,7 @@ struct nvme_ctrl {
>  	struct request_queue *connect_q;
>  	struct request_queue *fabrics_q;
>  	struct device *dev;
> +#define NVME_CTRL_INSTANCE_UNINITIALIZED (-1)
>  	int instance;
>  	int numa_node;
>  	struct blk_mq_tag_set *tagset;
> -- 
> 2.34.1
---end quoted text---

Re: [PATCH] nvme: Fix memory leak in nvme_init_ctrl error path

[Sagi Grimberg]

>> -	if (!ctrl->discard_page) {
>> -		ret = -ENOMEM;
>> -		goto out;
>> -	}
>> +	if (!ctrl->discard_page)
>> +		return -ENOMEM;
> 
> Can we please pre-load these cleanups in a separate patch?

OK.

>> -out:
>> -	if (ctrl->discard_page)
>> -		__free_page(ctrl->discard_page);
>> +	ctrl->instance = NVME_CTRL_INSTANCE_UNINITIALIZED;
>> +	/* pairs with device_initialize .release method will cleanup */
>> +	nvme_put_ctrl(ctrl);
> 
> Err, no.  We should not go through .release with a partial
> initialization.

But that is already the case, even if nvme_init_ctrl completes,
it can subsequently fail in nvme_init_ctrl_finish() and the
drivers already simply release the final reference on the
device and go through .release on a partly initialized dev.

> Please do proper unwinding before the device
> is added, and make sure everything is in a proper state by the
> time ->release can be called.

I don't see how that is possible.

The reason the .release is used in the patch is because device_add
allocates internal resources that are freed when it is released
(see device_private_init called, which is exactly the memleak
addressed in this patch).

So we can't know from nvme what to unwind. The only alternative
I see is to move device_private_init() from device_add to
device_initialize() and introduce device_uninitialize() to
unwind.