Re: [PATCH] mm/mprotect: don't skip uffd marker install when racing with PTE table reclaim

["David Hildenbrand (Arm)" <david@kernel.org>]

On 11/8/26 09:40, Lance Yang wrote:
> 
> On Sat, Nov 07, 2026 at 12:28:55AM +0100, David Hildenbrand (Arm) wrote:
>> change_pmd_range() populates PTE tables before installing uffd-wp
>> markers. An empty PTE table can be removed after change_pmd_prepare()
>> returns but before the pmd_none() check.
>>
>> That check then skips the range, allowing UFFDIO_WRITEPROTECT to report
>> success without installing the required markers. Subsequent writes by
>> userspace to these areas will not trigger uffd events.
>>
>> PTE table reclaim during MADV_DONTNEED and page table retraction during
>> khugepaged can trigger this.
>>
>> Retry page-table allocation in that case. Simplify the code, getting
>> rid of change_pmd_prepare() by routing all code through the "again" label.
> 
> Cool, nice work1
> 
>> It's a small race though, so likely this is not very urgent.
>>
>> Fixes: fe2567eb5521 ("mm/shmem: allow uffd wr-protect none pte for file-backed mem")
>> Fixes: 6375e95f381e ("mm: pgtable: reclaim empty PTE page in madvise(MADV_DONTNEED)")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
>> ---
>> I expect Andrew to pick this up.
>> ---
>> mm/mprotect.c | 48 +++++++++++-------------------------------------
>> 1 file changed, 11 insertions(+), 37 deletions(-)
>>
>> diff --git a/mm/mprotect.c b/mm/mprotect.c
>> index e18d4e12fc341..ef4e7188e24da 100644
>> --- a/mm/mprotect.c
>> +++ b/mm/mprotect.c
>> @@ -470,25 +470,8 @@ pgtable_populate_needed(struct vm_area_struct *vma, unsigned long cp_flags)
>> }
>>
>> /*
>> - * Populate the pgtable underneath for whatever reason if requested.
>> - * When {pte|pmd|...}_alloc() failed we treat it the same way as pgtable
>> - * allocation failures during page faults by kicking OOM and returning
>> - * error.
>> - */
>> -#define  change_pmd_prepare(vma, pmd, cp_flags)				\
>> -	({								\
>> -		long err = 0;						\
>> -		if (unlikely(pgtable_populate_needed(vma, cp_flags))) {	\
>> -			if (pte_alloc(vma->vm_mm, pmd))			\
>> -				err = -ENOMEM;				\
>> -		}							\
>> -		err;							\
>> -	})
>> -
>> -/*
>> - * This is the general pud/p4d/pgd version of change_pmd_prepare(). We need to
>> - * have separate change_pmd_prepare() because pte_alloc() returns 0 on success,
>> - * while {pmd|pud|p4d}_alloc() returns the valid pointer on success.
>> + * Populate the lower-level page table if marker installation requires it.
>> + * Return -ENOMEM on allocation failure.
>>  */
>> #define  change_prepare(vma, high, low, addr, cp_flags)			\
>> 	  ({								\
>> @@ -517,30 +500,21 @@ static inline long change_pmd_range(struct mmu_gather *tlb,
>> again:
>> 		next = pmd_addr_end(addr, end);
>>
>> -		ret = change_pmd_prepare(vma, pmd, cp_flags);
>> -		if (ret) {
>> -			pages = ret;
>> -			break;
>> -		}
>> -
>> -		if (pmd_none(*pmd))
>> +		if (unlikely(pgtable_populate_needed(vma, cp_flags))) {
>> +			if (pte_alloc(vma->vm_mm, pmd)) {
>> +				pages = -ENOMEM;
>> +				break;
>> +			}
>> +		} else if (pmd_none(*pmd)) {
>> 			goto next;
>> +		}
> 
> Previously, reclaim could clear the PMD after allocation and we'd just
> skip the range without installing the markers ...
> 
> Now it's change_pte_range() -> -EAGAIN -> goto again (retry the same range)
> 
> And yeah, that should do the trick ;)
> 
>>
>> 		_pmd = pmdp_get_lockless(pmd);
>> 		if (pmd_is_huge(_pmd)) {
>> 			if ((next - addr != HPAGE_PMD_SIZE) ||
>> 			    pgtable_split_needed(vma, cp_flags)) {
>> 				__split_huge_pmd(vma, pmd, addr, false);
>> -				/*
>> -				 * For file-backed, the pmd could have been
>> -				 * cleared; make sure pmd populated if
>> -				 * necessary, then fall-through to pte level.
>> -				 */
>> -				ret = change_pmd_prepare(vma, pmd, cp_flags);
>> -				if (ret) {
>> -					pages = ret;
>> -					break;
>> -				}
>> +				goto again;
> 
> And, after the split, we just go back to again -> allocate a PTE table
> if needed -> recheck the PMD.
> 
> So, no need for that second change_pmd_prepare() ANYMORE :D
> 
> LGTM!
> 
> Reviewed-by: Lance Yang <lance.yang@linux.dev>
> 
> Cheers, Lance

Thanks for the review! Let's merge this through Andrew's tree :)

-- 
Cheers,

David