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

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

On 11/12/26 15:20, 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.
> 
> 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)
>  
>  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)

Two tab indent.

> +{
> +	vm_flags_t old = vma_flags_to_legacy(old_vma_flags);
> +	vm_flags_t new = vma_flags_to_legacy(new_vma_flags);
> +	/* VM_ACCOUNT does not affect page tables */
> +	vm_flags_t mask = VM_READ | VM_WRITE | VM_EXEC | VM_ACCOUNT;

Why is VM_ACCOUNT a permission upgrade? At best it's "no change".

What am I missing?

Also, what you really care about is "no permission downgrade", and "no
permission upgrade" is not the inverse of that ;)


-- 
Cheers,

David