[PATCH] nvmet: preserve ANA log page copy errors

[PATCH] nvmet: preserve ANA log page copy errors

[Sreeraj S Kurup]

Do not overwrite an error returned while copying ANA
group descriptors with the result of copying the ANA
log page header.

Initialize the status to success before processing
ANA groups and copy the header only if all group
descriptor copies succeed. This ensures that failures
during descriptor copying are correctly reported to
the host.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/nvme/target/admin-cmd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 26028be8fe4d..2bcb6d3de9ec 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -543,7 +543,7 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 	u16 ngrps = 0;
 	u16 status;
 
-	status = NVME_SC_INTERNAL;
+	status = NVME_SC_SUCCESS;
 	desc = kmalloc_flex(*desc, nsids, NVMET_MAX_NAMESPACES);
 	if (!desc)
 		goto out;
@@ -572,7 +572,8 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 	kfree(desc);
 
 	/* copy the header last once we know the number of groups */
-	status = nvmet_copy_to_sgl(req, 0, &hdr, sizeof(hdr));
+	if (!status)
+		status = nvmet_copy_to_sgl(req, 0, &hdr, sizeof(hdr));
 out:
 	nvmet_req_complete(req, status);
 }
-- 
2.55.0

Re: [PATCH] nvmet: preserve ANA log page copy errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] nvmet: allocation failure in nvmet_execute_get_log_page_ana() returns success
- [High] nvmet: nvmet_execute_get_log_page_ana() breaks short ANA log page reads
--

commit a674cf164d41aef50b30cbe7f108586cbc265b8d
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

nvmet: preserve ANA log page copy errors

This commit initializes the status variable in
nvmet_execute_get_log_page_ana() to success and only copies the ANA log page
header if no error occurred while copying the ANA group descriptors.

> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 26028be8fe4d8..2bcb6d3de9eca 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -543,7 +543,7 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
>  	u16 ngrps = 0;
>  	u16 status;
>  
> -	status = NVME_SC_INTERNAL;
> +	status = NVME_SC_SUCCESS;
>  	desc = kmalloc_flex(*desc, nsids, NVMET_MAX_NAMESPACES);
>  	if (!desc)
>  		goto out;

[Severity: High]
If kmalloc_flex() returns null in nvmet_execute_get_log_page_ana() in
drivers/nvme/target/admin-cmd.c, won't jumping to the out label now complete
the request with a success status instead of an internal error status without
writing any data to the host buffer?

Previously, status was initialized before kmalloc_flex() so that the error
path on allocation failure would report an internal error:

drivers/nvme/target/admin-cmd.c:nvmet_execute_get_log_page_ana() {
	...
	status = NVME_SC_SUCCESS;
	desc = kmalloc_flex(*desc, nsids, NVMET_MAX_NAMESPACES);
	if (!desc)
		goto out;
	...
out:
	nvmet_req_complete(req, status);
}

> @@ -572,7 +572,8 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
>  	kfree(desc);
>  
>  	/* copy the header last once we know the number of groups */
> -	status = nvmet_copy_to_sgl(req, 0, &hdr, sizeof(hdr));
> +	if (!status)
> +		status = nvmet_copy_to_sgl(req, 0, &hdr, sizeof(hdr));

[Severity: High]
Does guarding the header copy with if (!status) in
nvmet_execute_get_log_page_ana() in drivers/nvme/target/admin-cmd.c break
short reads of the variable-length ana log page?

In nvmet_copy_to_sgl(), a non-zero status is returned when the copy exceeds
req->sg length (for example, when a host requests only sizeof(struct
nvme_ana_rsp_hdr) bytes to inspect chgcnt and ngrps before allocating a
buffer for the full log page):

drivers/nvme/target/core.c:nvmet_copy_to_sgl() {
	if (sg_pcopy_from_buffer(req->sg, req->sg_cnt, buf, len, off) != len) {
		req->error_loc = offsetof(struct nvme_common_command, dptr);
		return NVME_SC_SGL_INVALID_DATA | NVME_STATUS_DNR;
	}
	return 0;
}

