Re: [PATCH v3 17/23] mm: convert do_brk_flags() to use vma_flags_t

["Vlastimil Babka (SUSE)" <vbabka@kernel.org>]

On 3/18/26 16:50, Lorenzo Stoakes (Oracle) wrote:
> In order to be able to do this, we need to change VM_DATA_DEFAULT_FLAGS
> and friends and update the architecture-specific definitions also.
> 
> We then have to update some KSM logic to handle VMA flags, and introduce
> VMA_STACK_FLAGS to define the vma_flags_t equivalent of VM_STACK_FLAGS.
> 
> We also introduce two helper functions for use during the time we are
> converting legacy flags to vma_flags_t values - vma_flags_to_legacy() and
> legacy_to_vma_flags().

Nit: this was done by an earlier patch.

> This enables us to iteratively make changes to break these changes up into
> separate parts.
> 
> We use these explicitly here to keep VM_STACK_FLAGS around for certain
> users which need to maintain the legacy vm_flags_t values for the time
> being.
> 
> We are no longer able to rely on the simple VM_xxx being set to zero if
> the feature is not enabled, so in the case of VM_DROPPABLE we introduce
> VMA_DROPPABLE as the vma_flags_t equivalent, which is set to
> EMPTY_VMA_FLAGS if the droppable flag is not available.
> 
> While we're here, we make the description of do_brk_flags() into a kdoc
> comment, as it almost was already.
> 
> We use vma_flags_to_legacy() to not need to update the vm_get_page_prot()
> logic as this time.
> 
> Note that in create_init_stack_vma() we have to replace the BUILD_BUG_ON()
> with a VM_WARN_ON_ONCE() as the tested values are no longer build time
> available.
> 
> We also update mprotect_fixup() to use VMA flags where possible, though we
> have to live with a little duplication between vm_flags_t and vma_flags_t
> values for the time being until further conversions are made.
> 
> Finally, we update the VMA tests to reflect these changes.
> 
> Acked-by: Paul Moore <paul@paul-moore.com>	[SELinux]
> Signed-off-by: Lorenzo Stoakes (Oracle) <ljs@kernel.org>

Acked-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>

More nits below:

> diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h
> index b39cc1127e1f..e25d0d18f6d7 100644
> --- a/arch/arm64/include/asm/page.h
> +++ b/arch/arm64/include/asm/page.h
> @@ -46,7 +46,12 @@ int pfn_is_map_memory(unsigned long pfn);
>  
>  #endif /* !__ASSEMBLER__ */
>  
> -#define VM_DATA_DEFAULT_FLAGS	(VM_DATA_FLAGS_TSK_EXEC | VM_MTE_ALLOWED)
> +#ifdef CONFIG_ARM64_MTE
> +#define VMA_DATA_DEFAULT_FLAGS	append_vma_flags(VMA_DATA_FLAGS_TSK_EXEC, \
> +						 VMA_MTE_ALLOWED_BIT)

