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

[Magnus Kulke <magnuskulke@linux.microsoft.com>]

On Fri, Oct 30, 2026 at 11:18:23AM -0700, Wei Liu wrote:
> 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

ack,

magnus