Re: [PATCH 5/6] userfaultfd: decouple fault reason from VMA flags

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Tue, Aug 25, 2026 at 01:37:30PM +0300, Mike Rapoport wrote:
> On Mon, Aug 24, 2026 at 05:28:29PM +0100, Lorenzo Stoakes (ARM) wrote:
> > On Sun, Aug 23, 2026 at 03:17:42PM +0300, Mike Rapoport (Microsoft) wrote:
> > > Introduce enum uffd_reason to define reasons for user faults rather than
> > > overload VM_UFFD_* VMA flags for that.
> > >
> > > Using a dedicated enum makes the code clearer and decoupling the fault
> > > reason from VMA flags clears the way for moving the uffd mode bits out
> > > of VMA namespace.
> > >
> > > No functional change.
> > >
> > > Assisted-by: copilot:claude-opus-4.6
> > > Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> > > ---
> > >  include/linux/userfaultfd_k.h    | 16 ++++++++++++++--
> > >  include/uapi/linux/userfaultfd.h |  6 +++---
> > >  mm/huge_memory.c                 |  6 +++---
> > >  mm/hugetlb.c                     | 10 +++++-----
> > >  mm/memory.c                      | 10 +++++-----
> > >  mm/shmem.c                       |  4 ++--
> > >  mm/userfaultfd.c                 | 30 +++++++++++++++---------------
> > >  7 files changed, 47 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> > > index 45355bdb4ec7..f401623f315d 100644
> > > --- a/include/linux/userfaultfd_k.h
> > > +++ b/include/linux/userfaultfd_k.h
> > > @@ -9,6 +9,18 @@
> > >  #ifndef _LINUX_USERFAULTFD_K_H
> > >  #define _LINUX_USERFAULTFD_K_H
> > >
> > > +#include <linux/bits.h>
> > > +
> > > +/* Fault reason #PF handler passes to handle_userfault() */
> > > +enum uf_reason {
> > > +	USERFAULT_MISSING	= BIT(0),
> > > +	USERFAULT_MINOR		= BIT(1),
> > > +	USERFAULT_RWP		= BIT(2),
> > > +	USERFAULT_WP		= BIT(3),
> > > +};
> >
> > Hmm your commit message says uffd_reason, uf_reason makes me think of the
> > character Ulf from House of the Dragon. But not uffd. So as per David let's
> > rename it :)
> >
> > I'm also not sure if an enum is the right thing for flag values?
>
> I'll ask LLM why it chose it :)

I mean you then introduce the same flags again seemingly with different
names as #define's in the next patch... having several sets of flags with
subtly different names seems unwise.

It seems there is a 'mode' and a 'reason', maybe I missed something but
that just seems overly complicated.


But in general I think the objection to using an enum for flags is because
the compiler will treat a switch() {} of the enums as the only independent
values and possibly in other places too.

That's super theoretical but it is an odd thing to do potentially. I got
(annoying) push-back on using an enum for flags on a series in the past, so
I guess here I am paying it forwards ;)

>
> > Anything that is parameterised by enum uffd_reason that combines flags will
> > break any switch statement in there and yada yada.
> >
> > I wonder if better just as #define's + unsigned long or something?
> >
> > Or you could do (and this leads to nicer stuff later):
> >
> > 	enum uffd_reason {
> > 	     USERFAULT_MISSING_BIT = 0,
> > 	     USERFAULT_MINOR_BIT = 1,
> > 	     USERFAULT_RWP_BIT = 2,
> > 	     USERFAULT_WP_BIT = 3,
> > 	};
> >
> > 	#define USERFAULT_MISSING BIT(USERFAULT_MISSING_BIT)
> > etc.
>
> Looks over-engineered to me tbh, if we drop an enum, I'd just
>
> #define FLAG (1 << SHIFT)
>
> and call it a day.
>
> Also see below about aligning with uABI flags.

See review on 6/6, I'm confused actually why we have several sets of these
flags...

But in general it seems like these flags (in one form or another) are being
repeatedly referenced, so it's not really over-engineering I don't think to
abstract some of that.

Maybe can be in wrappers that make it nicer. But really the issue is the
duplication in modes/reasons/flags...

