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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The error path for a failed second hypercall erroneously frees pages already deposited to the hypervisor and double-frees `lastpfn`.
- [High] If the deposit hypercall fails with `U64_MAX`, the error handler skips freeing pages, leaking up to 511 pages per call.
--

commit 03b27f8f1637e85a6319eb9bbc84a63e9b9df5c5
Author: Mukesh R <mrathor@linux.microsoft.com>

mshv: Redesign hypervisor memory deposit logic

This commit redesigns the hypervisor memory deposit logic to improve
performance and fix contiguous range requirements. It changes the logic to
attempt allocating a 2MB contiguous range by default and reduces the
allocations per deposit.

> diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
> index 57864bb5bcd8c..0ebede0bc8b4f 100644
> --- a/drivers/hv/hv_proc.c
> +++ b/drivers/hv/hv_proc.c

[ ... ]

> +static int hv_call_deposit_pages(int node, u64 partition_id, bool contiguous)
> +{
> +	struct hv_deposit_memory *hc_input;
> +	int i, rc, num_pages;
> +	u64 status, *pfna, lastpfn = 0;
> +	bool trunc_extra = false;

[ ... ]

> +	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)) {
> +			if (contiguous)
> +				goto err_free_dep_pages;
> +
> +			/* We deposited lot earlier, so give it a go */
> +			__free_page(pfn_to_page(lastpfn));
> +		}
> +	}
> +
> +	free_page((unsigned long)hc_input);
> +	return 0;
> +
> +err_free_dep_pages:
> +	hv_status_err(status, "\n");
> +	rc = hv_result_to_errno(status);
> +
> +	for (i = hv_repcomp(status); i < num_pages; i++)
> +		__free_page(pfn_to_page(pfna[i]));

[Severity: High]
If hv_do_rep_hypercall() fails and returns U64_MAX, doesn't hv_repcomp(status)
evaluate to 4095?

Since 4095 is always greater than num_pages (which is capped at 511), this loop
condition would evaluate to false immediately, bypassing the cleanup and
causing an unconditional leak of the allocated pages.

[Severity: Critical]
If the first hypercall succeeds but the second hypercall fails while contiguous
is true, the code jumps to err_free_dep_pages.

At this point, num_pages is still from the first hypercall, but status is from
the failed second hypercall, so hv_repcomp(status) is likely 0. Does this mean
the loop will iterate from 0 to 511, mistakenly returning pages to the buddy
allocator that the hypervisor successfully assumed ownership of during the
first hypercall?

Furthermore, because hc_input->gpa_page_list[0] was replaced with lastpfn just
before the second hypercall, the loop will free lastpfn at index 0.

Could the subsequent block:

> +	if (lastpfn)
> +		__free_page(pfn_to_page(lastpfn));

then free lastpfn a second time, resulting in a double-free?

> +
> +out_free:
> +	free_page((unsigned long)hc_input);
> +	return rc;
> +}

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