* [PATCH] nvme: initialize identify ns data to NULL
@ 2024-03-25 15:45 ` Tokunori Ikegami
2024-03-26 8:20 ` Sagi Grimberg
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Tokunori Ikegami @ 2024-03-25 15:45 UTC (permalink / raw
To: linux-nvme; +Cc: 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: initialize identify ns data to NULL
2024-03-25 15:45 ` [PATCH] nvme: initialize identify ns data to NULL Tokunori Ikegami
@ 2024-03-26 8:20 ` Sagi Grimberg
2024-03-26 13:35 ` Tokunori Ikegami
2024-03-26 8:50 ` Kanchan Joshi
2024-03-26 15:37 ` Keith Busch
2 siblings, 1 reply; 9+ messages in thread
From: Sagi Grimberg @ 2024-03-26 8:20 UTC (permalink / raw
To: Tokunori Ikegami, linux-nvme
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...
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: initialize identify ns data to NULL
2024-03-25 15:45 ` [PATCH] nvme: initialize identify ns data to NULL Tokunori Ikegami
2024-03-26 8:20 ` Sagi Grimberg
@ 2024-03-26 8:50 ` Kanchan Joshi
2024-03-26 13:51 ` Tokunori Ikegami
[not found] ` <a92a1493-29ae-4f89-b17b-54d03ec0becc@gmail.com>
2024-03-26 15:37 ` Keith Busch
2 siblings, 2 replies; 9+ messages in thread
From: Kanchan Joshi @ 2024-03-26 8:50 UTC (permalink / raw
To: Tokunori Ikegami, linux-nvme
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: initialize identify ns data to NULL
2024-03-26 8:20 ` Sagi Grimberg
@ 2024-03-26 13:35 ` Tokunori Ikegami
0 siblings, 0 replies; 9+ messages in thread
From: Tokunori Ikegami @ 2024-03-26 13:35 UTC (permalink / raw
To: Sagi Grimberg, linux-nvme
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: initialize identify ns data to NULL
2024-03-26 8:50 ` Kanchan Joshi
@ 2024-03-26 13:51 ` Tokunori Ikegami
[not found] ` <a92a1493-29ae-4f89-b17b-54d03ec0becc@gmail.com>
1 sibling, 0 replies; 9+ messages in thread
From: Tokunori Ikegami @ 2024-03-26 13:51 UTC (permalink / raw
To: Kanchan Joshi, linux-nvme
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: initialize identify ns data to NULL
[not found] ` <a92a1493-29ae-4f89-b17b-54d03ec0becc@gmail.com>
@ 2024-03-26 14:02 ` Kanchan Joshi
2024-03-26 14:56 ` Tokunori Ikegami
0 siblings, 1 reply; 9+ messages in thread
From: Kanchan Joshi @ 2024-03-26 14:02 UTC (permalink / raw
To: Tokunori Ikegami, linux-nvme
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: initialize identify ns data to NULL
2024-03-26 14:02 ` Kanchan Joshi
@ 2024-03-26 14:56 ` Tokunori Ikegami
0 siblings, 0 replies; 9+ messages in thread
From: Tokunori Ikegami @ 2024-03-26 14:56 UTC (permalink / raw
To: Kanchan Joshi, linux-nvme
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: initialize identify ns data to NULL
2024-03-25 15:45 ` [PATCH] nvme: initialize identify ns data to NULL Tokunori Ikegami
2024-03-26 8:20 ` Sagi Grimberg
2024-03-26 8:50 ` Kanchan Joshi
@ 2024-03-26 15:37 ` Keith Busch
2024-03-26 15:54 ` Tokunori Ikegami
2 siblings, 1 reply; 9+ messages in thread
From: Keith Busch @ 2024-03-26 15:37 UTC (permalink / raw
To: Tokunori Ikegami; +Cc: linux-nvme
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: initialize identify ns data to NULL
2024-03-26 15:37 ` Keith Busch
@ 2024-03-26 15:54 ` Tokunori Ikegami
0 siblings, 0 replies; 9+ messages in thread
From: Tokunori Ikegami @ 2024-03-26 15:54 UTC (permalink / raw
To: Keith Busch; +Cc: linux-nvme
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-03-26 15:57 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20240325154544epcas5p4acf7f376241637872a433314489586fa@epcas5p4.samsung.com>
2024-03-25 15:45 ` [PATCH] nvme: initialize identify ns data to NULL Tokunori Ikegami
2024-03-26 8:20 ` Sagi Grimberg
2024-03-26 13:35 ` Tokunori Ikegami
2024-03-26 8:50 ` Kanchan Joshi
2024-03-26 13:51 ` Tokunori Ikegami
[not found] ` <a92a1493-29ae-4f89-b17b-54d03ec0becc@gmail.com>
2024-03-26 14:02 ` Kanchan Joshi
2024-03-26 14:56 ` Tokunori Ikegami
2024-03-26 15:37 ` Keith Busch
2024-03-26 15:54 ` Tokunori Ikegami
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.