Re: [PATCH] Drivers: hv: mshv_vtl: Fix vmemmap_shift exceeding MAX_FOLIO_ORDER

[Naman Jain <namjain@linux.microsoft.com>]

On 4/3/2026 9:05 AM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Tuesday, March 31, 2026 10:40 PM
>>
>> When registering VTL0 memory via MSHV_ADD_VTL0_MEMORY, the kernel
>> computes pgmap->vmemmap_shift as the number of trailing zeros in the
>> OR of start_pfn and last_pfn, intending to use the largest compound
>> page order both endpoints are aligned to.
>>
>> However, this value is not clamped to MAX_FOLIO_ORDER, so a
>> sufficiently aligned range (e.g. physical range 0x800000000000-
>> 0x800080000000, corresponding to start_pfn=0x800000000 with 35
>> trailing zeros) can produce a shift larger than what
>> memremap_pages() accepts, triggering a WARN and returning -EINVAL:
>>
>>    WARNING: ... memremap_pages+0x512/0x650
>>    requested folio size unsupported
>>
>> The MAX_FOLIO_ORDER check was added by
>> commit 646b67d57589 ("mm/memremap: reject unreasonable folio/compound
>> page sizes in memremap_pages()").
>> When CONFIG_HAVE_GIGANTIC_FOLIOS=y, CONFIG_SPARSEMEM_VMEMMAP=y, and
>> CONFIG_HUGETLB_PAGE is not set, MAX_FOLIO_ORDER resolves to
>> (PUD_SHIFT - PAGE_SHIFT) = 18. Any range whose PFN alignment exceeds
>> order 18 hits this path.
> 
> I'm not clear on what point you are making with this specific
> configuration that results in MAX_FOLIO_ORDER being 18. Is it just
> an example? Is 18 the largest expected value for MAX_FOLIO_ORDER?
> And note that PUD_SHIFT and PAGE_SHIFT might have different values
> on arm64 with a page size other than 4K.
> 

Yes, this was just an example. It is not generalized enough, I will 
remove it.
MAX_FOLIO_ORDER could go beyond 18.

>>
>> Fix this by clamping vmemmap_shift to MAX_FOLIO_ORDER so we always
>> request the largest order the kernel supports, rather than an
>> out-of-range value.
>>
>> Also fix the error path to propagate the actual error code from
>> devm_memremap_pages() instead of hard-coding -EFAULT, which was
>> masking the real -EINVAL return.
>>
>> Fixes: 7bfe3b8ea6e3 ("Drivers: hv: Introduce mshv_vtl driver")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
>> ---
>>   drivers/hv/mshv_vtl_main.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hv/mshv_vtl_main.c b/drivers/hv/mshv_vtl_main.c
>> index 5856975f32e12..255fed3a740c1 100644
>> --- a/drivers/hv/mshv_vtl_main.c
>> +++ b/drivers/hv/mshv_vtl_main.c
>> @@ -405,8 +405,12 @@ static int mshv_vtl_ioctl_add_vtl0_mem(struct mshv_vtl *vtl, void __user *arg)
>>   	/*
>>   	 * Determine the highest page order that can be used for the given memory range.
>>   	 * This works best when the range is aligned; i.e. both the start and the length.
>> +	 * Clamp to MAX_FOLIO_ORDER to avoid a WARN in memremap_pages() when the range
>> +	 * alignment exceeds the maximum supported folio order for this kernel config.
>>   	 */
>> -	pgmap->vmemmap_shift = count_trailing_zeros(vtl0_mem.start_pfn | vtl0_mem.last_pfn);
>> +	pgmap->vmemmap_shift = min_t(unsigned long,
>> +				     count_trailing_zeros(vtl0_mem.start_pfn | vtl0_mem.last_pfn),
>> +				     MAX_FOLIO_ORDER);
> 
> Is it necessary to use min_t() here, or would min() work?  Neither count_trailing_zeros()
> nor MAX_FOLIO_ORDER is ever negative, so it seems like just min() would work with
> no potential for doing a bogus comparison or assignment.
>

min could work, yes. I just felt min_t is more safer for comparing these 
two different types of values -
count_trailing_zeroes being 'int'
MAX_FOLIO_ORDER being a macro, calculated by applying bit operations.

and destination being unsigned int.


include/linux/mmzone.h:#define MAX_FOLIO_ORDER          MAX_PAGE_ORDER
include/linux/mmzone.h:#define MAX_FOLIO_ORDER          PFN_SECTION_SHIFT
include/linux/mmzone.h:#define MAX_FOLIO_ORDER          (ilog2(SZ_16G) - 
PAGE_SHIFT)
include/linux/mmzone.h:#define MAX_FOLIO_ORDER          (ilog2(SZ_1G) - 
PAGE_SHIFT)
include/linux/mmzone.h:#define MAX_FOLIO_ORDER          (PUD_SHIFT - 
PAGE_SHIFT)

I am fine with anything you suggest here.

> The shift is calculated using the originally passed in start_pfn and last_pfn, while the
> "range" struct in pgmap has an "end" value that is one page less. So is the idea to
> go ahead and create the mapping with folios of a size that includes that last page,
> and then just waste the last page of the last folio?

No, waste does not occur. The vmemmap_shift determines the folio order, 
and memmap_init_zone_device() walks the range [start_pfn, last_pfn) in 
steps of (1 << vmemmap_shift) pages, creating one folio per step. The OR 
operation ensures both boundaries are aligned to multiples of (1 << 
vmemmap_shift), guaranteeing the range divides evenly into folios with 
no partial folio at the end.
The intention is to find the highest folio order possible here, and if 
it reaches the MAX_FOLIO_ORDER, restrict vmemmap_shift to it.

> 
>>   	dev_dbg(vtl->module_dev,
>>   		"Add VTL0 memory: start: 0x%llx, end_pfn: 0x%llx, page order: %lu\n",
>>   		vtl0_mem.start_pfn, vtl0_mem.last_pfn, pgmap->vmemmap_shift);
>> @@ -415,7 +419,7 @@ static int mshv_vtl_ioctl_add_vtl0_mem(struct mshv_vtl *vtl, void __user *arg)
>>   	if (IS_ERR(addr)) {
>>   		dev_err(vtl->module_dev, "devm_memremap_pages error: %ld\n", PTR_ERR(addr));
>>   		kfree(pgmap);
>> -		return -EFAULT;
>> +		return PTR_ERR(addr);
>>   	}
>>
>>   	/* Don't free pgmap, since it has to stick around until the memory
>>
>> base-commit: 36ece9697e89016181e5ae87510e40fb31d86f2b
>> --
>> 2.43.0
>>

Thanks for reviewing.

Regards,
Naman