Re: [PATCH 03/23] nvmet: add context annotation for nvmet_ns::file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] nvmet: slba shift overflow in nvmet_file_execute_discard()
  [1]
--

commit 0b0e76ffccc07fe576097865269793cd78b566d9
Author: Nilay Shroff <nilay@linux.ibm.com>

nvmet: add context annotation for nvmet_ns::file

This commit annotates nvmet_ns::file with __guarded_by(&subsys->lock),
introduces the nvmet_ns_file() helper for lockless I/O hotpath accesses,
and adds __must_hold(&ns->subsys->lock) annotations to the file-backed
namespace helpers.

> diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
> index 695963dd932ee..4307232c9160c 100644
> --- a/drivers/nvme/target/io-cmd-file.c
> +++ b/drivers/nvme/target/io-cmd-file.c

[ ... ]

> @@ -293,7 +306,7 @@ static void nvmet_file_execute_discard(struct nvmet_req *req)
>  			break;
>  		}
>  
> -		ret = vfs_fallocate(req->ns->file, mode, offset, len);
> +		ret = vfs_fallocate(nvmet_ns_file(req->ns), mode, offset, len);
>  		if (ret && ret != -EOPNOTSUPP) {
>  			req->error_slba = le64_to_cpu(range.slba);
>  			status = errno_to_nvme_status(req, ret);
> @@ -347,7 +360,7 @@ static void nvmet_file_write_zeroes_work(struct work_struct *w)
>  		return;
>  	}
>  
> -	ret = vfs_fallocate(req->ns->file, mode, offset, len);
> +	ret = vfs_fallocate(nvmet_ns_file(req->ns), mode, offset, len);
>  	nvmet_req_complete(req, ret < 0 ? errno_to_nvme_status(req, ret) : 0);
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but can shifting slba by
req->ns->blksize_shift in nvmet_file_execute_discard() and
nvmet_file_write_zeroes_work() overflow offset [1]?

drivers/nvme/target/io-cmd-file.c:nvmet_file_execute_discard() {
	...
		offset = le64_to_cpu(range.slba) << req->ns->blksize_shift;
		len = le32_to_cpu(range.nlb);
		len <<= req->ns->blksize_shift;
		if (offset + len > req->ns->size) {
	...
}

drivers/nvme/target/io-cmd-file.c:nvmet_file_write_zeroes_work() {
	...
	offset = le64_to_cpu(write_zeroes->slba) << req->ns->blksize_shift;
	len = (((sector_t)le16_to_cpu(write_zeroes->length) + 1) <<
			req->ns->blksize_shift);

	if (unlikely(offset + len > req->ns->size)) {
	...
}

When a Discard or Write Zeroes command carries non-zero bits in the upper
blksize_shift bits of slba, the left shift discards those bits or wraps
the signed loff_t offset (or offset + len).  If the truncated result falls
within [0, req->ns->size - len], the bounds check is bypassed and
vfs_fallocate() punches holes or zeroes data at an unintended offset in
the backing file instead of failing with NVME_SC_CAP_EXCEEDED.  If offset
wraps to a negative value, vfs_fallocate() fails with -EINVAL and the
request completes with NVME_SC_INTERNAL rather than NVME_SC_CAP_EXCEEDED.

[1] https://sashiko.dev/#/bug/linux-db20adf5-cf8b-4a83-bea6-a689b8eaa9de

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109033102.982191-25-nilay@linux.ibm.com?part=3