Re: [PATCH v3 5/9] mm/mprotect: add MM_CP_PERM_UPGRADE

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

On 11/13/26 9:37 PM, Lorenzo Stoakes (ARM) wrote:
> On Thu, Nov 12, 2026 at 03:20:34PM +0100, Carlos López wrote:
>> Add a change_protection() flag for changes that only add permissions,
>> so that MMU notifier users can keep their mappings and fault in the new
>> permissions later. mprotect() sets it when the new VMA flags only add
>> read, write or execute permissions, or VM_ACCOUNT, which does not affect
>> the page tables. Any other change, such as a different protection key,
>> is not an upgrade.
> 
> I'm struggling to comprehend what this means.
> 
> 'Add a flag for changes that only add 'permissions' (not defined) so that 'MMU
> notifier users' can keep mappings and 'fault' in 'the new permissions' later.
> 
> The rest of the message just states what the code does but speaks about it as if
> it was already what happened.
> 
> This needs to be completely rewritten...!

I'll rewrite it. By permissions I mean the access granted by the
protection bits in the page table entries.

> [...]
>> +/* Whether this change only upgrades/adds permissions */
>> +#define  MM_CP_PERM_UPGRADE                (1UL << 6)
> 
> What are 'permissions' exactly?

See above

> [...]
>> +static bool vma_flags_is_perm_upgrade(vma_flags_t old_vma_flags,
>> +				      vma_flags_t new_vma_flags)
> 
> No comment, random abbreviation (but just one word). Not ideal.

I'll rename. As David noted, the name I chose is not really correct.

>> +{
>> +	vm_flags_t old = vma_flags_to_legacy(old_vma_flags);
>> +	vm_flags_t new = vma_flags_to_legacy(new_vma_flags);
> 
> This is beyond appalling and a complete abuse of the VMA flags API.
> 
> You seem to have decided that you won't try to learn how the API works, so
> instead you'll just write deprecated code that I have to find time to come and
> fix up later I guess?

Sorry, I'll rewrite the helper with the vma_flags_t API, using
VMA_ACCESS_FLAGS and VMA_ACCOUNT_BIT, and document it.

> And we're at v3...!
> 
>> +	/* VM_ACCOUNT does not affect page tables */
> 
> What does this mean? And why are you referring to a deprecated constant?

I meant that setting VM_ACCOUNT does not affect the protection bits of
an entry, so it should not prevent the new flag from being set.
mprotect() sets VM_ACCOUNT when a private mapping becomes writable,
which is the case I wanted to cover. I'll explain this better.