[PATCH V0 0/2] Redesign memory deposit logic

[PATCH V0 0/2] Redesign memory deposit logic

[Mukesh R]

This small series redesigns the memory design logic when Linux runs on
Hyper-V hypervisor as a privileged VM. It has many issues currently. It 
is split in two patches, the first patch stubs out the functions which 
makes the 2nd patch review much easier and nicer.

Thanks,
-Mukesh


Mukesh R (2):
  mshv: Stub out deposit memory functions
  mshv: Redesign hypervisor memory deposit logic

 drivers/hv/hv_proc.c           | 240 ++++++++++++++++++++-------------
 drivers/hv/mshv_root_hv_call.c |  10 +-
 include/asm-generic/mshyperv.h |   5 -
 3 files changed, 144 insertions(+), 111 deletions(-)

-- 
2.51.2.vfs.0.1

[PATCH V0 1/2] mshv: Stub out deposit memory functions

[Mukesh R]

Stub out deposit memory functions so adding of new redesigned functions
is much eaiser to review.

Signed-off-by: Mukesh R <mrathor@linux.microsoft.com>
---
 drivers/hv/hv_proc.c | 126 ++-----------------------------------------
 1 file changed, 3 insertions(+), 123 deletions(-)

diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
index 57b2c64197cb..d5ce9a032e9c 100644
--- a/drivers/hv/hv_proc.c
+++ b/drivers/hv/hv_proc.c
@@ -9,135 +9,15 @@
 #include <linux/export.h>
 #include <asm/mshyperv.h>
 
-/*
- * See struct hv_deposit_memory. The first u64 is partition ID, the rest
- * are GPAs.
- */
-#define HV_DEPOSIT_MAX (HV_HYP_PAGE_SIZE / sizeof(u64) - 1)
-
-/* Deposits exact number of pages. Must be called with interrupts enabled.  */
 int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
 {
-	struct page **pages, *page;
-	int *counts;
-	int num_allocations;
-	int i, j, page_count;
-	int order;
-	u64 status;
-	int ret;
-	u64 base_pfn;
-	struct hv_deposit_memory *input_page;
-	unsigned long flags;
-
-	if (num_pages > HV_DEPOSIT_MAX)
-		return -E2BIG;
-	if (!num_pages)
-		return 0;
-
-	/* One buffer for page pointers and counts */
-	page = alloc_page(GFP_KERNEL);
-	if (!page)
-		return -ENOMEM;
-	pages = page_address(page);
-
-	counts = kzalloc_objs(int, HV_DEPOSIT_MAX);
-	if (!counts) {
-		free_page((unsigned long)pages);
-		return -ENOMEM;
-	}
-
-	/* Allocate all the pages before disabling interrupts */
-	i = 0;
-
-	while (num_pages) {
-		/* Find highest order we can actually allocate */
-		order = 31 - __builtin_clz(num_pages);
-
-		while (1) {
-			pages[i] = alloc_pages_node(node, GFP_KERNEL, order);
-			if (pages[i])
-				break;
-			if (!order) {
-				ret = -ENOMEM;
-				num_allocations = i;
-				goto err_free_allocations;
-			}
-			--order;
-		}
-
-		split_page(pages[i], order);
-		counts[i] = 1 << order;
-		num_pages -= counts[i];
-		i++;
-	}
-	num_allocations = i;
-
-	local_irq_save(flags);
-
-	input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
-
-	input_page->partition_id = partition_id;
-
-	/* Populate gpa_page_list - these will fit on the input page */
-	for (i = 0, page_count = 0; i < num_allocations; ++i) {
-		base_pfn = page_to_pfn(pages[i]);
-		for (j = 0; j < counts[i]; ++j, ++page_count)
-			input_page->gpa_page_list[page_count] = base_pfn + j;
-	}
-	status = hv_do_rep_hypercall(HVCALL_DEPOSIT_MEMORY,
-				     page_count, 0, input_page, NULL);
-	local_irq_restore(flags);
-	if (!hv_result_success(status)) {
-		hv_status_err(status, "\n");
-		ret = hv_result_to_errno(status);
-		goto err_free_allocations;
-	}
-
-	ret = 0;
-	goto free_buf;
-
-err_free_allocations:
-	for (i = 0; i < num_allocations; ++i) {
-		base_pfn = page_to_pfn(pages[i]);
-		for (j = 0; j < counts[i]; ++j)
-			__free_page(pfn_to_page(base_pfn + j));
-	}
-
-free_buf:
-	free_page((unsigned long)pages);
-	kfree(counts);
-	return ret;
+	return -ENOTSUPP;
 }
 EXPORT_SYMBOL_GPL(hv_call_deposit_pages);
 
