On Tue, Aug 25, 2026 at 01:44:23PM +0100, Lorenzo Stoakes (ARM) wrote:
> I don't love referring to the legacy flags in the subject but I gues you
> have limited space...
>
> On Sun, Aug 23, 2026 at 03:17:43PM +0300, Mike Rapoport (Microsoft) wrote:
> > Add 'mode' field to struct vm_uffd_state and define UFFD_MODE_ flags.
>
> Can you mention that you're increasing the size of the VMA by 4 bytes
> please? (8 bytes if __HAVE_PFNMAP_TRACKING I believe too).
With CONFIG_PER_VMA_LOCK I'm decreasing the headroom by 4 bytes, I'll add a
few sentences in the changelog.
> > Use this field to differentiate VMA registration with userfaultfd
> > instead of relying on VM_UFFD_* flags.
>
> Here you should reference non-legacy VMA flag names.
Ok.
> > A VMA registered with userfaultfd will have a single VM_UFFD flag set
> > and its registration mode (MISSING, MINOR, WP, RWP) is determined by
> > vm_uffd_state.mode.
> >
> > This frees three vm_flags bits (12, 41, 43).
>
> Is the primary motivation here to eliminate these flags? We're paying a
> cost in VMA bloat here so I think you need to argue for it. I wouldn't say
> freeing up VMA flags justifies adding 4 or 8 bytes per VMA.
The motivation is to first disambiguate fault reason and VMA registration
mode and second create a per-VMA state for uffd for future use.
AFAIR Sean mentioned during guest_memfd discussions that a few status bits
would have been useful there.
> We've put a lot of effort into reducing VMA size so I think any size
> increase in standard shipped 64-bit kernels has to be justified.
Standard shipped kernels have CONFIG_PER_VMA_LOCK=y which makes VMAs padded
to the next cacheline so adding a field there only decreases padding.
I can also move vm_uffd_state after pfnmap_track_ctx to keep it in the end
so there won't be 4 bytes hole.
If/when we run out of space, we can allocate uffd state separately, but
it's more involved so I don't think it's necessary at this point.
> Also there's weirdness around the flag behaviour with WP. As I recall
> there's strange situations where you have to examine state of the
> destination VMA when doing a UFFDIO_MOVE or something like that and there's
> just strange edge cases.
>
> I'm guessing the change is just independent of this and in both cases
> you're checking for state just in different please?
The check is explicit in vma_needs_copy().
> > Update the relevant code to use UFFD_MODE_* instead of VM_UFFD_* flags.
>
> USERFAULT_, UF_, UFFD_... Can we settle on one?
As I replied to David, 'USERFAULT' means the type of the fault.
For modes, or flags, UFFD_ is the "subsystem" namespace.
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 4daf9cd6ae8e..416de7663951 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -303,7 +303,7 @@ enum {
> > DECLARE_VMA_BIT(MAYSHARE, 7),
> > DECLARE_VMA_BIT(GROWSDOWN, 8), /* general info on the segment */
> > #ifdef CONFIG_MMU
> > - DECLARE_VMA_BIT(UFFD_MISSING, 9),/* missing pages tracking */
> > + DECLARE_VMA_BIT(UFFD, 9), /* userfaultfd registered */
>
> Since we're getting rid of other flags, can we just have UFFD occupy a flag
> that isn't conditional on CONFIG_MMU? Maybe bit 12 instead?
Sure, can do.
> Presumably nommu will never set/use VMA_UFFD_BIT (CONFIG_USERFAULTFD won't
> be set) and it'll make everything easier this way.
>
> > -#define VM_COPY_ON_FORK (VM_PFNMAP | VM_MIXEDMAP | VM_UFFD_WP |
> > VM_UFFD_RWP | \
> > - VM_MAYBE_GUARD)
> > +#define VM_COPY_ON_FORK (VM_PFNMAP | VM_MIXEDMAP | VM_MAYBE_GUARD)
>
> Really this should be converted to the new VMA flags model, but I guess
> it's outside of the scope of this change.
It is, yes.
> >
> > /*
> > * mapping from the currently active vm_flags protection bits (the
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index d6deb655d82e..8354d1c18b29 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -723,6 +723,7 @@ struct vm_region {
> > #define NULL_VM_UFFD_STATE ((struct vm_uffd_state) { NULL, })
> > struct vm_uffd_state {
> > struct userfaultfd_ctx *ctx;
> > + unsigned int mode;
> > };
>
> Hmm this is adding 4 bytes at least to every VMA is that OK?
See above.
> I was going to say this adds a cache line but no it shouldn't as it's right
> at the end.
>
> VMA size scaling is a real issue though and this increases every VMA by 4
> bytes, can't it be put in userfaultfd_ctx? I guess not as it's a per-VMA
> thing.
It can't be in userfaultfd_ctx. It's essentially the backpointer to the
file descriptor context.
> And does all of the NULL stuff now actually still work?
Yes.
> > +/* Per-VMA uffd modes */
> > +#define UFFD_MODE_MISSING BIT(0)
> > +#define UFFD_MODE_MINOR BIT(1)
> > +#define UFFD_MODE_RWP BIT(2)
> > +#define UFFD_MODE_WP BIT(3)
> > +#define UFFD_MODE_ALL (UFFD_MODE_MISSING | UFFD_MODE_MINOR | \
> > + UFFD_MODE_RWP | UFFD_MODE_WP)
>
> An entirely new set of duplicative flags?
I'll make these BIT(31) - BIT(28) ;-)
> And now we're flitting from USERFAULT_ to UFFD_ for some reason...
See above.
> Mode also seems to me to imply a specific setting not a set of flags.
>
> So you probably want to put the word 'flag' in there somewhere... Or say
> 'mode_s_'. Since multiple can be set right?
I'll see how to improve the naming.
> And weird/inconsistent to declare the USERFAULT_xxx as an enum and #define's
> here as well as the naming?
Since the field lives in mm_types.h it'd require "include userfaultfd_k.h"
to make this an enum.
> > static inline bool is_mergeable_vm_uffd_state(struct vm_area_struct *vma,
> > struct vm_uffd_state vm_ctx)
> > {
> > - return vma->vm_uffd_state.ctx == vm_ctx.ctx;
> > + return vma->vm_uffd_state.ctx == vm_ctx.ctx &&
> > + uffd_mode(vma) == vm_ctx.mode;
>
> You see it's things like this that make the naming problematic, now it
> seems that mode (whose very name implies a singular state) is being checked
> against another which can either be in one mode or another but actually
> you're doing a flags check...
I see your point, will rename to some *flags*.
> > }
> >
> > static inline bool userfaultfd_missing(const struct vm_area_struct *vma)
> > {
> > - return vma_test_any_mask(vma, VMA_UFFD_MISSING);
> > + return vma_test(vma, VMA_UFFD_BIT) &&
> > + (uffd_mode(vma) & UFFD_MODE_MISSING);
>
> This is broken assuming this can be executed in a context where VMA_UFFD can
> be
> VMA_NONE.
No, it isn't. This is under #ifdef CONFIG_USERFAULTFD that depends on
CONFIG_MMU so VMA_UFFD is defined.
> You should use vma_test_single_mask(). Or preferably, as above, just always
> provide VMA_UFFD_BIT.
It's an interesting API engineering, where the most obvious API does not
always work and one needs to verify the bit definitions to understand what
exact API variant to use.
> The problem is with bits we can't express a VM_NONE equivalent, which is
> why VMA_UFFD is defined.
>
> It seems the only places that's used are ones where you could, or already
> do, gate on uffd being enabled:
>
> include/linux/mm.h: * vma_flags_t flags =
> mk_vma_flags_from_masks(VMA_UFFD_WP, VMA_UFFD_MINOR);
>
> (This is a comment that needs updating see my comment at the end of review).
See my comment there.
> mm/userfaultfd.c: vma_flags_clear_mask(&new_vma_flags, VMA_UFFD);
> mm/userfaultfd.c: vma_flags_set_mask(&new_vma_flags, VMA_UFFD);
>
> > }
> >
> > static inline bool userfaultfd_wp(const struct vm_area_struct *vma)
> > {
> > - return vma_test_any_mask(vma, VMA_UFFD_WP);
> > + return vma_test(vma, VMA_UFFD_BIT) &&
> > + (uffd_mode(vma) & UFFD_MODE_WP);
>
> Same comment as above this seems broken.
See above.
> > }
> >
> > static inline bool userfaultfd_minor(const struct vm_area_struct *vma)
> > {
> > - return vma_test_any_mask(vma, VMA_UFFD_MINOR);
> > + return vma_test(vma, VMA_UFFD_BIT) &&
> > + (uffd_mode(vma) & UFFD_MODE_MINOR);
>
> Same comment as above this seems broken.
See above.
> > }
> >
> > static inline bool userfaultfd_rwp(const struct vm_area_struct *vma)
> > @@ -203,7 +213,8 @@ static inline bool userfaultfd_rwp(const struct
> > vm_area_struct *vma)
> > */
> > if (!IS_ENABLED(CONFIG_ARCH_HAS_PTE_PROTNONE))
> > return false;
> > - return vma_test_single_mask(vma, VMA_UFFD_RWP);
> > + return vma_test(vma, VMA_UFFD_BIT) &&
> > + (uffd_mode(vma) & UFFD_MODE_RWP);
>
> Same comment as above this seems broken.
See above.
> Obviously I don't love the &&, & but that seems a moot point.
We agreed to disagree, didn't we? :)
> > }
> >
> > static inline bool userfaultfd_protected(const struct vm_area_struct *vma)
> > @@ -271,7 +282,7 @@ static inline bool userfaultfd_huge_pmd_rwp(struct
> > vm_area_struct *vma,
> >
> > static inline bool userfaultfd_armed(struct vm_area_struct *vma)
> > {
> > - return vma_test_any_mask(vma, __VMA_UFFD_FLAGS);
> > + return vma_test(vma, VMA_UFFD_BIT);
>
> Same comment as above.
See above.
> > }
> >
> > -IF_HAVE_UFFD_RWP(VM_UFFD_RWP, "uffd_rwp" )
> > \
> > {VM_LOCKED, "locked" }, \
> > {VM_IO, "io" }, \
> > {VM_SEQ_READ, "seqread" }, \
>
> Shouldn't you update the tracing logic to obtain these from uffd
> modes/flags?
Yep, will do.
> > /*
> > * If WP is the only mode enabled and context is wp async, allow any
> > * memory type.
> > */
> > - if (wp_async && (vm_flags == VM_UFFD_WP))
> > + if (wp_async && (mode == UFFD_MODE_WP))
>
> Yeah again this is so so confusing and the naming really doesn't help.
>
> I wonder if helpers similar to the vma flag helpers could come in handly.
>
> I know you claim that kind of thing is overengineering but you're
> open-coding checks all over the place, then doing a subtle variation like
> this which is really really easy to miss.
>
> Something like userfault_test() would be nice.
I don't see how it'll be clearer. How
userfault_test_single_mask(mode, UFFD_MODE_WP_BIT)
is better than a plain == comparison?
> At any rate 'modes' or 'flags' or something would be clearer here.
I'll rethink the naming.
> > new_vma_flags = vma->flags;
> > - vma_flags_clear_mask(&new_vma_flags, __VMA_UFFD_FLAGS);
> > - vma_flags_set_mask(&new_vma_flags, vma_flags);
> > + vma_flags_set_mask(&new_vma_flags, VMA_UFFD);
>
> This is oddly arbitrarily using VMA_UFFD inconsistent from all uses of
> VMA_UFFD_BIT.
Can't say I follow you here.
> > @@ -3814,11 +3817,11 @@ static int userfaultfd_register(struct
> > userfaultfd_ctx *ctx,
> > cond_resched();
> >
> > VM_WARN_ON_ONCE(!!cur->vm_uffd_state.ctx ^
> > - !!(cur->vm_flags & __VM_UFFD_FLAGS));
> > + vma_test(cur, VMA_UFFD_BIT));
>
> Same comments as elsewhere re vma_test() on VMA_UFFD_BIT.
See above.
> > @@ -3857,7 +3860,8 @@ static int userfaultfd_register(struct
> > userfaultfd_ctx *ctx,
> > if (end & (vma_hpagesize - 1))
> > goto out_unlock;
> > }
> > - if ((vm_flags & VM_UFFD_WP) && !(cur->vm_flags & VM_MAYWRITE))
> > + if ((mode & UFFD_MODE_WP) &&
> > + !vma_test(cur, VMA_MAYWRITE_BIT))
>
> Really weird indentation and I think on one line it's 80 chars anyway?
Indeed.
> Thanks for switching to new VMA flags model though!
Welcome :)
> > goto out_unlock;
> >
> > /*
> > @@ -3872,13 +3876,13 @@ static int userfaultfd_register(struct
> > userfaultfd_ctx *ctx,
> > goto out_unlock;
> >
> > /*
> > - * Mode switches that drop VM_UFFD_WP or VM_UFFD_RWP would
> > - * leave PTE markers without the flag that describes them;
> > + * Mode switches that drop WP or RWP would leave PTE markers
> > + * without the mode that describes them;
> > * subsequent mprotect() would then promote stale markers
> > * into the other mode. Require an unregister first.
> > */
> > if (cur->vm_uffd_state.ctx == ctx &&
> > - cur->vm_flags & (VM_UFFD_WP | VM_UFFD_RWP) & ~vm_flags)
> > + uffd_mode(cur) & (UFFD_MODE_WP | UFFD_MODE_RWP) & ~mode)
>
> I mean this is just horrible beyond words aesthetically (and was before
> tbf). But you've already rejected this kind of feedback so I guess, yeah I
> object. Using bits or wrappers would make this potentially nicer.
Very much doubt it.
> Same objection to the use of the word 'mode'. You really need to say flags
> here somehow.
>
> > goto out_unlock;
> >
> > /*
> > @@ -3891,7 +3895,7 @@ static int userfaultfd_register(struct
> > userfaultfd_ctx *ctx,
> > } for_each_vma_range(vmi, cur, end);
> > VM_WARN_ON_ONCE(!found);
> >
> > - ret = userfaultfd_register_range(ctx, vma, vm_flags, start, end,
> > + ret = userfaultfd_register_range(ctx, vma, mode, start, end,
> > wp_async);
> >
> > out_unlock:
> > @@ -3986,7 +3990,7 @@ static int userfaultfd_unregister(struct
> > userfaultfd_ctx *ctx,
> > cond_resched();
> >
> > VM_WARN_ON_ONCE(!!cur->vm_uffd_state.ctx ^
> > - !!(cur->vm_flags & __VM_UFFD_FLAGS));
> > + vma_test(cur, VMA_UFFD_BIT));
>
> Again you should use vma_test_single_mask(). I'm not sure why you dropped
> one for the other unless provably all of these paths are CONFIG_MMU.
This one as well inside #ifdef CONFIG_USERFAULTFD
> But it'd make life a lot easier to just use a bit number that isn't
> predicated on CONFIG_MMU.
It would have been easier if vma_test() could deal with that ;-P
> >
> > /*
> > * Prevent unregistering through a different userfaultfd than
> > @@ -4003,7 +4007,7 @@ static int userfaultfd_unregister(struct
> > userfaultfd_ctx *ctx,
> > * provides for more strict behavior to notice
> > * unregistration errors.
> > */
> > - if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
> > + if (!vma_can_userfault(cur, uffd_mode(cur), wp_async))
> > goto out_unlock;
> >
> > found = true;
> > @@ -4024,7 +4028,8 @@ static int userfaultfd_unregister(struct
> > userfaultfd_ctx *ctx,
> > goto skip;
> >
> > VM_WARN_ON_ONCE(vma->vm_uffd_state.ctx != ctx);
> > - VM_WARN_ON_ONCE(!vma_can_userfault(vma, vma->vm_flags,
> > wp_async));
> > + VM_WARN_ON_ONCE(!vma_can_userfault(vma, uffd_mode(vma),
> > + wp_async));
>
> Nit but pretty horrible alignment. Gues it can 't be helped
Would run out of 80 chars :(
But I'm thinking now to add one more patch to get rid from passing wp_async
to vma_can_userfault().
> Also, in the mk_vma_flags_from_masks() macro, there's a comment that
> explicitly references VMA_UFFD_MINOR:
>
> /*
> * Combine pre-computed vma_flags_t masks into one value, e.g.:
> *
> * vma_flags_t flags = mk_vma_flags_from_masks(VMA_UFFD_WP, VMA_UFFD_MINOR);
> *
> * Unlike mk_vma_flags(), which takes bit numbers, this takes whole masks --
> * each of which may be EMPTY_VMA_FLAGS when its feature is unavailable -- so
> a
> * bit that does not exist on the current build is never materialised.
> */
> #define mk_vma_flags_from_masks(...) \
>
> You should change that...
That was used only by uffd, do you want to keep the macro still?
> Could even be with placeholder flag names potentially.
Like VMA_FLAG_A, VMA_FLAG_B?
> On the engineering of this - this is one quite big, fiddly patch, if you
> abstracted some of the tests into another you could do the change and the
> abstraction separately.
>
> Overall I like what you're doing _in general_ but we have to:
>
> a. Figure out whether we want to pay the memory price for this (and the
> case has to be made in the commit message.
I'll update the commit message.
> b. Fix the VMA_UFFD_BIT stuff ideally with a bit that's just always set not
> predicated on CONFIG_MMU.
Yeah, I'll make it bit 12. Really curious why that one wasn't #ifdefed on
something.
> c. Improve the engineering so this stuff actually makes the code clearer
> rather than just reimplementing the same old confusing uffd mess.
I don't agree that replacing plain bit operations with long multiword
predicates makes the code clearer.
Userfault is a complex beast and using, say,
userfault_test_mode_bit_set(mode, UFFD_MODE_MISSING_BIT) instead of
mode & UFFD_MODE_MISSING won't make it any less complex.
> d. Fix the naming... modes, flags, uffd, userfault, uf, etc. let's stick
> with one and be consistent.
flags could work, yes.
uffd and userfault have different semantic meaning in the context of this
series, userfault is a type of the page fault forwarded to user space, uffd
is the namespace of userfault subsystem.
> IMO before it can move forwards.
>
> --
> Cheers, Lorenzo
--
Sincerely yours,
Mike.
