On Wed, Oct 21, 2026 at 06:27:00PM +0000, [email protected] 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 <[email protected]> > > 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/[email protected]?part=1
