Re: [PATCH v4] drivers/hv: remove deposited pages from direct map

[Wei Liu <wei.liu@kernel.org>]

On Wed, Oct 28, 2026 at 03:54:30PM -0700, Mukesh R wrote:
> On 9/30/26 05:10, Magnus Kulke wrote:
> > hv_call_deposit_pages() donates pages (deposit) to hypervisor for L2
> > guest on L1VH systems via HVCALL_DEPOSIT_MEMORY. The hypervisor takes
> > ownership of those pages and per contract revokes root partition
> > access to them, raising a #GP on access from the L1VH root partition.
> > 
> > However, the pages remain mapped in the kernel direct map, so kernel
> > code may still access them even though the hypervisor has revoked
> > access.
> > 
> > Helpers such as "load_unaligned_zeropad()" deliberately read past the
> > end of a buffer and across page boundaries. A read into an unmapped
> > page is tolerated and triggers a #PF, for which the kernel executes
> > a fixup in the exception table.
> > 
> > If such a call steps into a page that has been deposited, the access
> > raises a #GP by the hypervisor from which the above handler cannot
> > recover and the kernel panics:
> > 
> >    Oops: general protection fault, maybe for address 0xff1100941a3dfffc
> >    RIP: 0010:csum_partial+0xe5/0x110
> > 
> > This condition will appear on L1VH system that have created L2
> > partitions (and hence deposited pages) and exercise networking code
> > paths such as csum_partial() can trigger this condition when a buffer
> > ends close to a page boundary (e.g fffc in the above example).
> > 
> > The fix is to remove the deposited pages from the direct map before
> > they are passed to the hypervisor, and restore them when the hypervisor
> > returns them again.
> > 
> > We want to avoid flushing the TLB in the loop, so we use the _noflush()
> > variant of set_direct_map_valid() when marking a deposited page invalid
> > and flush the affected page ranges in one go ourselves. In the opposite
> > direction this is not required:
> > 
> >    > If a paging-structure entry is modified to change the P flag from
> >    > 0 to 1, no invalidation is necessary. This is because no TLB entry
> >    > or paging-structure cache entry is created with information from a
> >    > paging-structure entry in which the P flag is 0.
> > 
> > (Intel SDM Vol. 3, 4.10.4.3)
> > 
> > Signed-off-by: Magnus Kulke <magnuskulke@linux.microsoft.com>
> > ---
> > Changes since v3:
> > - Batch contiguous pages in the restore path
> > - Adjusted comment about failure modes
> > 
> > Changes since v2:
> > - Checkpatch format fix
> > 
> > Changes since RFC:
> > - Handle direct-map restoration failures without returning unmapped
> >    pages to the allocator.
> > - Move freeing of withdrawn pages into the restoration helper.
> > ---
> >   drivers/hv/hv_proc.c           | 83 +++++++++++++++++++++++++++++++++-
> >   drivers/hv/mshv_root_hv_call.c |  4 +-
> >   include/asm-generic/mshyperv.h |  4 ++
> >   3 files changed, 87 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
> > index 57b2c64197cb..b056051a5ac5 100644
> > --- a/drivers/hv/hv_proc.c
> > +++ b/drivers/hv/hv_proc.c
> > @@ -7,7 +7,9 @@
> >   #include <linux/cpuhotplug.h>
> >   #include <linux/minmax.h>
> >   #include <linux/export.h>
> > +#include <linux/set_memory.h>
> >   #include <asm/mshyperv.h>
> > +#include <asm/tlbflush.h>
> >   /*
> >    * See struct hv_deposit_memory. The first u64 is partition ID, the rest
> > @@ -15,6 +17,35 @@
> >    */
> >   #define HV_DEPOSIT_MAX (HV_HYP_PAGE_SIZE / sizeof(u64) - 1)
> > +/*
> > + * Add or remove a set of physically contiguous page runs from the kernel
> > + * direct map. Once a page has been deposited the hypervisor owns it and
> > + * revokes root partition access to it.
> > + */
> > +static int hv_deposit_update_direct_map(struct page **pages, int *counts,
> > +					int num_allocations, bool valid)
> > +{
> > +	int i, err, ret = 0;
> > +
> > +	for (i = 0; i < num_allocations; ++i) {
> > +		err = set_direct_map_valid_noflush(pages[i], counts[i], valid);
> > +		if (err && !ret)
> > +			ret = err;
> > +	}
> 
> I realize I am late to the review, so if too late, please ignore.
> If the caller is going to not continue on error but will revert,
> what is the reason to keep going on error then?
> 
> And this function should undo it itself if there is error instead
> of forcing the caller to do it imo. It could just undo the part which
> is done.
> 

Magnus, while you work on rebasing the patch, perhaps you can also
address this.

Wei