Re: [PATCH 1/4] mm: declare VMA flags by bit

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, Oct 29, 2025 at 04:02:28PM -0300, Jason Gunthorpe wrote:
> On Wed, Oct 29, 2025 at 05:49:35PM +0000, Lorenzo Stoakes wrote:
> > We declare a sparse-bitwise type vma_flag_t which ensures that users can't
> > pass around invalid VMA flags by accident and prepares for future work
> > towards VMA flags being a bitmap where we want to ensure bit values are
> > type safe.
>
> Does sparse attach the type to the enum item? Normal C says the enum
> item's type is always 'int' if the value fits in int..

It does, have tested this, not sure if due to sparse doing extra work to
make that happen or GNU C doing more there.

You can see an anon enum being used for this in the examples in the sparse
docs for instance (see [0]) so it's kind of a 'thing' it seems.

I also tested this to make sure, when intentionally passing some non-flag
value to the functions which accept vma_flag_t and it got picked up right
away, checked via:

make C=2 -j $(nproc) 2>&1 | grep vma_flag_t

[0]:https://docs.kernel.org/dev-tools/sparse.html

>
> And I'm not sure bitwise rules work quite the way you'd like for this
> enum, it was ment for things that are |'d..
>
> I have seen an agressively abuse-resistent technique before, I don't
> really recommend it, but FYI:
>
> struct vma_bits {
>   u8 VMA_READ_BIT;
>   u8 VMA_WRITE_BIT;
>   ..
> };
> #define VMA_BIT(bit_name) BIT(offsetof(struct vma_bits, bit_name))

Oh my eyes! :P I mean kinda clever but also lord above :)

I don't think we need this afaict. The idea is to catch accidental
instances of e.g.:

	vma_test(vma, VM_WRITE);

Rather than abuse. Doing the above is _very easy_ and so I wanted to
explicitly have the bots moan if people make this mistake.

If only C had a stronger type system...

>
> > Finally, we have to update some rather silly if-deffery found in
> > mm/task_mmu.c which would otherwise break.
> >
> > Additionally, update the VMA userland testing vma_internal.h header to
> > include these changes.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >  fs/proc/task_mmu.c               |   4 +-
> >  include/linux/mm.h               | 286 +++++++++++++++++---------
> >  tools/testing/vma/vma_internal.h | 341 +++++++++++++++++++++++++++----
>
> Maybe take the moment to put them in some vma_flags.h and then can
> that be included from tools/testing to avoid this copying??

It sucks to have this copy/paste yeah. The problem is to make the VMA
userland testing work, we intentionally isolate vma.h/vma.c dependencies
into vma_internal.h in mm/ and also do the same in the userland component,
so we can #include vma.c/h in the userland code.

So we'd have to have a strict requirement that vma_flags.h doesn't import
any other headers or at least none which aren't substituted somehow in the
tools/include directory.

The issue is people might quite reasonably update include/linux/vma_flags.h
to do more later and then break all of the VMA userland testing...

It's a bit of a delicate thing to keep it all