In nvmet_execute_get_log_page_ana(), when a descriptor does not fit in the
host buffer, the first loop breaks out early and the second loop (added in
commit be1277f5eb17 ("nvme: count all ANA groups for ANA Log page")) counts
the remaining enabled groups specifically so hdr.ngrps holds the total group
count when hdr is written at offset 0:

drivers/nvme/target/admin-cmd.c:nvmet_execute_get_log_page_ana() {
	...
	for (grpid = 1; grpid <= NVMET_MAX_ANAGRPS; grpid++) {
		if (!nvmet_ana_group_enabled[grpid])
			continue;
		len = nvmet_format_ana_group(req, grpid, desc);
		status = nvmet_copy_to_sgl(req, offset, desc, len);
		if (status)
			break;
		offset += len;
		ngrps++;
	}
	for ( ; grpid <= NVMET_MAX_ANAGRPS; grpid++) {
		if (nvmet_ana_group_enabled[grpid])
			ngrps++;
	}

	hdr.chgcnt = cpu_to_le64(nvmet_ana_chgcnt);
	hdr.ngrps = cpu_to_le16(ngrps);
	nvmet_clear_aen_bit(req, NVME_AEN_BIT_ANA_CHANGE);
	up_read(&nvmet_ana_sem);
}

With if (!status) before copying hdr, any short read that breaks out of the
first loop skips writing hdr, fails the command, renders the second loop dead
work, and still clears the asynchronous event bit via nvmet_clear_aen_bit()
even though the command fails.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031083505.4767-1-sreekuttan2156239@gmail.com?part=1

[PATCH v2] nvmet: preserve ANA log page copy errors

[Sreeraj S Kurup]

Preserve errors encountered while copying ANA group
descriptors instead of unconditionally overwriting
them with the result of copying the ANA log page
header.

Ensure that allocation failures are reported correctly
and that short ANA log page reads can still return the
header containing the change count and group count.
Preserve genuine descriptor-copy and header-copy
errors.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
v2:
 - Preserve the allocation failure status.
 - Handle short ANA log-page reads by checking
   descriptor length against the requested transfer
   length before copying.
 - Preserve genuine descriptor and header copy errors.

 drivers/nvme/target/admin-cmd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 26028be8fe4d..b0bcfbc94b93 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -542,17 +542,23 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 	u32 grpid;
 	u16 ngrps = 0;
 	u16 status;
+	u16 header_status;
 
 	status = NVME_SC_INTERNAL;
 	desc = kmalloc_flex(*desc, nsids, NVMET_MAX_NAMESPACES);
 	if (!desc)
 		goto out;
 
+	status = NVME_SC_SUCCESS;
+
 	down_read(&nvmet_ana_sem);
 	for (grpid = 1; grpid <= NVMET_MAX_ANAGRPS; grpid++) {
 		if (!nvmet_ana_group_enabled[grpid])
 			continue;
 		len = nvmet_format_ana_group(req, grpid, desc);
+		if (offset > req->transfer_len || len > req->transfer_len - offset)
+			break;
+
 		status = nvmet_copy_to_sgl(req, offset, desc, len);
 		if (status)
 			break;
@@ -572,7 +578,9 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 	kfree(desc);
 
 	/* copy the header last once we know the number of groups */
-	status = nvmet_copy_to_sgl(req, 0, &hdr, sizeof(hdr));
+	header_status = nvmet_copy_to_sgl(req, 0, &hdr, sizeof(hdr));
+	if (!status)
+		status = header_status;
 out:
 	nvmet_req_complete(req, status);
 }
-- 
2.55.0

Re: [PATCH v2] nvmet: preserve ANA log page copy errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvmet: skipped partial ANA descriptor copy in nvmet_execute_get_log_page_ana()
--

commit d3533edc412b03d8943fd7010d72c42247b5d211
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

nvmet: preserve ANA log page copy errors