I wonder what's the bloat-o-meter impact of these #define's (this
arm64-specific one isn't the only one) being no longer compile-time-constants?

> +#else
> +#define VMA_DATA_DEFAULT_FLAGS	VMA_DATA_FLAGS_TSK_EXEC
> +#endif
>  
>  #include <asm-generic/getorder.h>
>  

> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h

>  
> +#define VMA_STACK_FLAGS	append_vma_flags(VMA_STACK_DEFAULT_FLAGS,	\
> +		VMA_STACK_BIT, VMA_ACCOUNT_BIT)
> +
> +/* Temporary until VMA flags conversion complete. */
> +#define VM_STACK_FLAGS vma_flags_to_legacy(VMA_STACK_FLAGS)
> +
>  #define VM_STARTGAP_FLAGS (VM_GROWSDOWN | VM_SHADOW_STACK)
>  
>  #ifdef CONFIG_MSEAL_SYSTEM_MAPPINGS
> @@ -536,8 +547,6 @@ enum {
>  #define VM_SEALED_SYSMAP	VM_NONE
>  #endif
>  
> -#define VM_STACK_FLAGS	(VM_STACK | VM_STACK_DEFAULT_FLAGS | VM_ACCOUNT)
> -
>  /* VMA basic access permission flags */
>  #define VM_ACCESS_FLAGS (VM_READ | VM_WRITE | VM_EXEC)
>  #define VMA_ACCESS_FLAGS mk_vma_flags(VMA_READ_BIT, VMA_WRITE_BIT, VMA_EXEC_BIT)
> @@ -547,6 +556,9 @@ enum {
>   */
>  #define VM_SPECIAL (VM_IO | VM_DONTEXPAND | VM_PFNMAP | VM_MIXEDMAP)
>  
> +#define VMA_SPECIAL_FLAGS mk_vma_flags(VMA_IO_BIT, VMA_DONTEXPAND_BIT, \
> +				       VMA_PFNMAP_BIT, VMA_MIXEDMAP_BIT)

Should VM_SPECIAL be also redefined using vma_flags_to_legacy()?

> +
>  /*
>   * Physically remapped pages are special. Tell the
>   * rest of the world about it:
> @@ -1412,7 +1424,7 @@ static __always_inline void vma_desc_set_flags_mask(struct vm_area_desc *desc,
>   * vm_area_desc object describing a proposed VMA, e.g.:
>   *
>   * vma_desc_set_flags(desc, VMA_IO_BIT, VMA_PFNMAP_BIT, VMA_DONTEXPAND_BIT,
> - *		VMA_DONTDUMP_BIT);
> + *              VMA_DONTDUMP_BIT);

Looks like spurious tabs-to-space change inconsistent with other instances.

>   */
>  #define vma_desc_set_flags(desc, ...) \
>  	vma_desc_set_flags_mask(desc, mk_vma_flags(__VA_ARGS__))
> @@ -4059,7 +4071,6 @@ extern int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file);
>  extern struct file *get_mm_exe_file(struct mm_struct *mm);
>  extern struct file *get_task_exe_file(struct task_struct *task);
>  
> -extern bool may_expand_vm(struct mm_struct *, vm_flags_t, unsigned long npages);
>  extern void vm_stat_account(struct mm_struct *, vm_flags_t, long npages);
>  
>  extern bool vma_is_special_mapping(const struct vm_area_struct *vma,
> diff --git a/mm/internal.h b/mm/internal.h
> index f98f4746ac41..80d8651441a7 100644

> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 9681f055b9fc..eaa724b99908 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c

> @@ -773,19 +778,24 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
>  
>  	change_protection(tlb, vma, start, end, mm_cp_flags);
>  
> -	if ((oldflags & VM_ACCOUNT) && !(newflags & VM_ACCOUNT))
> +	if (vma_flags_test(&old_vma_flags, VMA_ACCOUNT_BIT) &&
> +	    !vma_flags_test(&new_vma_flags, VMA_ACCOUNT_BIT))
>  		vm_unacct_memory(nrpages);
>  
>  	/*
>  	 * Private VM_LOCKED VMA becoming writable: trigger COW to avoid major
>  	 * fault on access.
>  	 */
> -	if ((oldflags & (VM_WRITE | VM_SHARED | VM_LOCKED)) == VM_LOCKED &&
> -			(newflags & VM_WRITE)) {
> -		populate_vma_page_range(vma, start, end, NULL);
> +	if (vma_flags_test(&new_vma_flags, VMA_WRITE_BIT)) {
> +		const vma_flags_t mask =
> +			vma_flags_and(&old_vma_flags, VMA_WRITE_BIT,
> +				      VMA_SHARED_BIT, VMA_LOCKED_BIT);
> +
> +		if (vma_flags_same(&mask, VMA_LOCKED_BIT))

That converts the original logic 1:1, but I wonder if it's now feasible to
write it more obviously as "VMA_LOCKED_BIT must be set, VM_WRITE_BIT and
VM_SHARED_BIT must not" ?

> +			populate_vma_page_range(vma, start, end, NULL);
>  	}
>  
> -	vm_stat_account(mm, oldflags, -nrpages);
> +	vm_stat_account(mm, vma_flags_to_legacy(old_vma_flags), -nrpages);
>  	vm_stat_account(mm, newflags, nrpages);
>  	perf_event_mmap(vma);
>  	return 0;

> diff --git a/mm/vma.h b/mm/vma.h
> index cf8926558bf6..1f2de6cb3b97 100644
> --- a/mm/vma.h
> +++ b/mm/vma.h

> +static inline bool is_data_mapping_vma_flags(const vma_flags_t *vma_flags)
> +{
> +	const vma_flags_t mask = vma_flags_and(vma_flags,
> +			VMA_WRITE_BIT, VMA_SHARED_BIT, VMA_STACK_BIT);
> +
> +	return vma_flags_same(&mask, VMA_WRITE_BIT);

Ditto?

> +}
>  
>  static inline void vma_iter_config(struct vma_iterator *vmi,
>  		unsigned long index, unsigned long last)
> diff --git a/mm/vma_exec.c b/mm/vma_exec.c
> index 8134e1afca68..5cee8b7efa0f 100644