>
> > > @@ -168,9 +168,9 @@ struct uffd_msg {
> > >
> > >  /* flags for UFFD_EVENT_PAGEFAULT */
> > >  #define UFFD_PAGEFAULT_FLAG_WRITE	(1<<0)	/* If this was a write fault */
> > > -#define UFFD_PAGEFAULT_FLAG_WP		(1<<1)	/* If reason is VM_UFFD_WP */
> > > -#define UFFD_PAGEFAULT_FLAG_MINOR	(1<<2)	/* If reason is VM_UFFD_MINOR */
> > > -#define UFFD_PAGEFAULT_FLAG_RWP		(1<<3)	/* If reason is VM_UFFD_RWP */
> > > +#define UFFD_PAGEFAULT_FLAG_WP		(1<<1)	/* If reason is uffd-wp */
> > > +#define UFFD_PAGEFAULT_FLAG_MINOR	(1<<2)	/* If reason is uffd-minor */
> > > +#define UFFD_PAGEFAULT_FLAG_RWP		(1<<3)	/* If reason is uffd-rwp */
> >
> > Is it worth retaining the same bit indexes as the reasons?
> >
> > Reasons:
> >
> > 	Bit number
> > MINOR	0
> > RWP	1
> > WP	2
> >
> > Page fault flags:
> >
> > 	Bit number
> > MINOR	2
> > RWP	3
> > WP	1
>
> If we go this way, than it must be
>
> #define USERFAULT_MINOR UFFD_PAGEFAULT_FLAG_MINOR
>
> so we won't need to keep them in sync explicitly.
>
> With a caveat of USERFAULT_MISSING that is expressed as "no flags in
> uffd_msg" :)

Ugh.

>
> > > @@ -2607,7 +2607,7 @@ static inline void msg_init(struct uffd_msg *msg)
> > >  static inline struct uffd_msg userfault_msg(unsigned long address,
> > >  					    unsigned long real_address,
> > >  					    unsigned int flags,
> > > -					    unsigned long reason,
> > > +					    enum uf_reason reason,
> > >  					    unsigned int features)
> > >  {
> > >  	struct uffd_msg msg;
> > > @@ -2629,11 +2629,11 @@ static inline struct uffd_msg userfault_msg(unsigned long address,
> > >  	 */
> > >  	if (flags & FAULT_FLAG_WRITE)
> > >  		msg.arg.pagefault.flags |= UFFD_PAGEFAULT_FLAG_WRITE;
> > > -	if (reason & VM_UFFD_WP)
> > > +	if (reason & USERFAULT_WP)
> > >  		msg.arg.pagefault.flags |= UFFD_PAGEFAULT_FLAG_WP;
> > > -	if (reason & VM_UFFD_RWP)
> > > +	if (reason & USERFAULT_RWP)
> > >  		msg.arg.pagefault.flags |= UFFD_PAGEFAULT_FLAG_RWP;
> > > -	if (reason & VM_UFFD_MINOR)
> > > +	if (reason & USERFAULT_MINOR)
> > >  		msg.arg.pagefault.flags |= UFFD_PAGEFAULT_FLAG_MINOR;
> >
> > With matching flags and unsigned long you could do
> >
> > 	msg.arg.pagefault.flags |= reason;
> >
> > I think?
>
> Almost:
>
> 	msg.arg.pagefault.flags |= (reason & ~USERFAULT_MISSING);
>
> And define USERFAULT_MISSING as (1 << 0) with a comment why it's fine.
>
> I don't feel strongly about it, but my preference is to define reason flags
> independently of UFFD_PAGEFAULT_FLAGs and keep the ifs here.

And also modes... Again I think fixing that mess somehow is the better way forward.

>
> > > @@ -2793,14 +2793,14 @@ static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx,
> > >  	 * If VMA has UFFD WP faults enabled and WP fault, wait for userspace to
> > >  	 * resolve the fault.
> > >  	 */
> > > -	if (!pte_write(ptent) && (reason & VM_UFFD_WP))
> > > +	if (!pte_write(ptent) && (reason & USERFAULT_WP))
> >
> > I wonder if you could actually
> >
> > You do this quite a lot and they read a bit horribly with the && and & on the
> > same sight-line. With the changes to the enum proposed above you could do:
> >
> > 	if (!pte_write(ptent) && test_bit(reason, USERFAULT_WP_BIT))
>
> I find && and & perfectly readable and adding _BIT defines looks really
> excessive to me.

Discussed in sub-thread. We'll agree to disagree I suppose.

>
> > > @@ -2835,7 +2835,7 @@ static inline unsigned int userfaultfd_get_blocking_state(unsigned int flags)
> > >   * fatal_signal_pending()s, and the mmap_lock must be released before
> > >   * returning it.
> > >   */
> > > -vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason)
> > > +vm_fault_t handle_userfault(struct vm_fault *vmf, enum uf_reason reason)
> >
> > Hmm what was the 'reason' here before? The flags? Maybe more reason (no pun
> > intended) to keep the values the same?
>
> The 'reason' before was a VM_UFFD_SOMETHING, we really can't keep the
> values the same, but we surely can keep it unsigned long.

I notice the 'mode' which is not the same as the 'reason' is an unsigned
int in 6/6...

>
> > --
> > Cheers, Lorenzo
>
> --
> Sincerely yours,
> Mike.

--
Cheers, Lorenzo