[PATCH] nvme: initialize identify ns data to NULL

[PATCH] nvme: initialize identify ns data to NULL

[Tokunori Ikegami]

Currently nvme_identify_ns() sets the data to NULL if failed.
Also the data is not freed if the function returned failure.
But correctly the data should be initialized to NULL.
So to make sure fix to initialize the data to NULL.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
---
 drivers/nvme/host/sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 3c55f7edd181..4e996e10f46a 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -185,7 +185,7 @@ static DEVICE_ATTR_RO(metadata_bytes);
 
 static int ns_head_update_nuse(struct nvme_ns_head *head)
 {
-	struct nvme_id_ns *id;
+	struct nvme_id_ns *id = NULL;
 	struct nvme_ns *ns;
 	int srcu_idx, ret = -EWOULDBLOCK;
 
@@ -212,7 +212,7 @@ static int ns_head_update_nuse(struct nvme_ns_head *head)
 
 static int ns_update_nuse(struct nvme_ns *ns)
 {
-	struct nvme_id_ns *id;
+	struct nvme_id_ns *id = NULL;
 	int ret;
 
 	/* Avoid issuing commands too often by rate limiting the update. */
-- 
2.40.1

Re: [PATCH] nvme: initialize identify ns data to NULL

[Sagi Grimberg]

On 25/03/2024 17:45, Tokunori Ikegami wrote:
> Currently nvme_identify_ns() sets the data to NULL if failed.

Where? Nothing sets id to NULL in nvme_identify_n(), what am I missing?
> Also the data is not freed if the function returned failure.

Where? I think you may be looking at a different code base?

--
int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
                         struct nvme_id_ns **id)
{
         ....
         error = nvme_submit_sync_cmd(ctrl->admin_q, &c, *id, sizeof(**id));
         if (error) {
                 dev_warn(ctrl->device, "Identify namespace failed 
(%d)\n", error);
                 kfree(*id);
         }
         return error;
}

I don't see how this patch makes sense...

Re: [PATCH] nvme: initialize identify ns data to NULL

[Tokunori Ikegami]

On 2024/03/26 17:20, Sagi Grimberg wrote:
>
>
> On 25/03/2024 17:45, Tokunori Ikegami wrote:
>> Currently nvme_identify_ns() sets the data to NULL if failed.
>
> Where? Nothing sets id to NULL in nvme_identify_n(), what am I missing?

It is set as below.

https://github.com/torvalds/linux/blame/master/drivers/nvme/host/core.c#L1542

>> Also the data is not freed if the function returned failure.
>
> Where? I think you may be looking at a different code base?

The ns_update_nuse() returns without the free if the ns_update_nuse() 
failure as below and the ns_head_update_nuse() also same.

https://github.com/torvalds/linux/blame/master/drivers/nvme/host/sysfs.c#L224
>
> -- 
> int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
>                         struct nvme_id_ns **id)
> {
>         ....
>         error = nvme_submit_sync_cmd(ctrl->admin_q, &c, *id, 
> sizeof(**id));
>         if (error) {
>                 dev_warn(ctrl->device, "Identify namespace failed 
> (%d)\n", error);
>                 kfree(*id);
>         }
>         return error;
> }
>
> I don't see how this patch makes sense...

I mean the id pointer should be initialized to NULL before the 
nvme_identify_ns() call. Currently above both the NULL setting and the 
failure return without the free the double error issue resolved but 
looks ideally initialize the data to NULL.

Re: [PATCH] nvme: initialize identify ns data to NULL

[Kanchan Joshi]

On 3/25/2024 9:15 PM, Tokunori Ikegami wrote:
> Currently nvme_identify_ns() sets the data to NULL if failed.
> Also the data is not freed if the function returned failure.

If it fails, it frees the allocated memory too.
So I don't see how the patch helps.

Re: [PATCH] nvme: initialize identify ns data to NULL

[Tokunori Ikegami]

On 2024/03/26 17:50, Kanchan Joshi wrote:
> On 3/25/2024 9:15 PM, Tokunori Ikegami wrote:
>> Currently nvme_identify_ns() sets the data to NULL if failed.
>> Also the data is not freed if the function returned failure.
> If it fails, it frees the allocated memory too.
> So I don't see how the patch helps.
  Yes I think this just helps if in future the nvme_identify_ns() 
function or the caller functions changed the implementation.

Re: [PATCH] nvme: initialize identify ns data to NULL

[Kanchan Joshi]

On 3/26/2024 7:08 PM, Tokunori Ikegami wrote:
> 
> On 2024/03/26 17:50, Kanchan Joshi wrote:
>> On 3/25/2024 9:15 PM, Tokunori Ikegami wrote:
>>> Currently nvme_identify_ns() sets the data to NULL if failed.
>>> Also the data is not freed if the function returned failure.
>> If it fails, it frees the allocated memory too.
>> So I don't see how the patch helps.
> Yes I think this just helps if in future thenvme_identify_ns() function 
> or the caller functions changed the implementation.

Not a compelling case of future convenience, IMHO.

In the current scheme of things, the assignment to NULL in all the 
callers of nvme_identify_ns (there are few more than what this patch 
covered) is redundant.

Re: [PATCH] nvme: initialize identify ns data to NULL

[Tokunori Ikegami]

Thanks for your comments. Understood them so let me abandon the patch. 
By the way I thought also the nvme functions allocating memory handling 
is not unified for example the nvme_identify_ns_nvm(), etc. about this I 
will do consider more. (The nvm version function caller seems still have 
a similar issue.)

On 2024/03/26 23:02, Kanchan Joshi wrote:
> Not a compelling case of future convenience, IMHO.
>
> In the current scheme of things, the assignment to NULL in all the
> callers of nvme_identify_ns (there are few more than what this patch
> covered) is redundant.

Re: [PATCH] nvme: initialize identify ns data to NULL

[Keith Busch]

On Tue, Mar 26, 2024 at 12:45:03AM +0900, Tokunori Ikegami wrote:
>  static int ns_head_update_nuse(struct nvme_ns_head *head)
>  {
> -	struct nvme_id_ns *id;
> +	struct nvme_id_ns *id = NULL;
>  	struct nvme_ns *ns;
>  	int srcu_idx, ret = -EWOULDBLOCK;

This is a redundant setting. The first thing that happens to "id" is
reference passed to nvme_identify_ns, and the first thing it does is
this:

	*id = kmalloc(sizeof(**id), GFP_KERNEL);

So either kmalloc succeeds and overwrites your NULL setting, or malloc
fails and sets it to NULL again.

Re: [PATCH] nvme: initialize identify ns data to NULL

[Tokunori Ikegami]

Okay I see. The nvme_identify_ns_nvm() version only sets the memory 
allocated to the output parameter nvmp before the return success so if 
the nvme_identify_ns() also changed as same the NULL initialization 
works correctly. Thank you.

On 2024/03/27 0:37, Keith Busch wrote:
> On Tue, Mar 26, 2024 at 12:45:03AM +0900, Tokunori Ikegami wrote:
>>   static int ns_head_update_nuse(struct nvme_ns_head *head)
>>   {
>> -	struct nvme_id_ns *id;
>> +	struct nvme_id_ns *id = NULL;
>>   	struct nvme_ns *ns;
>>   	int srcu_idx, ret = -EWOULDBLOCK;
> This is a redundant setting. The first thing that happens to "id" is
> reference passed to nvme_identify_ns, and the first thing it does is
> this:
>
> 	*id = kmalloc(sizeof(**id), GFP_KERNEL);
>
> So either kmalloc succeeds and overwrites your NULL setting, or malloc
> fails and sets it to NULL again.