Re: [PATCH v2 5/9] mm/mprotect: keep writable entries writable on permission upgrades

["Carlos López" <clopez@suse.de>]

On 11/5/26 6:06 PM, David Hildenbrand (Arm) wrote:
> On 11/5/26 17:50, Carlos López wrote:
>> change_protection() applies vma->vm_page_prot, which lacks the write bit
>> for CoW and write-notified mappings, so it write-protects every entry
>> in them. When an entry would simply be made writable on fault without
>> further checks (e.g. dirty PTEs in shared mappings, exclusive anonymous
>> pages), MM_CP_TRY_CHANGE_WRITABLE makes the entry writable again to
>> avoid the future fault.
>>
>> However, writable entries in shared mappings that are not dirty still
>> become read-only. For example, a uffd-wp unprotect makes clean shmem
>> PTEs read-only even if they were never write-protected, e.g. when they
>> were mapped by a read fault before uffd-wp was registered. The next
>> write to each of them causes a fault that only restores the write bit.
>>
>> Now that MM_CP_PERM_UPGRADE indicates whether mprotect() only adds
>> permissions, change_protection() can tell, together with the
>> userfaultfd unprotect flags, when a change revokes no access. Keep
>> writable entries writable in these cases, at the PTE, PMD and PUD
>> levels. This is consistent with the write fault handler, as an entry
>> that is already writable needs no write fault.
>>
>> Signed-off-by: Carlos López <clopez@suse.de>
>> ---
>>  mm/huge_memory.c | 7 +++++++
>>  mm/internal.h    | 7 +++++++
>>  mm/mprotect.c    | 4 ++++
>>  3 files changed, 18 insertions(+)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 506e1e61c0fd..6e23d0e4ae66 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -2809,6 +2809,10 @@ int change_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>>  	if (userfaultfd_rwp(vma) && pmd_uffd(entry))
>>  		entry = pmd_modify(entry, PAGE_NONE);
>>  
>> +	/* See change_present_ptes(). */
>> +	if (mm_cp_is_perm_upgrade(cp_flags) && pmd_write(oldpmd))
>> +		entry = pmd_mkwrite(entry, vma);
>> +
>>  	/* See change_pte_range(). */
>>  	if ((cp_flags & MM_CP_TRY_CHANGE_WRITABLE) && !pmd_write(entry) &&
>>  	    can_change_pmd_writable(vma, addr, entry))
>> @@ -2885,6 +2889,9 @@ int change_huge_pud(struct mmu_gather *tlb, struct vm_area_struct *vma,
>>  	 */
>>  	oldpud = pudp_invalidate(vma, addr, pudp);
>>  	entry = pud_modify(oldpud, newprot);
>> +	/* See change_present_ptes(). */
>> +	if (mm_cp_is_perm_upgrade(cp_flags) && pud_write(oldpud))
>> +		entry = pud_mkwrite(entry);
> 
> That looks completely hacked on. We do have can_change_pud_writable() below to
> make such decisions.

Okay, for v3 I can pass the old entry to can_change_{pmd,pud}_writable()
and return true if it was writable, after the existing write-tracking
checks (uffd-wp/soft-dirty). For PTEs, can_change_pte_writable() is not
used on the change_protection() path, so the same check would go in
set_write_prot_commit_flush_ptes().

Does that sound more reasonable?

> Whaatever you propose here is either wrong or way too shaky, sorry.

The idea is that MM_CP_TRY_CHANGE_WRITABLE relies on the dirty bit to
make entries in shared mappings writable, which misses clean entries
that were already writable (e.g. shmem read-faulted before uffd-wp
registration). Such entries can stay writable, as anything that relies
on write faults write-protects the entry first.