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

[Mukesh R <mrathor@linux.microsoft.com>]

On 9/11/26 17:15, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Memory leak of all un-deposited pages when `hv_do_rep_hypercall` returns `U64_MAX`.
> --
> 
> commit 68b286f4de61fdc1a888242cfc4b1a1ea3d187dc
> Author: Mukesh R <mrathor@linux.microsoft.com>
> 
> mshv: Redesign hypervisor memory deposit logic
> 
> This commit redesigns the hypervisor memory deposit logic to optimize
> performance and address preexisting issues. It defaults to allocating and
> depositing 2M chunks when possible to reduce overhead and avoids unnecessary
> interrupt disabling during the process.
> 
>> diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
>> index 57864bb5bcd8c..df39a5c587ca2 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)
>> +{
> [ ... ]
>> +	/* Not using hyperv_pcpu_input_arg, so no need to disable interrupts */
>> +
>> +	status = hv_do_rep_hypercall(HVCALL_DEPOSIT_MEMORY, num_pages, 0,
>> +				     hc_input, NULL);
>> +	if (!hv_result_success(status))
>> +		goto err_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) && hv_repcomp(status) == 0)
> 
> [Severity: Medium]
> Will this leak the page pointed to by lastpfn if hv_do_rep_hypercall() returns
> U64_MAX?
> 
> If the hypervisor is unavailable or hypercalls are unsupported,
> hv_do_rep_hypercall() can return the sentinel value U64_MAX, which is explicitly
> handled by hv_result_to_errno() elsewhere.
> 
> When status is U64_MAX, the hv_repcomp(status) macro extracts bits 43:32, which
> results in 4095 instead of 0.
> 
> Does this cause the condition to evaluate to false and skip freeing the page?

This hypercall will not return U64_MAX.


>> +			/* 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++)
> 
> [Severity: Medium]
> Will this loop fail to free any pages in the pfna[] array when status is
> U64_MAX?
> 
> Similar to the check above, if status is U64_MAX, hv_repcomp(status) evaluates
> to 4095.
> 
> Since num_pages is bounded to a maximum of 511 (HV_DEPOSIT_INP_MAX) in this
> path, the loop initialization sets i = 4095, making the condition i < num_pages
> immediately false.
> 
> Does this cause the loop to terminate without executing, silently leaking up to
> 511 allocated pages per failed deposit attempt?


status cannot be U64_MAX for this hypercall.


>> +		__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;
>> +}
>