On Mon, May 11, 2026 at 08:06:44AM -0700, Stanislav Kinsburskii wrote:
> On Mon, May 11, 2026 at 01:48:56PM +0000, Anirudh Rayabharam wrote:
> > On Thu, May 07, 2026 at 03:43:09PM +0000, Stanislav Kinsburskii wrote:
> > > mshv_prepare_pinned_region() returns 0 (success) when mshv_region_map()
> > > fails on an unencrypted partition. The condition on the error path:
> > >
> > > if (ret && mshv_partition_encrypted(partition))
> > >
> > > only handles map failures for encrypted partitions — if the partition is
> > > not encrypted and the map fails, execution falls through to 'return 0',
> > > silently ignoring the error.
> > >
> > > Additionally, calling mshv_region_invalidate() inline on map failure
> > > zeroes the mreg_pages array before the caller's cleanup path
> > > (mshv_region_destroy) can call mshv_region_unmap(). Since unmap skips
> > > pages where mreg_pages[offset] is NULL, this can leave stale SLAT
> > > mappings for partially-mapped pages.
> > >
> > > Fix by returning immediately on success and falling through to error
> > > return on failure. For unencrypted partitions, the caller's
> > > mshv_region_destroy() handles unmap followed by invalidate in the
> > > correct order. For encrypted partitions where re-sharing fails, zero
> > > the page array without unpinning — the pages are inaccessible to the
> > > host and must not be unpinned, but zeroing prevents
> > > mshv_region_destroy() from attempting to unpin them.
> >
> > mshv_region_destroy() calls mshv_region_invalidate() which calls
> > mshv_region_invalidate_pages() which just does:
> >
> > static void mshv_region_invalidate_pages(struct mshv_mem_region *region,
> > u64 page_offset, u64
> > page_count)
> > {
> > if (region->mreg_type == MSHV_REGION_TYPE_MEM_PINNED)
> > unpin_user_pages(region->mreg_pages + page_offset,
> > page_count);
> >
> > memset(region->mreg_pages + page_offset, 0,
> > page_count * sizeof(struct page *));
> > }
> >
> > It doesn't checked for zeroed pages before unpinning.
> >
> > >
> > > Fixes: 621191d709b14 ("Drivers: hv: Introduce mshv_root module to expose
> > > /dev/mshv to VMMs")
> > > Signed-off-by: Stanislav Kinsburskii <[email protected]>
> > > ---
> > > drivers/hv/mshv_root_main.c | 26 ++++++++++++++++----------
> > > 1 file changed, 16 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> > > index 665d565899c15..7e4252b6bc65c 100644
> > > --- a/drivers/hv/mshv_root_main.c
> > > +++ b/drivers/hv/mshv_root_main.c
> > > @@ -1360,32 +1360,38 @@ static int mshv_prepare_pinned_region(struct
> > > mshv_mem_region *region)
> > > pt_err(partition,
> > > "Failed to unshare memory region (guest_pfn:
> > > %llu): %d\n",
> > > region->start_gfn, ret);
> > > - goto invalidate_region;
> > > + goto err_out;
> > > }
> > > }
> > >
> > > ret = mshv_region_map(region);
> > > - if (ret && mshv_partition_encrypted(partition)) {
> > > + if (ret)
> > > + goto share_region;
> > > +
> > > + return 0;
> > > +
> > > +share_region:
> > > + if (mshv_partition_encrypted(partition)) {
> > > int shrc;
> > >
> > > shrc = mshv_region_share(region);
> > > if (!shrc)
> > > - goto invalidate_region;
> > > + goto err_out;
> > >
> > > pt_err(partition,
> > > "Failed to share memory region (guest_pfn: %llu): %d\n",
> > > region->start_gfn, shrc);
> > > /*
> > > - * Don't unpin if marking shared failed because pages are no
> > > - * longer mapped in the host, ie root, anymore.
> > > + * Re-sharing failed — the pages remain inaccessible to the
> > > + * host. Zero the page array so that mshv_region_destroy()
> > > + * won't attempt to unpin them (leaking the page references
> > > + * is intentional; unpinning host-inaccessible pages would be
> > > + * unsafe).
> > > */
> >
> > Actually, mshv_region_destroy() also does mshv_region_share(). Maybe we
> > can remove it from here altogether. Either way, should this zeroing
> > logic be added there too so as not to crash the host by unpinning pages
> > that weren't marked shared?
> >
>
> Indeed.
> The issue with all this code is that we are leaking state in
> mshv_region_pin if we wimply return from it: we don't know if the pages
> are pinned or unshared (or mapped) in the destruction callback.
> We should either introduce a state variable to track this or just don't
> call the generic mshv_region_put on case of region creation error.
> The latter approch make mshv_map_user_memory idempotent and thus looks
> like a better way forward.
> What do you think?
I'm not sure I follow the latter approach. Omitting mshv_region_put()
would cause a dangling reference to the mshv_region right?
Thanks,
Anirudh.
