Re: [PATCH 12/12] mm/userfaultfd: check hugetlb end alignment in UFFDIO_UNREGISTER pre-scan

[Tal Zussman]

On 11/12/26 2:52 PM, Lorenzo Stoakes (ARM) wrote:
> 
> hugetlb vs userfaultfd is a bit like the alien vs. predator of worst mm features
> of all time (TM)
> 

Alien vs. Predator had a sequel... let's see what the mm version of
that will be :)

> On Wed, Nov 11, 2026 at 12:55:20PM -0500, Tal Zussman wrote:
>> UFFDIO_REGISTER and UFFDIO_UNREGISTER both scan the VMAs of the range
>> before changing anything. For a hugetlb VMA, userfaultfd_register()
>> rejects a range whose start or end is not huge page aligned there.
>> However, userfaultfd_unregister() only checks the start.
>>
>> An unaligned end only fails later, when vma_modify_flags_uffd() cannot
>> split the VMA there. At that point, userfaultfd_clear_vma() already
>> cleared the WP or RWP protection. The ioctl returns -EINVAL and the VMA
>> stays registered, but accesses to its pages are no longer reported. This
>> requires a caller passing a misaligned range.
>>
>> To fix it, check the end of a registered VMA in the pre-scan as well.
>> The ioctl still returns -EINVAL, but no longer unregisters the VMAs
>> before the misaligned one first.
>>
>> Fixes: cab350afcbc9 ("userfaultfd: hugetlbfs: allow registration of ranges containing huge pages")
>> Fixes: f369b07c8614 ("mm/uffd: reset write protection when unregister with wp-mode")
>> Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>
>> ---
>>  mm/userfaultfd.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
>> index bf42fabb5afd..c0076c8b4e9d 100644
>> --- a/mm/userfaultfd.c
>> +++ b/mm/userfaultfd.c
>> @@ -3983,6 +3983,18 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>>  		if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
>>  			goto out_unlock;
>>
>> +		/*
>> +		 * A range ending inside a registered hugetlb vma must end
>> +		 * huge page aligned, as in userfaultfd_register().
>> +		 */
> 
> Hmm I think this is more or less saying what the code is doing :) so I think
> unnecessary. Maybe worth using the words 'cursed' and 'eldrich' here to describe
> what's going on? Or possibly just e.g.:
> 
> 	/* See check in userfaultfd_register(). */
> 

Will cut it down.

>> +		if (cur->vm_userfaultfd_ctx.ctx && vma_is_hugetlb(cur) &&
> 
> This first check is pretty horrid to spell out loud, maybe do something like:
> 
> 	do {
> 		const struct userfaultfd_ctx *vma_ctx = cur->vm_userfaultfd_ctx.ctx;
> 		...
> 		if (vma_ctx && vma_is_hugetlb(cur) && end <= cur->vm_end &&
> 		    end > cur->vm_start) {
> 		    ...
> 		}
> 
>> +		    end <= cur->vm_end && end > cur->vm_start) {
>> +			unsigned long vma_hpagesize = vma_kernel_pagesize(cur);
> 
> Could be const?
> 
> Also is vma_kernel_pagesize() right? If the VMA is hugetlb, then that'll end up
> as hugetlb_vm_op_pagesize() which is huge_page_size(hstate_vma()), but it seems
> userfaultfd is full of this crap and yeah ok. Fine :)
> 
>> +
>> +			if (end & (vma_hpagesize - 1))
> 
> Could do:
> 			if (!IS_ALIGNED(end, vma_hpagesize))
> 
> Instead?
> 
> 

I just took this chunk straight from userfaultfd_register() and added the
ctx check, but I can clean this up while here. userfaultfd_register()
probably deserves a follow-up too.

>> +				goto out_unlock;
>> +		}
>> +
>>  		found = true;
>>  	} for_each_vma_range(vmi, cur, end);
>>  	VM_WARN_ON_ONCE(!found);
>>
>> --
>> 2.39.5
>>
> 
> --
> Cheers, Lorenzo
>