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

On Fri, Mar 20, 2026 at 10:57:32AM +0100, Vlastimil Babka (SUSE) wrote:
> 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.

Ack will fix up.

>
> > 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 <[email protected]>	[SELinux]
> > Signed-off-by: Lorenzo Stoakes (Oracle) <[email protected]>
>
> Acked-by: Vlastimil Babka (SUSE) <[email protected]>

Thanks!

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

I mean there's a precedent for this, but the compiler _should_ figure out this
as a constant value, I have repeatedly confirmed that it's good at that in
godbolt, via make foo/bar.S etc.

So it should have no measureable impact at 64-bit VMA flags anyway, but I can
give it a go and see before/after.

>
> > +#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()?

Could do! It should be a pretty clear indicator this is 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.

Yeah that's a mistake, will fixup.

>
> >   */
> >  #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" ?

Hmm, I'm not sure if I can express this more clearly, it's a pain either
way. Could do:

	if (vma_flags_test(&new_vma_flags, VMA_WRITE_BIT) &&
	    !vma_flags_test_any(&old_vma_flags, VMA_WRITE_BIT, VMA_SHARED_BIT))
		populate_vma_page_range(vma, start, end, NULL);

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

I may do both as a follow up patch given the series is a pain to rework at this point
and I want at least the first version to be like-for-like intentionally.

>
> > +}
> >
> >  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

Thanks, Lorenzo