>
> > +/**
> > + * vma_flag_t - specifies an individual VMA flag by bit number.
> > + *
> > + * This value is made type safe by sparse to avoid passing invalid flag values
> > + * around.
> > + */
> > +typedef int __bitwise vma_flag_t;
> > +
> > +enum {
> > +	/* currently active flags */
> > +	VMA_READ_BIT = (__force vma_flag_t)0,
> > +	VMA_WRITE_BIT = (__force vma_flag_t)1,
> > +	VMA_EXEC_BIT = (__force vma_flag_t)2,
> > +	VMA_SHARED_BIT = (__force vma_flag_t)3,
> > +
> > +	/* mprotect() hardcodes VM_MAYREAD >> 4 == VM_READ, and so for r/w/x bits. */
> > +	VMA_MAYREAD_BIT = (__force vma_flag_t)4, /* limits for mprotect() etc */
> > +	VMA_MAYWRITE_BIT = (__force vma_flag_t)5,
> > +	VMA_MAYEXEC_BIT = (__force vma_flag_t)6,
> > +	VMA_MAYSHARE_BIT = (__force vma_flag_t)7,
> > +
> > +	VMA_GROWSDOWN_BIT = (__force vma_flag_t)8, /* general info on the segment */
> > +#ifdef CONFIG_MMU
> > +	VMA_UFFD_MISSING_BIT = (__force vma_flag_t)9, /* missing pages tracking */
> > +#else
> > +	/* nommu: R/O MAP_PRIVATE mapping that might overlay a file mapping */
> > +	VMA_MAYOVERLAY_BIT = (__force vma_flag_t)9,
> > +#endif
> > +	/* Page-ranges managed without "struct page", just pure PFN */
> > +	VMA_PFNMAP_BIT = (__force vma_flag_t)10,
> > +
> > +	VMA_MAYBE_GUARD_BIT = (__force vma_flag_t)11,
> > +
> > +	VMA_UFFD_WP_BIT = (__force vma_flag_t)12, /* wrprotect pages tracking */
> > +
> > +	VMA_LOCKED_BIT = (__force vma_flag_t)13,
> > +	VMA_IO_BIT = (__force vma_flag_t)14, /* Memory mapped I/O or similar */
> > +
> > +	/* Used by madvise() */
> > +	VMA_SEQ_READ_BIT = (__force vma_flag_t)15, /* App will access data sequentially */
> > +	VMA_RAND_READ_BIT = (__force vma_flag_t)16, /* App will not benefit from clustered reads */
> > +
> > +	VMA_DONTCOPY_BIT = (__force vma_flag_t)17, /* Do not copy this vma on fork */
> > +	VMA_DONTEXPAND_BIT = (__force vma_flag_t)18, /* Cannot expand with mremap() */
> > +	VMA_LOCKONFAULT_BIT = (__force vma_flag_t)19, /* Lock pages covered when faulted in */
> > +	VMA_ACCOUNT_BIT = (__force vma_flag_t)20, /* Is a VM accounted object */
> > +	VMA_NORESERVE_BIT = (__force vma_flag_t)21, /* should the VM suppress accounting */
> > +	VMA_HUGETLB_BIT = (__force vma_flag_t)22, /* Huge TLB Page VM */
> > +	VMA_SYNC_BIT = (__force vma_flag_t)23, /* Synchronous page faults */
> > +	VMA_ARCH_1_BIT = (__force vma_flag_t)24, /* Architecture-specific flag */
> > +	VMA_WIPEONFORK_BIT = (__force vma_flag_t)25, /* Wipe VMA contents in child. */
> > +	VMA_DONTDUMP_BIT = (__force vma_flag_t)26, /* Do not include in the core dump */
> > +
> > +#ifdef CONFIG_MEM_SOFT_DIRTY
> > +	VMA_SOFTDIRTY_BIT = (__force vma_flag_t)27, /* Not soft dirty clean area */
> > +#endif
> > +
> > +	VMA_MIXEDMAP_BIT = (__force vma_flag_t)28, /* Can contain struct page and pure PFN pages */
> > +	VMA_HUGEPAGE_BIT = (__force vma_flag_t)29, /* MADV_HUGEPAGE marked this vma */
> > +	VMA_NOHUGEPAGE_BIT = (__force vma_flag_t)30, /* MADV_NOHUGEPAGE marked this vma */
> > +	VMA_MERGEABLE_BIT = (__force vma_flag_t)31, /* KSM may merge identical pages */
> > +
> > +#ifdef CONFIG_64BIT
> > +	/* These bits are reused, we define specific uses below. */
> > +#ifdef CONFIG_ARCH_USES_HIGH_VMA_FLAGS
> > +	VMA_HIGH_ARCH_0_BIT = (__force vma_flag_t)32,
> > +	VMA_HIGH_ARCH_1_BIT = (__force vma_flag_t)33,
> > +	VMA_HIGH_ARCH_2_BIT = (__force vma_flag_t)34,
> > +	VMA_HIGH_ARCH_3_BIT = (__force vma_flag_t)35,
> > +	VMA_HIGH_ARCH_4_BIT = (__force vma_flag_t)36,
> > +	VMA_HIGH_ARCH_5_BIT = (__force vma_flag_t)37,
> > +	VMA_HIGH_ARCH_6_BIT = (__force vma_flag_t)38,
> > +#endif
> > +
> > +	VMA_ALLOW_ANY_UNCACHED_BIT = (__force vma_flag_t)39,
> > +	VMA_DROPPABLE_BIT = (__force vma_flag_t)40,
> > +
> > +#ifdef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR
> > +	VMA_UFFD_MINOR_BIT = (__force vma_flag_t)41,
> > +#endif
> > +
> > +	VMA_SEALED_BIT = (__force vma_flag_t)42,
> > +#endif /* CONFIG_64BIT */
> > +};
> > +
> > +#define VMA_BIT(bit)	BIT((__force int)bit)
>
> > -/* mprotect() hardcodes VM_MAYREAD >> 4 == VM_READ, and so for r/w/x bits. */
> > -#define VM_MAYREAD	0x00000010	/* limits for mprotect() etc */
> > -#define VM_MAYWRITE	0x00000020
> > -#define VM_MAYEXEC	0x00000040
> > -#define VM_MAYSHARE	0x00000080
> > +#define VM_MAYREAD	VMA_BIT(VMA_MAYREAD_BIT)
> > +#define VM_MAYWRITE	VMA_BIT(VMA_MAYWRITE_BIT)
> > +#define VM_MAYEXEC	VMA_BIT(VMA_MAYEXEC_BIT)
> > +#define VM_MAYSHARE	VMA_BIT(VMA_MAYSHARE_BIT)
>
> I suggest removing some of this duplication..
>
> #define DECLARE_VMA_BIT(name, bitno) \
>     NAME ## _BIT = (__force vma_flag_t)bitno,
>     NAME = BIT(bitno),
>
> enum {
>    DECLARE_VMA_BIT(VMA_READ, 0),
> }
>
> Especially since the #defines and enum need to have matching #ifdefs.
>
> It is OK to abuse the enum like the above, C won't get mad and works
> better in gdb/clangd.

I think having the enum anon avoids issues I've been concerned about with
named enum's containing flags when used as parameters yes.

>
> Later you can have a variation of the macro for your first sytem
> word/second system word idea.

Well I think we'd probably want to name the macro accordingly.

DECLARE_VMA_BIT_AND_FLAG() maybe? And mention in the comment that it's for
system word siz

>
> Otherwise I think this is a great thing to do, thanks!

Thanks :)

To give due credit - Matthew suggested this a while ago, I've been working
towards it with the mm flags first as an easier case to tackle.

It came out of my assuming that the VM_MAYBE_GUARD stuff didn't have a flag
free to do this in the 32-bit space. As part of this work it became
apparent I was wrong, so I implemented + sent that series yesterday (doh!)
but this change is still useful as it's beyond silly that we're constrained
like this.

I should actually probably put a Suggested-by for this, didn't even think
to, sorry Matthew! :)

>
> Jason

Cheers, Lorenzo