Re: [PATCH v3 03/10] mm/vmalloc: clamp an unaligned range in vunmap_pte_range()

[Dev Jain <dev.jain@arm.com>]

On 02/11/26 11:37 am, Hao Ge wrote:
> 
> 
> On 2026/10/30 22:20, Dev Jain wrote:
>>
>>
>> On 30/10/26 2:00 pm, Hao Ge wrote:
>>> Hi Dev
>>>
>>> Thanks for your review.
>>>
>>> On 2026/10/30 15:14, Dev Jain wrote:
>>>>
>>>>
>>>> On 26/10/26 1:33 pm, Hao Ge wrote:
>>>>> commit a739e6b557af ("mm: vmalloc: WARN_ON if mapping size is not
>>>>> PAGE_SIZE aligned") made vmap_pte_range() warn and bail out when the
>>>>> size is not a multiple of PAGE_SIZE. That loop steps in page
>>>>
>>>> Why are we checking the alignment so late into vmap_pte_range? Why not
>>>> simply do
>>>>
>>>> if (!IS_ALIGNED(addr, PAGE_SIZE) || !IS_ALIGNED(end, PAGE_SIZE))
>>>> 	return; (or return -EINVAL);
>>>>
>>>> at the start of vmap_range_noflush and vunmap_range_noflush?
>>>
>>> I think adding that check at the entry of vunmap_range_noflush()
>>> goes against its design intent. Its job is to tear down the mappings
>>> of the given VA range, so even when the caller passes a size that is
>>> not page-aligned, we still have to unmap whatever is mapped within
>>> that range.
>>>
>>> So the patch clamps end down to the last full page and keeps
>>> clearing. A buggy caller still gets its mapping torn down as far as
>>> it goes, and nothing is left behind. The check here is just for
>>> protection. 
>>>
>>> This is just my preliminary understanding; I may be mistaken.
>>
>> Hmm I get you, but this is very ugly. We are trying to tolerate
>> a blatant bug in the caller with not-so-nice code. But see below.
>>>
>>> Thanks
>>> Best Regards
>>> Hao
>>>
>>>
>>> But that's just my understanding, happy to be corrected.
>>>
>>>
>>>>
>>>>
>>>>> multiples and stops only at addr == end; with an unaligned size addr
>>>>> never lands on end, so the loop cannot terminate and walks off the
>>>>> end of the page table.
>>>>>
>>>>> vunmap_pte_range() has the same loop without any guard, so an
>>>>> unaligned range would clear PTEs past end without ever stopping,
>>>>> walking off the end of the page table.
>>>>>
>>>>> Add the check there too. The range is clamped down to the last full
>>>>> page rather than dropped, and the loop is skipped when nothing is
>>>>> left, so a buggy caller still gets its PTEs cleared as far as they
>>>>> go.
>>>>>
>>>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>>>>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
>>>>> ---
>>>>>  mm/vmalloc.c | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>>>>> index 44e0eb32708f..7131f60d1a8e 100644
>>>>> --- a/mm/vmalloc.c
>>>>> +++ b/mm/vmalloc.c
>>>>> @@ -393,6 +393,11 @@ static void vunmap_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
>>>>>  	pte_t ptent;
>>>>>  	unsigned long size = PAGE_SIZE;
>>>>>  
>>>>> +	if (WARN_ON_ONCE(!PAGE_ALIGNED(end - addr)))
>>>>> +		end = addr + PAGE_ALIGN_DOWN(end - addr);
>>
>> For simplicity assume PAGE_SIZE is 4, start is 3, end is 10.
>> With your code we will get
>>
>> addr = 3, end = 3 + ALIGN_DOWN(10 - 3) = 3 + 4 = 7.
>> So the range becomes [3, 7) clearing only two ptes, the one covering
>> [0, 3], the second covering [4, 7]. I feel we should rather make
>> the range from [3, 10] to [0, 11], clearing three ptes.
>>
> 
> Thanks for the example. Looks like I nearly introduced a new bug while
> guarding against the old one, good thing you caught it.
> 
>> So, at the start of vunmap_range_noflush we could basically
>> WARN_ON on non-alignment, and then clamp the start down, and clamp
>> the end up to PAGE_SIZE.
> 
> I'm wondering whether we should simply BUG_ON for non-aligned ranges here.
> Because the unmap could silently clear mappings at adjacent addresses, which
> may not even belong to the caller, and this kind of problem is really hard to
> track down afterwards. Say [0, 3) is in use by someone else, with PAGE_SIZE=4
> an unmap starting at 3 still begins with the pte covering [0, 4), since the loop
> clears page by page, so that owner loses his mapping without doing anything wrong,
> and only sees the damage much later when he touches it again.
> 
> WDYT?
> 
>>
>> And with this also throw in a patch to bring the -EINVAL bailout
>> in vmap_pte_range to the top, in vmap_range_noflush and check
>> both start and end alignment.
> 
> For the vmap_range_noflush() check, fully agreed.  The current check
> is indeed too late, the range is only found to be bad after part of
> it has already been mapped. 
> 
> Looking forward to your valuable comments.

I was on vacation, I'll review the next version soon.


> 
> Thanks
> Best Regards
> Hao
>>
>>>>> +	if (end == addr)
>>>>> +		return;
>>>>> +
>>>>>  	pte = pte_offset_kernel(pmd, addr);
>>>>>  	lazy_mmu_mode_enable();
>>>>>  
>>>>
>>