| Message ID | 20241124125628.2532658-1-nilay@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2] nvmet: use kzalloc instead of ZERO_PAGE in nvme_execute_identify_ns_nvm() | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
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 >
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
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); }
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(-)