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