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

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

[Karthikeyan KS]

vhost_scsi_map_to_sgl() pins guest buffer pages via
iov_iter_get_pages2() (FOLL_GET), which does not prevent
copy-on-write or page migration while DMA is in flight. Switch to
iov_iter_extract_pages() (FOLL_PIN), which does.

Cc: Jason Wang <jasowangio@gmail.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: virtualization@lists.linux.dev
Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>
---
 drivers/vhost/scsi.c | 49 ++++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 7a1f39a327da..e14b4da4ea9c 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -115,6 +115,8 @@ struct vhost_scsi_cmd {
 	u32 tvc_sgl_count;
 	u32 tvc_prot_sgl_count;
 	u32 copied_iov:1;
+	u32 tvc_need_unpin:1;
+	u32 tvc_prot_need_unpin:1;
 	const void *read_iov;
 	struct iov_iter *read_iter;
 	struct scatterlist *sgl;
@@ -433,8 +435,13 @@ static void vhost_scsi_release_cmd_res(struct se_cmd *se_cmd)
 
 			if (tv_cmd->copied_iov)
 				__free_page(page);
-			else
-				put_page(page);
+			else if (tv_cmd->tvc_need_unpin)
+				unpin_user_page(page);
+			/*
+			 * Else: iov_iter_extract_pages() took neither a ref
+			 * nor a pin on this page (ITER_KVEC/BVEC/etc), so
+			 * there is nothing to release here.
+			 */
 		}
 		kfree(tv_cmd->read_iter);
 		kfree(tv_cmd->read_iov);
@@ -443,8 +450,8 @@ static void vhost_scsi_release_cmd_res(struct se_cmd *se_cmd)
 	if (tv_cmd->tvc_prot_sgl_count) {
 		for_each_sgtable_sg(&tv_cmd->prot_table, sg, i) {
 			page = sg_page(sg);
-			if (page)
-				put_page(page);
+			if (page && tv_cmd->tvc_prot_need_unpin)
+				unpin_user_page(page);
 		}
 		sg_free_table_chained(&tv_cmd->prot_table, vs->inline_sg_cnt);
 	}
@@ -773,7 +780,8 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, u64 scsi_tag)
 
 static void vhost_scsi_revert_map_iov_to_sgl(struct iov_iter *iter,
 					     struct scatterlist *curr,
-					     struct scatterlist *end)
+					     struct scatterlist *end,
+					     bool need_unpin)
 {
 	size_t revert_bytes = 0;
 	struct page *page;
@@ -782,7 +790,8 @@ static void vhost_scsi_revert_map_iov_to_sgl(struct iov_iter *iter,
 		page = sg_page(curr);
 
 		if (page) {
-			put_page(page);
+			if (need_unpin)
+				unpin_user_page(page);
 			revert_bytes += curr->length;
 		}
 		/* Clear so we can re-use it for the copy path */
@@ -812,9 +821,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;
 
@@ -837,7 +846,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);
@@ -849,14 +858,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;
 	}
 
@@ -926,6 +938,7 @@ vhost_scsi_copy_iov_to_sgl(struct vhost_scsi_cmd *cmd, struct iov_iter *iter,
 	}
 
 	cmd->copied_iov = 1;
+	cmd->tvc_need_unpin = 0;
 	return 0;
 
 err:
@@ -950,11 +963,17 @@ vhost_scsi_map_iov_to_sgl(struct vhost_scsi_cmd *cmd, struct iov_iter *iter,
 	struct scatterlist *sg = sg_table->sgl;
 	int ret;
 
+	if (is_prot)
+		cmd->tvc_prot_need_unpin = iov_iter_extract_will_pin(iter);
+	else
+		cmd->tvc_need_unpin = iov_iter_extract_will_pin(iter);
+
 	while (iov_iter_count(iter)) {
 		ret = vhost_scsi_map_to_sgl(cmd, iter, sg_table, &sg, is_prot);
 		if (ret < 0) {
 			vhost_scsi_revert_map_iov_to_sgl(iter, sg_table->sgl,
-							 sg);
+					sg, is_prot ? cmd->tvc_prot_need_unpin :
+						      cmd->tvc_need_unpin);
 			return ret;
 		}
 	}
@@ -1392,7 +1411,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 		/*
 		 * If T10_PI header + payload is present, setup prot_iter values
 		 * and recalculate data_iter for vhost_scsi_mapal() mapping to
-		 * host scatterlists via get_user_pages_fast().
+		 * host scatterlists via iov_iter_extract_pages().
 		 */
 		if (t10_pi) {
 			if (v_req_pi.pi_bytesout) {
-- 
2.43.0

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

[sashiko-bot]

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