This commit preserves descriptor-copy and allocation errors in
nvmet_execute_get_log_page_ana() instead of overwriting status when copying
the ANA log page header, and skips copying ANA group descriptors when their
length exceeds the remaining transfer length.

> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 26028be8fe4d8..b0bcfbc94b930 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -542,17 +542,23 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
>  	u32 grpid;
>  	u16 ngrps = 0;
>  	u16 status;
> +	u16 header_status;
>  
>  	status = NVME_SC_INTERNAL;
>  	desc = kmalloc_flex(*desc, nsids, NVMET_MAX_NAMESPACES);
>  	if (!desc)
>  		goto out;
>  
> +	status = NVME_SC_SUCCESS;
> +
>  	down_read(&nvmet_ana_sem);
>  	for (grpid = 1; grpid <= NVMET_MAX_ANAGRPS; grpid++) {
>  		if (!nvmet_ana_group_enabled[grpid])
>  			continue;
>  		len = nvmet_format_ana_group(req, grpid, desc);
> +		if (offset > req->transfer_len || len > req->transfer_len - offset)
> +			break;
> +
>  		status = nvmet_copy_to_sgl(req, offset, desc, len);
>  		if (status)
>  			break;
> @@ -572,7 +578,9 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
>  	kfree(desc);
>  
>  	/* copy the header last once we know the number of groups */
> -	status = nvmet_copy_to_sgl(req, 0, &hdr, sizeof(hdr));
> +	header_status = nvmet_copy_to_sgl(req, 0, &hdr, sizeof(hdr));
> +	if (!status)
> +		status = header_status;

[Severity: High]
As reported by Sashiko on v1 [1], in nvmet_execute_get_log_page_ana() in
drivers/nvme/target/admin-cmd.c, when a host issues a Get Log Page command
for NVME_LOG_ANA with req->transfer_len ending partway through an enabled ANA
group descriptor (offset < req->transfer_len < offset + len), does breaking
out of the loop before calling nvmet_copy_to_sgl() leave the trailing
[offset, req->transfer_len) portion of req->sg unwritten?

In drivers/nvme/target/core.c, nvmet_copy_to_sgl() wraps
sg_pcopy_from_buffer():

u16 nvmet_copy_to_sgl(struct nvmet_req *req, off_t off, const void *buf,
		size_t len)
{
	if (sg_pcopy_from_buffer(req->sg, req->sg_cnt, buf, len, off) != len) {
		req->error_loc = offsetof(struct nvme_common_command, dptr);
		return NVME_SC_SGL_INVALID_DATA | NVME_STATUS_DNR;
	}
	return 0;
}

Previously, when offset < req->transfer_len and offset + len >
req->transfer_len, sg_pcopy_from_buffer() still copied the first
req->transfer_len - offset bytes of desc to fill req->sg up to
req->transfer_len before returning a short count, and the final header copy
at offset 0 reset status to 0.

Because nvmet transports allocate req->sg with capacity req->transfer_len
without zeroing it and transmit the full req->transfer_len on completion,
skipping nvmet_copy_to_sgl() when len > req->transfer_len - offset omits the
partial descriptor bytes that fit in the buffer and exposes req->transfer_len
- offset bytes of uninitialized kernel memory to the host.

Also, since req->sg has total capacity req->transfer_len,
sg_pcopy_from_buffer() only returns less than len when off + len exceeds
req->transfer_len. With the pre-check ensuring offset + len <=
req->transfer_len, can nvmet_copy_to_sgl(req, offset, desc, len) ever return
a non-zero status here?

[1] https://lore.kernel.org/r/20261031083505.4767-1-sreekuttan2156239@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031093420.3017-1-sreekuttan2156239@gmail.com?part=1

Re: [PATCH v2] nvmet: preserve ANA log page copy errors

[Sreeraj S Kurup]

Hi,

After further investigation, I found that the existing header-copy behavior is intentional for short ANA log-page reads. My proposed changes do not provide a functional fix.

I am withdrawing this patch. Please disregard both versions.

Sorry for the noise.

Thanks,
Sreeraj