-int hv_deposit_memory_node(int node, u64 partition_id,
-			   u64 hv_status)
+int hv_deposit_memory_node(int node, u64 partition_id, u64 hv_status)
 {
-	u32 num_pages = 1;
-
-	switch (hv_result(hv_status)) {
-	case HV_STATUS_INSUFFICIENT_MEMORY:
-		break;
-	case HV_STATUS_INSUFFICIENT_CONTIGUOUS_MEMORY:
-		num_pages = HV_MAX_CONTIGUOUS_ALLOCATION_PAGES;
-		break;
-
-	case HV_STATUS_INSUFFICIENT_CONTIGUOUS_ROOT_MEMORY:
-		num_pages = HV_MAX_CONTIGUOUS_ALLOCATION_PAGES;
-		fallthrough;
-	case HV_STATUS_INSUFFICIENT_ROOT_MEMORY:
-		if (!hv_root_partition()) {
-			hv_status_err(hv_status, "Unexpected root memory deposit\n");
-			return -ENOMEM;
-		}
-		partition_id = HV_PARTITION_ID_SELF;
-		break;
-
-	default:
-		hv_status_err(hv_status, "Unexpected!\n");
-		return -ENOMEM;
-	}
-	return hv_call_deposit_pages(node, partition_id, num_pages);
+	return -ENOTSUPP;
 }
 EXPORT_SYMBOL_GPL(hv_deposit_memory_node);
 
-- 
2.51.2.vfs.0.1

[PATCH V0 2/2] mshv: Redesign hypervisor memory deposit logic

[Mukesh R]

There are few issues/bugs in the deposit memory implementation:
 o It is very slow
 o Contiguous range requirement is ignored, and is critical bug
 o An incorrect assumption is made that contiguous memory size would
   always be power of 2.
 o Two pages are allocated, only one is really needed. This adds to
   overhead.
 o For a 512 page deposit, the allocation is split into two: one for 511
   and second for 1. Thus, an order 9 allocation never happens. A
   contiguous 2M range would significantly improve performance in the
   hypervisor.
 o Since a page is already allocated to collect the frames, there is
   not really a need to use per cpu input page, and hence avoid local
   irq disable.

All of above is addressed by:
 o Start with a full 2M range alloc, thus getting contiguous if available.
 o Allocate only one page in the deposit function and collect 511 pfns
   there. Just use a local variable for last pfn.
 o Use the page as input to hypercall. Since this page is allocated, irq
   disable can be avoided helping speed up the deposit.
 o Fix the contiguous requirement by using kmalloc in such case.
 o A minimum deposit of 2M done universally to overcome bad performance 
   overheads. The hypervisor will always first reuse any unused memory 
   that was previously deposited before asking for more.

Signed-off-by: Mukesh R <mrathor@linux.microsoft.com>
---
 drivers/hv/hv_proc.c           | 180 +++++++++++++++++++++++++++++++--
 drivers/hv/mshv_root_hv_call.c |  10 +-
 include/asm-generic/mshyperv.h |   5 -
 3 files changed, 174 insertions(+), 21 deletions(-)

diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
index d5ce9a032e9c..fd74c286e612 100644
--- a/drivers/hv/hv_proc.c
+++ b/drivers/hv/hv_proc.c
@@ -9,15 +9,182 @@
 #include <linux/export.h>
 #include <asm/mshyperv.h>
 
