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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mm: memory leak of successfully restored pages on error path in hv_call_deposit_pages()
- [Low] build: invocation of non-existent function set_direct_map_valid_noflush()
--

commit 702a398988acd774f1ea0597061a05c15e6ab245
Author: Magnus Kulke <magnuskulke@linux.microsoft.com>

drivers/hv: remove deposited pages from direct map

This commit removes deposited pages from the kernel direct map before passing
them to the hypervisor to prevent #GP faults on accidental access. It also
restores the direct map entries when the pages are returned.

> diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
> index 57b2c64197cb9..b056051a5ac59 100644
> --- a/drivers/hv/hv_proc.c
> +++ b/drivers/hv/hv_proc.c

[ ... ]

> +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);

[Severity: Low]
Does this code cause a build failure? The function
set_direct_map_valid_noflush() does not appear to exist in the kernel API.
The set_memory.h header provides set_direct_map_invalid_noflush() and
set_direct_map_default_noflush(), rather than a single multiplexed function
with a boolean flag.

> +		if (err && !ret)
> +			ret = err;
> +	}
> +
> +	if (valid)
> +		return ret;

[ ... ]

> @@ -90,12 +125,23 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
>  	if (!hv_result_success(status)) {
>  		hv_status_err(status, "\n");
>  		ret = hv_result_to_errno(status);
> -		goto err_free_allocations;
> +		goto err_restore_direct_map;
>  	}
>  
>  	ret = 0;
>  	goto free_buf;
>  
> +err_restore_direct_map:
> +	/*
> +	 * We don't want to return pages to the allocator if weren't able to
> +	 * mark them valid in the direct map.
> +	 */
> +	if (hv_deposit_update_direct_map(pages, counts, num_allocations, true)) {
> +		WARN(1, "leaking %d page block(s) that could not be set to valid\n",
> +		     num_allocations);
> +		goto free_buf;
> +	}

[Severity: Medium]
Does this error path leak successfully restored pages?

In hv_deposit_update_direct_map(), batch processing continues even if one
block fails, but the function returns the first error it encounters.

If a non-zero error is returned here, the goto skips the err_free_allocations
loop. This permanently leaks all pages in the batch, including the ones that
were successfully marked as valid in the direct map again.

> +
>  err_free_allocations:
>  	for (i = 0; i < num_allocations; ++i) {
>  		base_pfn = page_to_pfn(pages[i]);

[ ... ]

> @@ -110,6 +156,41 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
>  }
>  EXPORT_SYMBOL_GPL(hv_call_deposit_pages);
>  
> +/*
> + * Put withdrawn pages back in the direct map. Counterpart to the direct map
> + * removal done by hv_call_deposit_pages().
> + */
> +void hv_restore_withdrawn_pages(const u64 *pfns, int count)
> +{
> +	int i, j, end, n, ret;
> +	struct page *page;
> +
> +	for (i = 0; i < count; i = end) {
> +		page = pfn_to_page(pfns[i]);

[ ... ]

> +		for (j = i; j < end; ++j) {
> +			page = pfn_to_page(pfns[j]);
> +			/*
> +			 * Retry individually on failed batch. We don't want
> +			 * to return pages to the allocator that we couldn't mark
> +			 * valid in the direct map.
> +			 */
> +			if (ret &&
> +			    WARN_ON_ONCE(set_direct_map_valid_noflush(page, 1, true)))

[Severity: Low]
As mentioned above, set_direct_map_valid_noflush() does not exist in the
kernel API. Should this call set_direct_map_default_noflush() instead?

> +				continue;
> +			__free_page(page);
> +		}
> +	}
> +}

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