| Message ID | 20241122085113.2487839-1-nilay@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nvmet: fix the use of ZERO_PAGE in nvme_execute_identify_ns_nvm() | expand |
Sorry but I forgot to add the reported-by tag. Reported-by: Yi Zhang <yi.zhang@redhat.com> On 11/22/24 14:20, 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 value and causes the undesired side effect. This patch > implements the fix ensuring that we use virtual-address of the > zero page for copying all zeros to the SG list buffers. > > Link: https://lore.kernel.org/all/CAHj4cs8OVyxmn4XTvA=y4uQ3qWpdw-x3M3FSUYr-KpE-nhaFEA@mail.gmail.com/ > Fixes: 64a51080eaba ("nvmet: implement id ns for nvm command set") > [nilay: Use page_to_virt() for converting ZERO_PAGE address to > virtual-address as suggested by Maurizio Lombardi] > Signed-off-by: Nilay Shroff <nilay@linux.ibm.com> > --- > drivers/nvme/target/admin-cmd.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c > index 934b401fbc2f..a2b0444f28ab 100644 > --- a/drivers/nvme/target/admin-cmd.c > +++ b/drivers/nvme/target/admin-cmd.c > @@ -901,12 +901,14 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req) > static void nvme_execute_identify_ns_nvm(struct nvmet_req *req) > { > u16 status; > + void *zero_buf; > > status = nvmet_req_find_ns(req); > if (status) > goto out; > > - status = nvmet_copy_to_sgl(req, 0, ZERO_PAGE(0), > + zero_buf = page_to_virt(ZERO_PAGE(0)); > + status = nvmet_copy_to_sgl(req, 0, zero_buf, > NVME_IDENTIFY_DATA_SIZE); > out: > nvmet_req_complete(req, status);
pá 22. 11. 2024 v 9:51 odesílatel Nilay Shroff <nilay@linux.ibm.com> napsal: > static void nvme_execute_identify_ns_nvm(struct nvmet_req *req) > { > u16 status; > + void *zero_buf; > > status = nvmet_req_find_ns(req); > if (status) > goto out; > > - status = nvmet_copy_to_sgl(req, 0, ZERO_PAGE(0), > + zero_buf = page_to_virt(ZERO_PAGE(0)); > + status = nvmet_copy_to_sgl(req, 0, zero_buf, > NVME_IDENTIFY_DATA_SIZE); > out: > nvmet_req_complete(req, status); I will later submit a patch to ensure this function complies with the NVMe base specification, building on your patch. Maurizio
On Fri, Nov 22, 2024 at 02:20:36PM +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 value and causes the undesired side effect. This patch > implements the fix ensuring that we use virtual-address of the > zero page for copying all zeros to the SG list buffers. I wonder if using ZERO_PAGE() is simply a little too smart for it's own sake and it should just use kzalloc like a bunch of other identify implementation..
On Fri, Nov 22, 2024 at 01:08:28PM +0100, Christoph Hellwig wrote: > On Fri, Nov 22, 2024 at 02:20:36PM +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 value and causes the undesired side effect. This patch > > implements the fix ensuring that we use virtual-address of the > > zero page for copying all zeros to the SG list buffers. > > I wonder if using ZERO_PAGE() is simply a little too smart for it's > own sake and it should just use kzalloc like a bunch of other identify > implementation.. Sure. That'll make it easier to report non-zero values if we decide to implement a non-stubbed version of this identification later.
On 11/22/24 21:30, Keith Busch wrote: > On Fri, Nov 22, 2024 at 01:08:28PM +0100, Christoph Hellwig wrote: >> On Fri, Nov 22, 2024 at 02:20:36PM +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 value and causes the undesired side effect. This patch >>> implements the fix ensuring that we use virtual-address of the >>> zero page for copying all zeros to the SG list buffers. >> >> I wonder if using ZERO_PAGE() is simply a little too smart for it's >> own sake and it should just use kzalloc like a bunch of other identify >> implementation.. > > Sure. That'll make it easier to report non-zero values if we decide to > implement a non-stubbed version of this identification later. Ok, so if we prefer using kzalloc instead of ZERO_PAGE() in nvme_execute_identify_ns_nvm function, as we're using kzalloc at other identify call sites, then I would update the patch and send v2 where we would replace ZERO_PAGE() with kzalloc. Thanks, --Nilay
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c index 934b401fbc2f..a2b0444f28ab 100644 --- a/drivers/nvme/target/admin-cmd.c +++ b/drivers/nvme/target/admin-cmd.c @@ -901,12 +901,14 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req) static void nvme_execute_identify_ns_nvm(struct nvmet_req *req) { u16 status; + void *zero_buf; status = nvmet_req_find_ns(req); if (status) goto out; - status = nvmet_copy_to_sgl(req, 0, ZERO_PAGE(0), + zero_buf = page_to_virt(ZERO_PAGE(0)); + status = nvmet_copy_to_sgl(req, 0, zero_buf, NVME_IDENTIFY_DATA_SIZE); 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 value and causes the undesired side effect. This patch implements the fix ensuring that we use virtual-address of the zero page for copying all zeros to the SG list buffers. Link: https://lore.kernel.org/all/CAHj4cs8OVyxmn4XTvA=y4uQ3qWpdw-x3M3FSUYr-KpE-nhaFEA@mail.gmail.com/ Fixes: 64a51080eaba ("nvmet: implement id ns for nvm command set") [nilay: Use page_to_virt() for converting ZERO_PAGE address to virtual-address as suggested by Maurizio Lombardi] Signed-off-by: Nilay Shroff <nilay@linux.ibm.com> --- drivers/nvme/target/admin-cmd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)