-int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
+#define HV_DEPOSIT_MAX 512
+#define HV_DEPOSIT_INP_MAX ((HV_HYP_PAGE_SIZE -  \
+	offsetof(struct hv_deposit_memory, gpa_page_list)) / sizeof(u64))
+
+static int hv_alloc_contig_pages(int node, u64 *pfna, u64 *lastpfnp,
+				 int num_pages)
+{
+	void *p;
+	int i, tmp;
+	ulong pfn;
+	size_t size = num_pages * HV_HYP_PAGE_SIZE;
+
+	if (num_pages > HV_DEPOSIT_MAX ||
+	    (num_pages == HV_DEPOSIT_MAX && lastpfnp == NULL))
+		return -EINVAL;
+
+	p = kmalloc_node(size, GFP_KERNEL, node);
+	if (p == NULL)
+		return -ENOMEM;
+
+	pfn = PFN_DOWN(virt_to_phys(p));
+	tmp = min(num_pages, HV_DEPOSIT_INP_MAX);
+
+	for (i = 0; i < tmp; i++, pfn++)
+		pfna[i] = pfn;
+
+	if (num_pages == HV_DEPOSIT_MAX)
+		*lastpfnp = pfn;
+
+	return num_pages;
+}
+
+
+/*
+ * Allocate free pages for deposit to hypervisor. pfna[] must be large enough
+ * to hold HV_DEPOSIT_INP_MAX (511) pages. If num_pages is 512, return last
+ * pfn in lastpfn.
+ *
+ * Returns : -ENOMEM if zero allocated, else number of pages allocated
+ */
+static int hv_alloc_dep_pages(int node, u64 *pfna, u64 *lastpfnp, int num_pages)
+{
+	struct page *page;
+	int num_allocd, count = 0;
+
+	/* Published ABI, enforce its immutability. */
+	BUILD_BUG_ON(HV_DEPOSIT_INP_MAX != 511);
+
+	if (num_pages > HV_DEPOSIT_MAX ||
+	    (num_pages == HV_DEPOSIT_MAX && lastpfnp == NULL))
+		return -EINVAL;
+
+	while (num_pages) {
+		/* Find highest order we can actually allocate */
+		int order = 31 - __builtin_clz(num_pages);
+
+		while (1) {
+			page = alloc_pages_node(node, GFP_KERNEL, order);
+			if (page || order == 0)
+				break;
+
+			order--;
+		}
+
+		if (page == NULL)
+			break;
+
+		split_page(page, order);
+		num_allocd = 1 << order;
+		num_pages -= num_allocd;
+
+		while (num_allocd && count < HV_DEPOSIT_INP_MAX) {
+			pfna[count++] = page_to_pfn(page++);
+			num_allocd--;
+		}
+
+		if (num_allocd-- && count == HV_DEPOSIT_INP_MAX) {
+			*lastpfnp = page_to_pfn(page);
+			count++;
+			break;
+		}
+	}
+
+	return count ? count : -ENOMEM;
+}
+
+/*
+ * Deposit memory in the hypervisor. A contiguous 2M worth of pfns is utmost
+ * desired, but short of that, we deposit whatever contiguous chunks we can
+ * get.
+ */
+static int hv_call_deposit_memory(int node, u64 partition_id, bool contiguous)
 {
-	return -ENOTSUPP;
+	struct hv_deposit_memory *hc_input;
+	int i, rc, num_pages;
+	u64 status, *pfna, lastpfn = 0;
+
+	BUILD_BUG_ON(HV_MAX_CONTIGUOUS_ALLOCATION_PAGES > HV_DEPOSIT_MAX);
+
+	if (contiguous)
+		num_pages = HV_MAX_CONTIGUOUS_ALLOCATION_PAGES;
+	else
+		num_pages = HV_DEPOSIT_MAX;
+
+	hc_input = (struct hv_deposit_memory *)get_zeroed_page(GFP_KERNEL);
+	if (hc_input == NULL)
+		return -ENOMEM;
+
+	hc_input->partition_id = partition_id;
+	pfna = hc_input->gpa_page_list;
+
+	if (contiguous)
+		rc = hv_alloc_contig_pages(node, pfna, &lastpfn, num_pages);
+	else
+		rc = hv_alloc_dep_pages(node, pfna, &lastpfn, num_pages);
+	if (rc < 0)
+		goto out_free;
+
+	num_pages = rc;
+	if (num_pages > HV_DEPOSIT_INP_MAX)
+		num_pages = HV_DEPOSIT_INP_MAX;
+
+	/* We are not using hyperv_pcpu_input_arg, so no need to disable */
+
+	status = hv_do_rep_hypercall(HVCALL_DEPOSIT_MEMORY, num_pages,
+				     0, hc_input, NULL);
+	if (!hv_result_success(status)) {
+		hv_status_err(status, "\n");
+		rc = hv_result_to_errno(status);
+		goto out_free_dep_pages;
+	}
+
+	if (lastpfn) {
+		hc_input->gpa_page_list[0] = lastpfn;
+		status = hv_do_rep_hypercall(HVCALL_DEPOSIT_MEMORY, 1, 0,
+					     hc_input, NULL);
+		if (!hv_result_success(status))
+			/* We deposited some earlier, so just free this */
+			__free_page(pfn_to_page(lastpfn));
+	}
+
+	free_page((unsigned long)hc_input);
+	return 0;
+
+out_free_dep_pages:
+	for (i = 0; i < num_pages; i++)
+		__free_page(pfn_to_page(pfna[i]));
+	if (lastpfn)
+		__free_page(pfn_to_page(lastpfn));
+
+out_free:
+	free_page((unsigned long)hc_input);
+	return rc;
 }
