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

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) <[email protected]>
> > ---
> >  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 :)
 
> 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.
 
> > @@ -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" :)
 
> > @@ -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.
 
> > @@ -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.
 
> > @@ -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.
 
> --
> Cheers, Lorenzo

-- 
Sincerely yours,
Mike.