[PATCHv2] nvmet: use kzalloc instead of ZERO_PAGE in nvme_execute_identify_ns_nvm()

[PATCHv2] nvmet: use kzalloc instead of ZERO_PAGE in nvme_execute_identify_ns_nvm()

[Nilay Shroff]

The nvme_execute_identify_ns_nvm function uses ZERO_PAGE for copying
SG list with all zeros. As ZERO_PAGE would not necessarily return the
virtual-address of the zero page, we need to first convert the page
address to kernel virtual-address and then use it as source address
for copying the data to SG list with all zeros. Using return address
of ZERO_PAGE(0) as source address for copying data to SG list would
fill the target buffer with random/garbage value and causes the
undesired side effect.

As other identify implemenations uses kzalloc for allocating a zero
filled buffer, we decided use kzalloc for allocating a zero filled
buffer in nvme_execute_identify_ns_nvm function and then use this
buffer for copying all zeros to SG list buffers. So esentially, we
now avoid using ZERO_PAGE.

Reported-by: Yi Zhang <yi.zhang@redhat.com>
Fixes: 64a51080eaba ("nvmet: implement id ns for nvm command set")
Link: https://lore.kernel.org/all/CAHj4cs8OVyxmn4XTvA=y4uQ3qWpdw-x3M3FSUYr-KpE-nhaFEA@mail.gmail.com/
Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
Changes from v1:
    - Use kzalloc instead of ZERO_PAGE() for allocating zero filled
	  buffer (Christoph Hellwing, Keith Busch)

 drivers/nvme/target/admin-cmd.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 934b401fbc2f..f92c5cb1a25b 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -901,13 +901,18 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req)
 static void nvme_execute_identify_ns_nvm(struct nvmet_req *req)
 {
 	u16 status;
+	struct nvme_id_ns_nvm *id;
 
 	status = nvmet_req_find_ns(req);
 	if (status)
 		goto out;
 
-	status = nvmet_copy_to_sgl(req, 0, ZERO_PAGE(0),
-				   NVME_IDENTIFY_DATA_SIZE);
+	id = kzalloc(sizeof(*id), GFP_KERNEL);
+	if (!id) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
 out:
 	nvmet_req_complete(req, status);
 }
-- 
2.45.2

Re: [PATCHv2] nvmet: use kzalloc instead of ZERO_PAGE in nvme_execute_identify_ns_nvm()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv2] nvmet: use kzalloc instead of ZERO_PAGE in nvme_execute_identify_ns_nvm()

[Yi Zhang]

Thanks for the fix.

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

On Sun, Nov 24, 2024 at 8:57 PM Nilay Shroff <nilay@linux.ibm.com> wrote:
>
> The nvme_execute_identify_ns_nvm function uses ZERO_PAGE for copying
> SG list with all zeros. As ZERO_PAGE would not necessarily return the
> virtual-address of the zero page, we need to first convert the page
> address to kernel virtual-address and then use it as source address
> for copying the data to SG list with all zeros. Using return address
> of ZERO_PAGE(0) as source address for copying data to SG list would
> fill the target buffer with random/garbage value and causes the
> undesired side effect.
>
> As other identify implemenations uses kzalloc for allocating a zero
> filled buffer, we decided use kzalloc for allocating a zero filled
> buffer in nvme_execute_identify_ns_nvm function and then use this
> buffer for copying all zeros to SG list buffers. So esentially, we
> now avoid using ZERO_PAGE.
>
> Reported-by: Yi Zhang <yi.zhang@redhat.com>
> Fixes: 64a51080eaba ("nvmet: implement id ns for nvm command set")
> Link: https://lore.kernel.org/all/CAHj4cs8OVyxmn4XTvA=y4uQ3qWpdw-x3M3FSUYr-KpE-nhaFEA@mail.gmail.com/
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
> Changes from v1:
>     - Use kzalloc instead of ZERO_PAGE() for allocating zero filled
>           buffer (Christoph Hellwing, Keith Busch)
>
>  drivers/nvme/target/admin-cmd.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 934b401fbc2f..f92c5cb1a25b 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -901,13 +901,18 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req)
>  static void nvme_execute_identify_ns_nvm(struct nvmet_req *req)
>  {
>         u16 status;
> +       struct nvme_id_ns_nvm *id;
>
>         status = nvmet_req_find_ns(req);
>         if (status)
>                 goto out;
>
> -       status = nvmet_copy_to_sgl(req, 0, ZERO_PAGE(0),
> -                                  NVME_IDENTIFY_DATA_SIZE);
> +       id = kzalloc(sizeof(*id), GFP_KERNEL);
> +       if (!id) {
> +               status = NVME_SC_INTERNAL;
> +               goto out;
> +       }
> +       status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
>  out:
>         nvmet_req_complete(req, status);
>  }
> --
> 2.45.2
>


-- 
Best Regards,
  Yi Zhang

Re: [PATCHv2] nvmet: use kzalloc instead of ZERO_PAGE in nvme_execute_identify_ns_nvm()

[Chaitanya Kulkarni]

On 11/24/24 04:55, Nilay Shroff wrote:
> The nvme_execute_identify_ns_nvm function uses ZERO_PAGE for copying
> SG list with all zeros. As ZERO_PAGE would not necessarily return the
> virtual-address of the zero page, we need to first convert the page
> address to kernel virtual-address and then use it as source address
> for copying the data to SG list with all zeros. Using return address
> of ZERO_PAGE(0) as source address for copying data to SG list would
> fill the target buffer with random/garbage value and causes the
> undesired side effect.
>
> As other identify implemenations uses kzalloc for allocating a zero
> filled buffer, we decided use kzalloc for allocating a zero filled
> buffer in nvme_execute_identify_ns_nvm function and then use this
> buffer for copying all zeros to SG list buffers. So esentially, we
> now avoid using ZERO_PAGE.
>
> Reported-by: Yi Zhang<yi.zhang@redhat.com>
> Fixes: 64a51080eaba ("nvmet: implement id ns for nvm command set")
> Link:https://lore.kernel.org/all/CAHj4cs8OVyxmn4XTvA=y4uQ3qWpdw-x3M3FSUYr-KpE-nhaFEA@mail.gmail.com/
> Signed-off-by: Nilay Shroff<nilay@linux.ibm.com>


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCHv2] nvmet: use kzalloc instead of ZERO_PAGE in nvme_execute_identify_ns_nvm()

[Keith Busch]

On Sun, Nov 24, 2024 at 06:25:53PM +0530, Nilay Shroff wrote:
> The nvme_execute_identify_ns_nvm function uses ZERO_PAGE for copying
> SG list with all zeros. As ZERO_PAGE would not necessarily return the
> virtual-address of the zero page, we need to first convert the page
> address to kernel virtual-address and then use it as source address
> for copying the data to SG list with all zeros. Using return address
> of ZERO_PAGE(0) as source address for copying data to SG list would
> fill the target buffer with random/garbage value and causes the
> undesired side effect.
> 
> As other identify implemenations uses kzalloc for allocating a zero
> filled buffer, we decided use kzalloc for allocating a zero filled
> buffer in nvme_execute_identify_ns_nvm function and then use this
> buffer for copying all zeros to SG list buffers. So esentially, we
> now avoid using ZERO_PAGE.

Thanks, applied to nvme-6.13.