RE: [PATCH] mshv: Fix error handling in mshv_region_populate_pages

[Michael Kelley]

From: Wei Liu <wei.liu@xxxxxxxxxx> Sent: Tuesday, March 17, 2026 11:20 PM
> 
> On Tue, Mar 17, 2026 at 09:56:07PM +0000, Michael Kelley wrote:
> > From: Stanislav Kinsburskii <skinsburskii@xxxxxxxxxxxxxxxxxxx> Sent: Tuesday, March 17, 2026 8:05 AM
> > >
> > > The current error handling has two issues:
> > >
> > > First, pin_user_pages_fast() can return a short pin count (less than
> > > requested but greater than zero) when it cannot pin all requested pages.
> > > This is treated as success, leading to partially pinned regions being
> > > used, which causes memory corruption.
> > >
> > > Second, when an error occurs mid-loop, already pinned pages from the
> > > current batch are not released before calling mshv_region_evict_pages(),
> > > causing a page reference leak.
> >
> > There's now an online LLM-based tool that is automatically reviewing
> > kernel patches.  For this patch, the results are here:
> >
> >
> https://sashiko.dev/#/patchset/177375989324.25621.6532741522672582851.stgit
> %40skinsburskii-cloud-desktop.internal.cloudapp.net
> >
> > It has flagged the commit message as incorrectly referencing the
> > function mshv_region_evict_pages(), which doesn't exist.
> >
> > FWIW, the announcement about sashiko.dev is here:
> >
> > https://lore.kernel.org/lkml/7ia4o6kmpj5s.fsf@xxxxxxxxxxxxxxxxxxxxx/
> >
> > Other than the commit message reference, this looks good to me.
> >
> > Reviewed-by: Michael Kelley <mhklinux@xxxxxxxxxxx>
> 
> The second point is written as if the code here should release the
> already pinned pages before calling mshv_region_invalidate_pages(), but
> the code actually relies on mshv_mem_region_invalidate_pages() to
> release the pages. The change here fixes the accounting.
> 
>  Second, when an error occurs mid-loop, already pinned pages from the
>  current batch are not accounted for before calling
>  mshv_region_invalidate_pages(), causing a page reference leak.
> 
> And queued up the patch to hyperv-fixes.

One other thing I noticed:  The "Subject" of the patch is wrong. It
mentions mshv_region_populate_pages(), but the function being
modified is actually mshv_region_pin().

Michael

> 
> Wei
> 
> >
> > >
> > > Fix by treating short pins as errors and explicitly unpinning the
> > > partial batch before cleanup.
> > >
> > > Signed-off-by: Stanislav Kinsburskii <skinsburskii@xxxxxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/hv/mshv_regions.c |    6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/hv/mshv_regions.c b/drivers/hv/mshv_regions.c
> > > index c28aac0726de..fdffd4f002f6 100644
> > > --- a/drivers/hv/mshv_regions.c
> > > +++ b/drivers/hv/mshv_regions.c
> > > @@ -314,15 +314,17 @@ int mshv_region_pin(struct mshv_mem_region *region)
> > >  		ret = pin_user_pages_fast(userspace_addr, nr_pages,
> > >  					  FOLL_WRITE | FOLL_LONGTERM,
> > >  					  pages);
> > > -		if (ret < 0)
> > > +		if (ret != nr_pages)
> > >  			goto release_pages;
> > >  	}
> > >
> > >  	return 0;
> > >
> > >  release_pages:
> > > +	if (ret > 0)
> > > +		done_count += ret;
> > >  	mshv_region_invalidate_pages(region, 0, done_count);
> > > -	return ret;
> > > +	return ret < 0 ? ret : -ENOMEM;
> > >  }
> > >
> > >  static int mshv_region_chunk_unmap(struct mshv_mem_region *region,
> > >
> > >
> >