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

Vlastimil Babka (SUSE)
2026-03-20T09:57:52Z

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