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

[sashiko-bot@kernel.org]

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