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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

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...!

>
> Signed-off-by: Carlos López <clopez@suse.de>
> ---
>  include/linux/mm.h |  2 ++
>  mm/mprotect.c      | 13 +++++++++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 6af3b7008f1a..001829a61405 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3585,6 +3585,8 @@ int get_cmdline(struct task_struct *task, char *buffer, int buflen);
>  #define  MM_CP_UFFD_RWP_RESOLVE            (1UL << 5) /* resolve rwp */
>  #define  MM_CP_UFFD_RWP_ALL                (MM_CP_UFFD_RWP | \
>  					    MM_CP_UFFD_RWP_RESOLVE)
> +/* Whether this change only upgrades/adds permissions */
> +#define  MM_CP_PERM_UPGRADE                (1UL << 6)

What are 'permissions' exactly?

>
>  bool can_change_pte_writable(struct vm_area_struct *vma, unsigned long addr,
>  			     pte_t pte);
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 84bcfe0281d3..8c0fcd10f5a3 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -767,6 +767,17 @@ static const struct mm_walk_ops prot_none_walk_ops = {
>  	.walk_lock		= PGWALK_WRLOCK,
>  };
>
> +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.

> +{
> +	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?

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?

> +	vm_flags_t mask = VM_READ | VM_WRITE | VM_EXEC | VM_ACCOUNT;
> +
> +	return new == (old | (new & mask));

Ordinarily I'd try to help you use the new API, but the very fact you'd send
it like this dissuades me :/

> +}
> +
>  int
>  mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
>  	       struct vm_area_struct *vma, struct vm_area_struct **pprev,
> @@ -847,6 +858,8 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
>  	vma_overwrite_flags(vma, new_vma_flags);
>  	if (vma_wants_manual_pte_write_upgrade(vma))
>  		mm_cp_flags |= MM_CP_TRY_CHANGE_WRITABLE;
> +	if (vma_flags_is_perm_upgrade(old_vma_flags, new_vma_flags))
> +		mm_cp_flags |= MM_CP_PERM_UPGRADE;
>  	vma_set_page_prot(vma);
>
>  	change_protection(tlb, vma, start, end, mm_cp_flags);
> --
> 2.51.0
>

--
Cheers, Lorenzo