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

[sashiko-bot@kernel.org]

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