-EXPORT_SYMBOL_GPL(hv_call_deposit_pages);
 
-int hv_deposit_memory_node(int node, u64 partition_id, u64 hv_status)
+int hv_deposit_memory_node(int node, u64 pt_id, u64 hv_status)
 {
-	return -ENOTSUPP;
+	int result = hv_result(hv_status);
+	bool contiguous = false;
+
+	if (result == HV_STATUS_INSUFFICIENT_ROOT_MEMORY ||
+	    result == HV_STATUS_INSUFFICIENT_CONTIGUOUS_ROOT_MEMORY) {
+		if (!hv_root_partition()) {
+			hv_status_err(hv_status,
+				      "Unexpected root memory deposit\n");
+			return -EINVAL;
+		}
+
+		pt_id = HV_PARTITION_ID_SELF;
+	}
+
+	if (result == HV_STATUS_INSUFFICIENT_CONTIGUOUS_MEMORY ||
+	    result == HV_STATUS_INSUFFICIENT_CONTIGUOUS_ROOT_MEMORY)
+		contiguous = true;
+
+	return hv_call_deposit_memory(node, pt_id, contiguous);
 }
 EXPORT_SYMBOL_GPL(hv_deposit_memory_node);
 
@@ -85,8 +252,7 @@ int hv_call_create_vp(int node, u64 partition_id, u32 vp_index, u32 flags)
 
 	/* Root VPs don't seem to need pages deposited */
 	if (partition_id != hv_current_partition_id) {
-		/* The value 90 is empirically determined. It may change. */
-		ret = hv_call_deposit_pages(node, partition_id, 90);
+		ret = hv_call_deposit_memory(node, partition_id, false);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
index cb55d4d4be2e..b8d199f95299 100644
--- a/drivers/hv/mshv_root_hv_call.c
+++ b/drivers/hv/mshv_root_hv_call.c
@@ -15,8 +15,6 @@
 #include "mshv_root.h"
 
 /* Determined empirically */
-#define HV_INIT_PARTITION_DEPOSIT_PAGES 208
-#define HV_MAP_GPA_DEPOSIT_PAGES	256
 #define HV_UMAP_GPA_PAGES		512
 
 #define HV_PAGE_COUNT_2M_ALIGNED(pg_count) (!((pg_count) & (0x200 - 1)))
@@ -140,11 +138,6 @@ int hv_call_initialize_partition(u64 partition_id)
 
 	input.partition_id = partition_id;
 
-	ret = hv_call_deposit_pages(NUMA_NO_NODE, partition_id,
-				    HV_INIT_PARTITION_DEPOSIT_PAGES);
-	if (ret)
-		return ret;
-
 	do {
 		status = hv_do_fast_hypercall8(HVCALL_INITIALIZE_PARTITION,
 					       *(u64 *)&input);
@@ -248,8 +241,7 @@ static int hv_do_map_gpa_hcall(u64 partition_id, u64 gfn, u64 page_struct_count,
 		completed = hv_repcomp(status);
 
 		if (hv_result_needs_memory(status)) {
-			ret = hv_call_deposit_pages(NUMA_NO_NODE, partition_id,
-						    HV_MAP_GPA_DEPOSIT_PAGES);
+			ret = hv_deposit_memory(partition_id, status);
 			if (ret)
 				break;
 
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index bf601d67cecb..c16abaecb65e 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -345,7 +345,6 @@ static inline bool hv_parent_partition(void)
 
 bool hv_result_needs_memory(u64 status);
 int hv_deposit_memory_node(int node, u64 partition_id, u64 status);
-int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages);
 int hv_call_add_logical_proc(int node, u32 lp_index, u32 acpi_id);
 int hv_call_notify_all_processors_started(void);
 bool hv_lp_exists(u32 lp_index);
@@ -360,10 +359,6 @@ static inline int hv_deposit_memory_node(int node, u64 partition_id, u64 status)
 {
 	return -EOPNOTSUPP;
 }
-static inline int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
-{
-	return -EOPNOTSUPP;
-}
 static inline int hv_call_add_logical_proc(int node, u32 lp_index, u32 acpi_id)
 {
 	return -EOPNOTSUPP;
-- 
2.51.2.vfs.0.1

Re: [PATCH V0 2/2] mshv: Redesign hypervisor memory deposit logic

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect memory allocator and cleanup for contiguous pages leads to memory corruption and kernel crashes.
- [High] Unhandled partial success in the deposit memory rep hypercall leads to memory corruption.
- [Medium] Optimistic high-order allocations missing __GFP_NOWARN will spam the kernel log.
--

--- Patch [2]: [PATCH V0 2/2] mshv: Redesign hypervisor memory deposit logic ---
commit 8f3cb1a3334e617608894587fd59c0c23c46b19a
Author: Mukesh R <mrathor@linux.microsoft.com>

mshv: Redesign hypervisor memory deposit logic

This commit redesigns the hypervisor memory deposit logic to improve
performance. It attempts a contiguous 2M allocation and minimizes allocations
to speed up the deposit process while handling contiguous requirements.

> diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
> index d5ce9a032e9c7..fd74c286e6129 100644
> --- a/drivers/hv/hv_proc.c
> +++ b/drivers/hv/hv_proc.c
> @@ -9,15 +9,182 @@
>  #include <linux/export.h>
>  #include <asm/mshyperv.h>
>  
> -int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
> +#define HV_DEPOSIT_MAX 512
> +#define HV_DEPOSIT_INP_MAX ((HV_HYP_PAGE_SIZE -  \
> +	offsetof(struct hv_deposit_memory, gpa_page_list)) / sizeof(u64))
> +
> +static int hv_alloc_contig_pages(int node, u64 *pfna, u64 *lastpfnp,
> +				 int num_pages)
> +{
> +	void *p;
> +	int i, tmp;
> +	ulong pfn;
> +	size_t size = num_pages * HV_HYP_PAGE_SIZE;
> +
> +	if (num_pages > HV_DEPOSIT_MAX ||
> +	    (num_pages == HV_DEPOSIT_MAX && lastpfnp == NULL))
> +		return -EINVAL;
> +
> +	p = kmalloc_node(size, GFP_KERNEL, node);

[Severity: High]
Does this kmalloc_node() allocation lead to memory corruption or crashes?

For small requests, kmalloc_node() returns memory from a slab cache, sharing
the physical page with other objects. For large requests (e.g. 512 pages/2MB),
it returns a compound page.

When these PFNs are handed to the hypervisor, the hypervisor takes full
physical ownership of the page. If it's a slab-managed page, won't this corrupt
other kernel objects residing in the same slab page?

Additionally, when this is a compound page, isn't there a risk in the error
path below?

hv_alloc_contig_pages()
    p = kmalloc_node(size, ...);

hv_call_deposit_memory()
    __free_page(pfn_to_page(lastpfn));

Since this lastpfn is a tail page of a compound allocation, won't __free_page()
trigger a VM_BUG_ON_PAGE(PageTail(page), page) and crash the kernel?

> +	if (p == NULL)
> +		return -ENOMEM;
> +
> +	pfn = PFN_DOWN(virt_to_phys(p));
> +	tmp = min(num_pages, HV_DEPOSIT_INP_MAX);
> +
> +	for (i = 0; i < tmp; i++, pfn++)
> +		pfna[i] = pfn;
> +
> +	if (num_pages == HV_DEPOSIT_MAX)
> +		*lastpfnp = pfn;
> +
> +	return num_pages;
> +}
> +
> +
> +/*
> + * Allocate free pages for deposit to hypervisor. pfna[] must be large enough
> + * to hold HV_DEPOSIT_INP_MAX (511) pages. If num_pages is 512, return last
> + * pfn in lastpfn.
> + *
> + * Returns : -ENOMEM if zero allocated, else number of pages allocated
> + */
> +static int hv_alloc_dep_pages(int node, u64 *pfna, u64 *lastpfnp, int num_pages)
> +{
> +	struct page *page;
> +	int num_allocd, count = 0;
> +
> +	/* Published ABI, enforce its immutability. */
> +	BUILD_BUG_ON(HV_DEPOSIT_INP_MAX != 511);
> +
> +	if (num_pages > HV_DEPOSIT_MAX ||
> +	    (num_pages == HV_DEPOSIT_MAX && lastpfnp == NULL))
> +		return -EINVAL;
> +
> +	while (num_pages) {
> +		/* Find highest order we can actually allocate */
> +		int order = 31 - __builtin_clz(num_pages);
> +
> +		while (1) {
> +			page = alloc_pages_node(node, GFP_KERNEL, order);

[Severity: Medium]
Should this high-order allocation attempt include the __GFP_NOWARN flag?

High-order allocations frequently fail on fragmented systems. Because this
failure is fully expected and handled gracefully by the fallback loop
decrementing the order, this warning is spurious and could needlessly spam the
kernel log, or even crash the system if panic_on_warn is enabled.

> +			if (page || order == 0)
> +				break;
> +
> +			order--;
> +		}
> +
> +		if (page == NULL)
> +			break;

[ ... ]

> +static int hv_call_deposit_memory(int node, u64 partition_id, bool contiguous)
>  {

[ ... ]

> +	/* We are not using hyperv_pcpu_input_arg, so no need to disable */
> +
> +	status = hv_do_rep_hypercall(HVCALL_DEPOSIT_MEMORY, num_pages,
> +				     0, hc_input, NULL);
> +	if (!hv_result_success(status)) {
> +		hv_status_err(status, "\n");
> +		rc = hv_result_to_errno(status);
> +		goto out_free_dep_pages;
> +	}

[Severity: High]
Could this error path cause memory corruption if the rep hypercall partially
completes?

Rep hypercalls can partially complete (e.g., due to a timeout or interrupt in
the hypervisor). In this case, hv_result_success(status) returns false, but
hv_repcomp(status) indicates the number of successfully processed elements.

Since the code treats any non-success status as a complete failure, jumping
directly to out_free_dep_pages without checking hv_repcomp(status), won't the
error path blindly free pages that were already consumed by the hypervisor?

This could lead to Linux and the hypervisor concurrently using the same
physical pages.

> +
> +	if (lastpfn) {
> +		hc_input->gpa_page_list[0] = lastpfn;
> +		status = hv_do_rep_hypercall(HVCALL_DEPOSIT_MEMORY, 1, 0,
> +					     hc_input, NULL);
> +		if (!hv_result_success(status))
> +			/* We deposited some earlier, so just free this */
> +			__free_page(pfn_to_page(lastpfn));
> +	}
> +
> +	free_page((unsigned long)hc_input);
> +	return 0;
> +
> +out_free_dep_pages:
> +	for (i = 0; i < num_pages; i++)
> +		__free_page(pfn_to_page(pfna[i]));
> +	if (lastpfn)
> +		__free_page(pfn_to_page(lastpfn));
> +
> +out_free:
> +	free_page((unsigned long)hc_input);
> +	return rc;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812000507.3809046-1-mrathor@linux.microsoft.com?part=2

Re: [PATCH V0 1/2] mshv: Stub out deposit memory functions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Intentional breakage of core driver functionality, boot crashes, and UAPI violation via hardcoded error stubs.

Pre-existing issues:
- [Critical] A partial failure during the HVCALL_MAP_GPA_PAGES batch processing fails to unmap successfully mapped pages, leaving the guest with valid GPA mappings to host pages that are subsequently unpinned and reallocated.
- [Critical] A partial failure during HVCALL_RELEASE_SPARSE_SPA_PAGE_HOST_ACCESS causes the host to permanently lose access to pinned memory, leading to a host kernel crash when the pages are unpinned and reused.
--

--- Patch [1]: [PATCH V0 1/2] mshv: Stub out deposit memory functions ---
commit 31673114cbc4abf8a6e554e641203a81af3ed70c
Author: Mukesh R <mrathor@linux.microsoft.com>

mshv: Stub out deposit memory functions

This commit replaces the implementations of hv_call_deposit_pages() and
hv_deposit_memory_node() with stubs that unconditionally return -ENOTSUPP.
The commit message states this is to make adding new redesigned functions
easier to review.

> diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
> index 57b2c64197cb9..d5ce9a032e9c7 100644
> --- a/drivers/hv/hv_proc.c
> +++ b/drivers/hv/hv_proc.c
> @@ -9,135 +9,15 @@
>  #include <linux/export.h>
>  #include <asm/mshyperv.h>
>  
[ ... ]
>  int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
>  {
[ ... ]
> -free_buf:
> -	free_page((unsigned long)pages);
> -	kfree(counts);
> -	return ret;
> +	return -ENOTSUPP;
>  }

[Severity: High]
Does unconditionally returning -ENOTSUPP here break core driver
functionality and cause boot failures?

If hv_smp_prepare_cpus() in arch/x86/kernel/cpu/mshyperv.c requires memory
during early CPU setup, the -ENOTSUPP return will trigger a BUG_ON(ret),
resulting in a fatal early boot crash.

Additionally, hv_call_initialize_partition() in
drivers/hv/mshv_root_hv_call.c unconditionally calls this function.
Returning -ENOTSUPP (which is internal to the kernel) will propagate up
to the mshv_partition_ioctl return value and leak to userspace as an
unrecognized error, while guaranteeing partition creation always fails.

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential problem in
drivers/hv/mshv_root_hv_call.c:hv_do_map_gpa_hcall():

		} else if (!hv_result_success(status)) {
			ret = hv_result_to_errno(status);
			break;
		}

		done += completed;

If a partial failure occurs during the HVCALL_MAP_GPA_PAGES batch processing,
the break statement skips the done += completed update.

Could this permanently lose track of the successfully mapped pages in this
batch?

If the stale done value is used during cleanup, successfully mapped pages
would not be unmapped, leaving the guest with valid GPA mappings to host
pages that are subsequently unpinned and reallocated.

[Severity: Critical]
This is a pre-existing issue, but there appears to be another error handling
concern in drivers/hv/mshv_root_hv_call.c:hv_call_modify_spa_host_access():

		completed = hv_repcomp(status);

		if (!hv_result_success(status))
			return hv_result_to_errno(status);

		done += completed;

If this hypercall partially succeeds but returns an error for a subsequent
page, the function returns immediately and discards the completed count.

Can this prevent the caller from knowing which pages need their host access
restored?

If the caller treats the region as failed and unpins the pages without
restoring host access, the host kernel allocator could reuse pages that lack
host access. This could lead to a hardware-level access violation and a fatal
kernel panic when the host accesses them.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812000507.3809046-1-mrathor@linux.microsoft.com?part=1

Re: [PATCH V0 0/2] Redesign memory deposit logic

[Mukesh R]

On 8/11/26 17:05, Mukesh R wrote:
> This small series redesigns the memory design logic when Linux runs on
> Hyper-V hypervisor as a privileged VM. It has many issues currently. It
> is split in two patches, the first patch stubs out the functions which
> makes the 2nd patch review much easier and nicer.
> 
> Thanks,
> -Mukesh
> 
> 
> Mukesh R (2):
>    mshv: Stub out deposit memory functions
>    mshv: Redesign hypervisor memory deposit logic
> 
>   drivers/hv/hv_proc.c           | 240 ++++++++++++++++++++-------------
>   drivers/hv/mshv_root_hv_call.c |  10 +-
>   include/asm-generic/mshyperv.h |   5 -
>   3 files changed, 144 insertions(+), 111 deletions(-)
> 

Please ignore this V0 series in light of some new information from
the hypervisor team. Will send V1.

Thanks,
-Mukesh