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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Oct 21, 2026 at 06:27:00PM +0000, sashiko-bot@kernel.org wrote:
> 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


Karthikeyan were you going to address this review?

> --
> 
> 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