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

On Tue, Aug 25, 2026 at 02:00:10PM +0100, Lorenzo Stoakes (ARM) wrote:

On Tue, Aug 25, 2026 at 01:37:30PM +0300, Mike Rapoport wrote:

quoted

On Mon, Aug 24, 2026 at 05:28:29PM +0100, Lorenzo Stoakes (ARM) wrote:

quoted

On Sun, Aug 23, 2026 at 03:17:42PM +0300, Mike Rapoport (Microsoft) wrote:

quoted

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.

The names are important, the values are not. 

There are two cases that currently use the same VMA_UFFD_* flags:
* the way VMA is registered with uffd, i.e. the 'mode' part
* the type of the user fault that the generic #PF handler passes to
  handle_userfault()

They are related, a fault in a VMA that was registered as MISSING will
never pass MINOR to handle_userfault(), but I think it'll be actually
clearer to separate them semantically, so that when you read a call site of
handle_userfault() it is clear what type of the fault it is and when you
parse userfaultfd code you see what modes user wanted for a VMA.
 

quoted

quoted

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...

I can see that ;-)
 

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.

Again, the bit numbers do not matter, they are the same because it's easy
to count from 0. I can make one of those count backwards if it helps :)
 

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

quoted

quoted

quoted

@@ -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.

Yeah, and the PAGEFAULT_FLAG numbers are set in stone because it's uABI.
 

quoted

quoted

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.

Can you elaborate?

quoted

quoted

quoted

@@ -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.

 
Yes, we will :)

quoted

quoted

quoted

@@ -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...

Didn't you suggest to make 'reason' an unsigned int as well?

'mode' in 6/6 is an unsigned int because if it were an enum it'd require
#include <linux/userfaultfd_k.h> in mm_types.h, see the commit message
there.

--
Cheers, Lorenzo

-- 
Sincerely yours,
Mike.