Re: [PATCH RESEND] vhost-scsi: use FOLL_PIN for DMA page pinning

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] vhost-scsi: page pin leak in vhost_scsi_map_to_sgl() error path
--

commit 1042924e4de0195bca9a4409c2fafbb0c7c58e45
Author: Karthikeyan KS <karthiproffesional@gmail.com>

vhost-scsi: use FOLL_PIN for DMA page pinning

This commit switches vhost-scsi from iov_iter_get_pages2() (FOLL_GET) to
iov_iter_extract_pages() (FOLL_PIN) when mapping guest I/O buffers into
scatterlists, and updates the teardown and error paths to unpin the pages
with unpin_user_page().

> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 885fac5ed042e..8162d83bdfcc9 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c

[ ... ]

> @@ -842,9 +851,9 @@ vhost_scsi_map_to_sgl(struct vhost_scsi_cmd *cmd,
>  	size_t offset;
>  	unsigned int n, npages = 0;
>  
> -	bytes = iov_iter_get_pages2(iter, pages, LONG_MAX,
> -				VHOST_SCSI_PREALLOC_UPAGES, &offset);
> -	/* No pages were pinned */
> +	bytes = iov_iter_extract_pages(iter, &pages, LONG_MAX,
> +				VHOST_SCSI_PREALLOC_UPAGES, 0, &offset);
> +	/* No pages were extracted */
>  	if (bytes <= 0)
>  		return bytes < 0 ? bytes : -EFAULT;
>  
> @@ -867,7 +876,7 @@ vhost_scsi_map_to_sgl(struct vhost_scsi_cmd *cmd,
>  		    cmd->tvc_sgl_count > BIO_MAX_VECS) {
>  			WARN_ONCE(true,
>  				  "vhost-scsi detected misaligned IO. Performance may be degraded.");
> -			goto revert_iter_get_pages;
> +			goto revert_iter_extract_pages;
>  		}
>  
>  		sg_set_page(sg, pages[npages++], n, offset);
> @@ -879,14 +888,17 @@ vhost_scsi_map_to_sgl(struct vhost_scsi_cmd *cmd,
>  	*sgl = sg;
>  	return npages;
>  
> -revert_iter_get_pages:
> -	vhost_scsi_revert_map_iov_to_sgl(iter, *sgl, sg);
> +revert_iter_extract_pages:
> +	vhost_scsi_revert_map_iov_to_sgl(iter, *sgl, sg,
> +			is_prot ? cmd->tvc_prot_need_unpin : cmd->tvc_need_unpin);
>  
>  	iov_iter_revert(iter, bytes);
>  	while (bytes) {
>  		n = min_t(unsigned int, PAGE_SIZE, bytes);
>  
> -		put_page(pages[npages++]);
> +		if (is_prot ? cmd->tvc_prot_need_unpin : cmd->tvc_need_unpin)
> +			unpin_user_page(pages[npages]);
> +		npages++;
>  		bytes -= n;
>  	}
>  

[Severity: High]
Can the cleanup loop at revert_iter_extract_pages in vhost_scsi_map_to_sgl()
leak a FOLL_PIN page pin on the last page in pages[] when the misaligned-IO
check triggers on the first iteration with a non-zero offset?

In vhost_scsi_map_to_sgl(), iov_iter_extract_pages() extracts and pins
DIV_ROUND_UP(offset + bytes, PAGE_SIZE) pages into pages[], where pages[0]
only covers min_t(unsigned int, PAGE_SIZE - offset, bytes) bytes starting at
offset:

	while (bytes) {
		n = min_t(unsigned int, PAGE_SIZE - offset, bytes);
		...
		if (!is_prot &&
		    (offset & (SECTOR_SIZE - 1) || n & (SECTOR_SIZE - 1)) &&
		    cmd->tvc_sgl_count > BIO_MAX_VECS) {
			WARN_ONCE(true,
				  "vhost-scsi detected misaligned IO. Performance may be degraded.");
			goto revert_iter_extract_pages;
		}

		sg_set_page(sg, pages[npages++], n, offset);
		sg = sg_next(sg);
		bytes -= n;
		offset = 0;
	}

If this check fails on the first page (npages == 0) when offset > 0, it jumps
to revert_iter_extract_pages before bytes is reduced by PAGE_SIZE - offset
and before offset is reset to 0.

At revert_iter_extract_pages, the loop steps by PAGE_SIZE instead of
PAGE_SIZE - offset:

	while (bytes) {
		n = min_t(unsigned int, PAGE_SIZE, bytes);

		if (is_prot ? cmd->tvc_prot_need_unpin : cmd->tvc_need_unpin)
			unpin_user_page(pages[npages]);
		npages++;
		bytes -= n;
	}

When offset + bytes crosses an extra page boundary (for example, offset = 3840
and bytes = 512, which pins 2 pages across the boundary while bytes <=
PAGE_SIZE), the loop only runs DIV_ROUND_UP(bytes, PAGE_SIZE) iterations
instead of DIV_ROUND_UP(offset + bytes, PAGE_SIZE) iterations, leaving the
last page in pages[] pinned.

Should the cleanup loop account for offset on the first iteration (using
min_t(unsigned int, PAGE_SIZE - offset, bytes) and then setting offset = 0),
matching the main loop?

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