gup_can_follow_protnone() is defined in include/linux/mm.h but only used
by mm/gup.c.
First, there is no reason to have it in already gigantic header.
Next, the upcoming refactoring of userfaultfd flags will make
gup_can_follow_protnone() depend on userfaultfd_k.h which would cause a
cyclic header dependency.
Move gup_can_follow_protnone() to mm/gup.c.
No functional change.
Assisted-by: copilot:claude-opus-5
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
include/linux/mm.h | 38 --------------------------------------
mm/gup.c | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 38 insertions(+), 38 deletions(-)
userfaultfd_{missing,wp,minor,rwp}() and userfaultfd_protected() only
read the VMA.
Make their vma parameter const.
No functional change.
Assisted-by: copilot:claude-opus-5
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
include/linux/userfaultfd_k.h | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
Move userfaultfd_{missing,wp,minor,rwp}() and userfaultfd_protected()
ahead of uffd_disable_huge_pmd_share() and uffd_disable_fault_around()
and make the latter two use the helpers rather than open coded VMA flag
masks.
Convert open coded VMA flag test in mfill_get_vma() to userfaultfd_wp()
as well.
With every user of the per-VMA uffd modes going through the helpers,
their underlying representation can be changed in the next step.
No functional change.
Assisted-by: copilot:claude-opus-5
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
include/linux/userfaultfd_k.h | 70 +++++++++++++++++++++----------------------
mm/userfaultfd.c | 2 +-
2 files changed, 35 insertions(+), 37 deletions(-)
Rename struct vm_userfaultfd_ctx to vm_uffd_state to better reflect that
it will represent the userfaultfd state for a VMA rather than just a
context pointer.
This is a preparatory step for extending the struct with a mode field.
Mechanical rename, no functional change.
Assisted-by: copilot:claude-opus-4.6
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
Documentation/mm/process_addrs.rst | 4 +--
include/linux/mm_types.h | 10 +++---
include/linux/userfaultfd_k.h | 24 ++++++-------
mm/mremap.c | 4 +--
mm/userfaultfd.c | 74 +++++++++++++++++++-------------------
mm/vma.c | 4 +--
mm/vma.h | 6 ++--
mm/vma_init.c | 4 +--
tools/testing/vma/include/dup.h | 2 +-
tools/testing/vma/include/stubs.h | 6 ++--
10 files changed, 69 insertions(+), 69 deletions(-)
@@ -229,8 +229,8 @@ These are the core fields which describe the MM the VMA belongs to and its attri NUMA balancing in relation to this VMA. lock. Updated under mmap read lock by:c:func:`!task_numa_work`.-:c:member:`!vm_userfaultfd_ctx` CONFIG_USERFAULTFD Userfaultfd context wrapper object of mmap write,- type :c:type:`!vm_userfaultfd_ctx`, VMA write.+:c:member:`!vm_uffd_state` CONFIG_USERFAULTFD Userfaultfd context wrapper object of mmap write,+ type :c:type:`!vm_uffd_state`, VMA write. either of zero size if userfaultfd is disabled, or containing a pointer to an underlying
@@ -1812,8 +1812,8 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,return-EINVAL;/* Ensure dst_vma is registered in uffd we are operating on */-if(!dst_vma->vm_userfaultfd_ctx.ctx||-dst_vma->vm_userfaultfd_ctx.ctx!=ctx)+if(!dst_vma->vm_uffd_state.ctx||+dst_vma->vm_uffd_state.ctx!=ctx)return-EINVAL;/* Only allow moving across anonymous vmas */
@@ -2389,10 +2389,10 @@ static void userfaultfd_release_new(struct userfaultfd_ctx *ctx)structvm_area_struct*vma;VMA_ITERATOR(vmi,mm,0);-/* the various vma->vm_userfaultfd_ctx still points to it */+/* the various vma->vm_uffd_state still points to it */mmap_write_lock(mm);for_each_vma(vmi,vma){-if(vma->vm_userfaultfd_ctx.ctx==ctx)+if(vma->vm_uffd_state.ctx==ctx)userfaultfd_reset_ctx(vma);}mmap_write_unlock(mm);
@@ -3293,7 +3293,7 @@ int userfaultfd_unmap_prep(struct vm_area_struct *vma, unsigned long start,unsignedlongend,structlist_head*unmaps){structuserfaultfd_unmap_ctx*unmap_ctx;-structuserfaultfd_ctx*ctx=vma->vm_userfaultfd_ctx.ctx;+structuserfaultfd_ctx*ctx=vma->vm_uffd_state.ctx;if(!ctx||!(ctx->features&UFFD_FEATURE_EVENT_UNMAP)||has_unmap_ctx(ctx,unmaps,start,end))
@@ -3813,7 +3813,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,do{cond_resched();-VM_WARN_ON_ONCE(!!cur->vm_userfaultfd_ctx.ctx^+VM_WARN_ON_ONCE(!!cur->vm_uffd_state.ctx^!!(cur->vm_flags&__VM_UFFD_FLAGS));/* check not compatible vmas */
@@ -3867,8 +3867,8 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,*wouldn'tknowwhichonetodelivertheuserfaultsto.*/ret=-EBUSY;-if(cur->vm_userfaultfd_ctx.ctx&&-cur->vm_userfaultfd_ctx.ctx!=ctx)+if(cur->vm_uffd_state.ctx&&+cur->vm_uffd_state.ctx!=ctx)gotoout_unlock;/*
@@ -3877,7 +3877,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,*subsequentmprotect()wouldthenpromotestalemarkers*intotheothermode.Requireanunregisterfirst.*/-if(cur->vm_userfaultfd_ctx.ctx==ctx&&+if(cur->vm_uffd_state.ctx==ctx&&cur->vm_flags&(VM_UFFD_WP|VM_UFFD_RWP)&~vm_flags)gotoout_unlock;
@@ -3985,15 +3985,15 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,do{cond_resched();-VM_WARN_ON_ONCE(!!cur->vm_userfaultfd_ctx.ctx^+VM_WARN_ON_ONCE(!!cur->vm_uffd_state.ctx^!!(cur->vm_flags&__VM_UFFD_FLAGS));/**Preventunregisteringthroughadifferentuserfaultfdthan*theoneusedforregistration.*/-if(cur->vm_userfaultfd_ctx.ctx&&-cur->vm_userfaultfd_ctx.ctx!=ctx)+if(cur->vm_uffd_state.ctx&&+cur->vm_uffd_state.ctx!=ctx)gotoout_unlock;/*
@@ -4020,10 +4020,10 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,cond_resched();/* VMA not registered with userfaultfd. */-if(!vma->vm_userfaultfd_ctx.ctx)+if(!vma->vm_uffd_state.ctx)gotoskip;-VM_WARN_ON_ONCE(vma->vm_userfaultfd_ctx.ctx!=ctx);+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->vm_flags&VM_MAYWRITE));
@@ -4041,7 +4041,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,structuserfaultfd_wake_rangerange;range.start=start;range.len=vma_end-start;-wake_userfault(vma->vm_userfaultfd_ctx.ctx,&range);+wake_userfault(vma->vm_uffd_state.ctx,&range);}vma=userfaultfd_clear_vma(&vmi,prev,vma,
@@ -4382,7 +4382,7 @@ static int userfaultfd_set_mode(struct userfaultfd_ctx *ctx,VMA_ITERATOR(vmi,mm,0);for_each_vma(vmi,vma){-if(vma->vm_userfaultfd_ctx.ctx==ctx)+if(vma->vm_uffd_state.ctx==ctx)vma_start_write(vma);}}
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(-)
@@ -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 */structuffdio_api{/* userland asks for an API number and the features to enable */
@@ -2698,7 +2698,7 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx,#elsestaticinlinebooluserfaultfd_huge_must_wait(structuserfaultfd_ctx*ctx,structvm_fault*vmf,-unsignedlongreason)+enumuf_reasonreason){/* Should never get here. */VM_WARN_ON_ONCE(1);
@@ -2835,7 +2835,7 @@ static inline unsigned int userfaultfd_get_blocking_state(unsigned int flags)*fatal_signal_pending()s,andthemmap_lockmustbereleasedbefore*returningit.*/-vm_fault_thandle_userfault(structvm_fault*vmf,unsignedlongreason)+vm_fault_thandle_userfault(structvm_fault*vmf,enumuf_reasonreason){structvm_area_struct*vma=vmf->vma;structmm_struct*mm=vma->vm_mm;
@@ -2861,7 +2861,7 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason)VM_WARN_ON_ONCE(ctx->mm!=mm);/* Any unrecognized flag is a bug. */-VM_WARN_ON_ONCE(reason&~__VM_UFFD_FLAGS);+VM_WARN_ON_ONCE(reason&~USERFAULT_ANY);/* 0 or > 1 flags set is a bug; we expect exactly 1. */VM_WARN_ON_ONCE(!reason||(reason&(reason-1)));
Add 'mode' field to struct vm_uffd_state and define UFFD_MODE_ flags.
Use this field to differentiate VMA registration with userfaultfd
instead of relying on VM_UFFD_* flags.
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).
Update the relevant code to use UFFD_MODE_* instead of VM_UFFD_* flags.
Remove VM_UFFD_WP and VM_UFFD_RWP from VM_COPY_ON_FORK, adding an
explicit userfaultfd_protected() check in vma_needs_copy() instead.
/proc/pid/smaps representation of VmFlags is slightly changed:
- any VMA registered with UFFD shows 'uf'
- the existing userfault markers ('um', 'uw', 'ui', 'ur') are shown after
VmFlags rather than in the middle
Assisted-by: copilot:claude-opus-4.6
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
fs/proc/task_mmu.c | 18 +++---
include/linux/mm.h | 67 +++++-----------------
include/linux/mm_types.h | 1 +
include/linux/pgtable.h | 4 +-
include/linux/userfaultfd_k.h | 37 +++++++-----
include/trace/events/mmflags.h | 17 +-----
mm/gup.c | 5 +-
mm/hugetlb.c | 2 +-
mm/khugepaged.c | 2 +-
mm/memory.c | 6 +-
mm/mprotect.c | 2 +-
mm/shmem.c | 2 +-
mm/userfaultfd.c | 123 +++++++++++++++++++++-------------------
tools/testing/vma/include/dup.h | 18 ++----
14 files changed, 134 insertions(+), 170 deletions(-)
@@ -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 */#else/* nommu: R/O MAP_PRIVATE mapping that might overlay a file mapping */DECLARE_VMA_BIT(MAYOVERLAY,9),
@@ -311,7 +311,7 @@ enum {/* Page-ranges managed without "struct page", just pure PFN */DECLARE_VMA_BIT(PFNMAP,10),DECLARE_VMA_BIT(MAYBE_GUARD,11),-DECLARE_VMA_BIT(UFFD_WP,12),/* wrprotect pages tracking */+/* Bit 12 is free */DECLARE_VMA_BIT(LOCKED,13),DECLARE_VMA_BIT(IO,14),/* Memory mapped I/O or similar */DECLARE_VMA_BIT(SEQ_READ,15),/* App will access data sequentially */
@@ -352,9 +352,8 @@ enum {#elif defined(CONFIG_64BIT)DECLARE_VMA_BIT(DROPPABLE,40),#endif-DECLARE_VMA_BIT(UFFD_MINOR,41),+/* Bits 41 and 43 are free */DECLARE_VMA_BIT(SEALED,42),-DECLARE_VMA_BIT(UFFD_RWP,43),/* Flags that reuse flags above. */DECLARE_VMA_BIT_ALIAS(PKEY_BIT0,HIGH_ARCH_0),DECLARE_VMA_BIT_ALIAS(PKEY_BIT1,HIGH_ARCH_1),
@@ -50,10 +50,10 @@ struct mfill_state {pmd_t*pmd;};-staticboolanon_can_userfault(structvm_area_struct*vma,vm_flags_tvm_flags)+staticboolanon_can_userfault(structvm_area_struct*vma,unsignedintmode){/* anonymous memory does not support MINOR mode */-if(vm_flags&VM_UFFD_MINOR)+if(mode&UFFD_MODE_MINOR)returnfalse;returntrue;}
@@ -462,7 +462,7 @@ static int mfill_copy_folio_locked(struct folio *folio, unsigned long src_addr)}#define MFILL_RETRY_STATE_VMA_FLAGS \-append_vma_flags(__VMA_UFFD_FLAGS,VMA_SHARED_BIT)+append_vma_flags(VMA_UFFD,VMA_SHARED_BIT)/**VMAstatesavedbeforedroppingthelocksinmfill_copy_folio_retry().
@@ -109,7 +109,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 */#else/* nommu: R/O MAP_PRIVATE mapping that might overlay a file mapping */DECLARE_VMA_BIT(MAYOVERLAY,9),
@@ -117,7 +117,7 @@ enum {/* Page-ranges managed without "struct page", just pure PFN */DECLARE_VMA_BIT(PFNMAP,10),DECLARE_VMA_BIT(MAYBE_GUARD,11),-DECLARE_VMA_BIT(UFFD_WP,12),/* wrprotect pages tracking */+/* Bit 12 is free */DECLARE_VMA_BIT(LOCKED,13),DECLARE_VMA_BIT(IO,14),/* Memory mapped I/O or similar */DECLARE_VMA_BIT(SEQ_READ,15),/* App will access data sequentially */
@@ -158,7 +158,7 @@ enum {#elseDECLARE_VMA_BIT(DROPPABLE,40),#endif-DECLARE_VMA_BIT(UFFD_MINOR,41),+/* Bit 41 is free */DECLARE_VMA_BIT(SEALED,42),/* Flags that reuse flags above. */DECLARE_VMA_BIT_ALIAS(PKEY_BIT0,HIGH_ARCH_0),
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Incomplete API refactoring: userfaultfd_wp() mock in VMA test framework was not updated to take a const VMA.
--
commit 6a82c674bb78b012b867cde9192fff955e6adee6
Author: Mike Rapoport (Microsoft) [off-list ref]
userfaultfd: constify VMA parameter of userfaultfd_*() helpers
This patch constifies the VMA parameter for various userfaultfd helper
functions. These helpers only read from the VMA, so this change ensures
const-correctness without affecting functionality.
[Severity: Low]
Does the mock definition of userfaultfd_wp() in the VMA test framework also
need to be updated to take a const VMA?
tools/testing/vma/include/stubs.h:userfaultfd_wp() {
static inline bool userfaultfd_wp(struct vm_area_struct *vma)
{
return false;
}
}
Leaving this mock out of sync with the actual kernel API might break the VMA
userland test suite build (due to a "discarding const qualifier" warning) if
future code passes a const struct vm_area_struct * to this function.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260823-uffd-vm-flags-v1-v1-0-3086981b33cf@kernel.org?part=2
From: Barry Song <baohua@kernel.org> Date: 2026-08-23 21:03:40
On Sun, Aug 23, 2026 at 8:18 PM Mike Rapoport (Microsoft)
[off-list ref] wrote:
userfaultfd_{missing,wp,minor,rwp}() and userfaultfd_protected() only
read the VMA.
Make their vma parameter const.
No functional change.
Assisted-by: copilot:claude-opus-5
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
LGTM, thanks!
Reviewed-by: Barry Song <baohua@kernel.org>
From: Barry Song <baohua@kernel.org> Date: 2026-08-23 21:04:10
On Sun, Aug 23, 2026 at 8:17 PM Mike Rapoport (Microsoft)
[off-list ref] wrote:
gup_can_follow_protnone() is defined in include/linux/mm.h but only used
by mm/gup.c.
First, there is no reason to have it in already gigantic header.
Next, the upcoming refactoring of userfaultfd flags will make
gup_can_follow_protnone() depend on userfaultfd_k.h which would cause a
cyclic header dependency.
Move gup_can_follow_protnone() to mm/gup.c.
No functional change.
Assisted-by: copilot:claude-opus-5
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
LGTM, thanks!
Reviewed-by: Barry Song <baohua@kernel.org>
From: Barry Song <baohua@kernel.org> Date: 2026-08-23 21:14:22
On Sun, Aug 23, 2026 at 8:18 PM Mike Rapoport (Microsoft)
[off-list ref] wrote:
Move userfaultfd_{missing,wp,minor,rwp}() and userfaultfd_protected()
ahead of uffd_disable_huge_pmd_share() and uffd_disable_fault_around()
and make the latter two use the helpers rather than open coded VMA flag
masks.
Convert open coded VMA flag test in mfill_get_vma() to userfaultfd_wp()
as well.
With every user of the per-VMA uffd modes going through the helpers,
their underlying representation can be changed in the next step.
No functional change.
Assisted-by: copilot:claude-opus-5
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
Looks like this drops registration mode from the retry snapshot. Assume a
shared shmem VMA is registered for MISSING and COPY reaches
mfill_copy_folio_retry(). While locks are dropped, the same userfaultfd|
can re-register the range for MINOR. VMA_UFFD, VM_SHARED, ops, file and
pgoff all stay unchanged, so the old COPY can continue instead of
returning -EAGAIN ... no?
The snapshot and comparison bracket the unlocked copy:
static int mfill_copy_folio_retry(struct mfill_state *mfill_state,
struct folio *folio)
{
...
mfill_retry_state_save(&retry_state, mfill_state->vma);
/* retry copying with mm_lock dropped */
mfill_put_vma(mfill_state);
...
/* reget VMA and PMD, they could change underneath us */
err = mfill_get_vma(mfill_state);
if (err)
return err;
if (mfill_retry_state_changed(&retry_state, mfill_state->vma))
return -EAGAIN;
...
}
Since mode now lives in vm_uffd_state.mode, could we save it before
mfill_put_vma() and compare it after mfill_get_vma()? The UFFD flags
comment also needs an update, since the mask no longer contains per mode
flags.
Maybe something like this?
---8<---
From: Muchun Song <muchun.song@linux.dev> Date: 2026-08-24 08:13:04
On Aug 23, 2026, at 20:17, Mike Rapoport (Microsoft) [off-list ref] 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>
Acked-by: Muchun Song <muchun.song@linux.dev> # for HugeTLB.
Thanks.
Looks like this drops registration mode from the retry snapshot. Assume a
shared shmem VMA is registered for MISSING and COPY reaches
mfill_copy_folio_retry(). While locks are dropped, the same userfaultfd|
can re-register the range for MINOR. VMA_UFFD, VM_SHARED, ops, file and
pgoff all stay unchanged, so the old COPY can continue instead of
returning -EAGAIN ... no?
@@ -493,8 +495,9 @@ static bool mfill_retry_state_changed(struct mfill_retry_state *state,vma_flags_tflags=vma_flags_and_mask(&vma->flags,MFILL_RETRY_STATE_VMA_FLAGS);-/* Have any UFFD flags (missing, WP, minor) changed? */-if(!vma_flags_same_pair(&state->flags,&flags))+/* UFFD registration mode or VMA sharing changed */+if(!vma_flags_same_pair(&state->flags,&flags)||+s->mode!=uffd_mode(vma))returntrue;/* VMA type or effective uffd_ops changed while the lock was dropped */
Looks like this drops registration mode from the retry snapshot. Assume a
shared shmem VMA is registered for MISSING and COPY reaches
mfill_copy_folio_retry(). While locks are dropped, the same userfaultfd|
can re-register the range for MINOR. VMA_UFFD, VM_SHARED, ops, file and
pgoff all stay unchanged, so the old COPY can continue instead of
returning -EAGAIN ... no?
On 8/23/26 14:17, Mike Rapoport (Microsoft) wrote:
gup_can_follow_protnone() is defined in include/linux/mm.h but only used
by mm/gup.c.
First, there is no reason to have it in already gigantic header.
Once upon a time there was a user in mm/huge_memory.c, in a beautifully named
function called follow_trans_huge_pmd().
Next, the upcoming refactoring of userfaultfd flags will make
gup_can_follow_protnone() depend on userfaultfd_k.h which would cause a
cyclic header dependency.
Move gup_can_follow_protnone() to mm/gup.c.
No functional change.
Assisted-by: copilot:claude-opus-5
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
[...]
quoted hunk
typedef int (*pte_fn_t)(pte_t *pte, unsigned long addr, void *data);
extern int apply_to_page_range(struct mm_struct *mm, unsigned long address,
unsigned long size, pte_fn_t fn, void *data);
On 8/23/26 14:17, Mike Rapoport (Microsoft) wrote:
Rename struct vm_userfaultfd_ctx to vm_uffd_state to better reflect that
it will represent the userfaultfd state for a VMA rather than just a
context pointer.
This is a preparatory step for extending the struct with a mode field.
Mechanical rename, no functional change.
Assisted-by: copilot:claude-opus-4.6
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
Acked-by: David Hildenbrand (Arm) <david@kernel.org>
--
Cheers,
David
On 8/23/26 14:17, Mike Rapoport (Microsoft) wrote:
quoted hunk
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(-)
On Sun, Aug 23, 2026 at 03:17:38PM +0300, Mike Rapoport (Microsoft) wrote:
gup_can_follow_protnone() is defined in include/linux/mm.h but only used
by mm/gup.c.
First, there is no reason to have it in already gigantic header.
Next, the upcoming refactoring of userfaultfd flags will make
gup_can_follow_protnone() depend on userfaultfd_k.h which would cause a
cyclic header dependency.
Move gup_can_follow_protnone() to mm/gup.c.
No functional change.
Assisted-by: copilot:claude-opus-5
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
LGTM so:
Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
On Sun, Aug 23, 2026 at 03:17:39PM +0300, Mike Rapoport (Microsoft) wrote:
userfaultfd_{missing,wp,minor,rwp}() and userfaultfd_protected() only
read the VMA.
Make their vma parameter const.
No functional change.
Assisted-by: copilot:claude-opus-5
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
LGTM and compiles GTM so:
Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
On Sun, Aug 23, 2026 at 03:17:40PM +0300, Mike Rapoport (Microsoft) wrote:
Move userfaultfd_{missing,wp,minor,rwp}() and userfaultfd_protected()
ahead of uffd_disable_huge_pmd_share() and uffd_disable_fault_around()
and make the latter two use the helpers rather than open coded VMA flag
masks.
Convert open coded VMA flag test in mfill_get_vma() to userfaultfd_wp()
as well.
It'd be better to do the moves and the reworks separately. We don't have a limit
on patch count :)
With every user of the per-VMA uffd modes going through the helpers,
their underlying representation can be changed in the next step.
No functional change.
There is a functional change, or at least seems to be, see below.
This is changing the logic.
Before we were testing only the flags, now we have:
static inline bool userfaultfd_rwp(const struct vm_area_struct *vma)
{
/*
* Callers gate PAGE_NONE usage on this; PAGE_NONE is a BUILD_BUG()
* without CONFIG_ARCH_HAS_PTE_PROTNONE, so fold to false.
*/
if (!IS_ENABLED(CONFIG_ARCH_HAS_PTE_PROTNONE))
return false;
return vma_test_single_mask(vma, VMA_UFFD_RWP);
}
I.e. adding in a CONFIG_ARCH_HAS_PTE_PROTNONE check.
BTW side-note these:
static inline bool userfaultfd_missing(const struct vm_area_struct *vma)
{
return vma_test_any_mask(vma, VMA_UFFD_MISSING);
}
static inline bool userfaultfd_wp(const struct vm_area_struct *vma)
{
return vma_test_any_mask(vma, VMA_UFFD_WP);
}
static inline bool userfaultfd_minor(const struct vm_area_struct *vma)
{
return vma_test_any_mask(vma, VMA_UFFD_MINOR);
}
Should all use vma_test_single_mask() really :)
On Sun, Aug 23, 2026 at 03:17:41PM +0300, Mike Rapoport (Microsoft) wrote:
Rename struct vm_userfaultfd_ctx to vm_uffd_state to better reflect that
it will represent the userfaultfd state for a VMA rather than just a
context pointer.
This is a preparatory step for extending the struct with a mode field.
Mechanical rename, no functional change.
Assisted-by: copilot:claude-opus-4.6
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
LGTM and compiles GTM so:
Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
@@ -229,8 +229,8 @@ These are the core fields which describe the MM the VMA belongs to and its attri NUMA balancing in relation to this VMA. lock. Updated under mmap read lock by:c:func:`!task_numa_work`.-:c:member:`!vm_userfaultfd_ctx` CONFIG_USERFAULTFD Userfaultfd context wrapper object of mmap write,- type :c:type:`!vm_userfaultfd_ctx`, VMA write.+:c:member:`!vm_uffd_state` CONFIG_USERFAULTFD Userfaultfd context wrapper object of mmap write,+ type :c:type:`!vm_uffd_state`, VMA write. either of zero size if userfaultfd is disabled, or containing a pointer to an underlying
@@ -1812,8 +1812,8 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,return-EINVAL;/* Ensure dst_vma is registered in uffd we are operating on */-if(!dst_vma->vm_userfaultfd_ctx.ctx||-dst_vma->vm_userfaultfd_ctx.ctx!=ctx)+if(!dst_vma->vm_uffd_state.ctx||+dst_vma->vm_uffd_state.ctx!=ctx)return-EINVAL;/* Only allow moving across anonymous vmas */
@@ -2389,10 +2389,10 @@ static void userfaultfd_release_new(struct userfaultfd_ctx *ctx)structvm_area_struct*vma;VMA_ITERATOR(vmi,mm,0);-/* the various vma->vm_userfaultfd_ctx still points to it */+/* the various vma->vm_uffd_state still points to it */mmap_write_lock(mm);for_each_vma(vmi,vma){-if(vma->vm_userfaultfd_ctx.ctx==ctx)+if(vma->vm_uffd_state.ctx==ctx)userfaultfd_reset_ctx(vma);}mmap_write_unlock(mm);
@@ -3293,7 +3293,7 @@ int userfaultfd_unmap_prep(struct vm_area_struct *vma, unsigned long start,unsignedlongend,structlist_head*unmaps){structuserfaultfd_unmap_ctx*unmap_ctx;-structuserfaultfd_ctx*ctx=vma->vm_userfaultfd_ctx.ctx;+structuserfaultfd_ctx*ctx=vma->vm_uffd_state.ctx;if(!ctx||!(ctx->features&UFFD_FEATURE_EVENT_UNMAP)||has_unmap_ctx(ctx,unmaps,start,end))
@@ -3813,7 +3813,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,do{cond_resched();-VM_WARN_ON_ONCE(!!cur->vm_userfaultfd_ctx.ctx^+VM_WARN_ON_ONCE(!!cur->vm_uffd_state.ctx^!!(cur->vm_flags&__VM_UFFD_FLAGS));/* check not compatible vmas */
@@ -3867,8 +3867,8 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,*wouldn'tknowwhichonetodelivertheuserfaultsto.*/ret=-EBUSY;-if(cur->vm_userfaultfd_ctx.ctx&&-cur->vm_userfaultfd_ctx.ctx!=ctx)+if(cur->vm_uffd_state.ctx&&+cur->vm_uffd_state.ctx!=ctx)gotoout_unlock;/*
@@ -3877,7 +3877,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,*subsequentmprotect()wouldthenpromotestalemarkers*intotheothermode.Requireanunregisterfirst.*/-if(cur->vm_userfaultfd_ctx.ctx==ctx&&+if(cur->vm_uffd_state.ctx==ctx&&cur->vm_flags&(VM_UFFD_WP|VM_UFFD_RWP)&~vm_flags)gotoout_unlock;
@@ -3985,15 +3985,15 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,do{cond_resched();-VM_WARN_ON_ONCE(!!cur->vm_userfaultfd_ctx.ctx^+VM_WARN_ON_ONCE(!!cur->vm_uffd_state.ctx^!!(cur->vm_flags&__VM_UFFD_FLAGS));/**Preventunregisteringthroughadifferentuserfaultfdthan*theoneusedforregistration.*/-if(cur->vm_userfaultfd_ctx.ctx&&-cur->vm_userfaultfd_ctx.ctx!=ctx)+if(cur->vm_uffd_state.ctx&&+cur->vm_uffd_state.ctx!=ctx)gotoout_unlock;/*
@@ -4020,10 +4020,10 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,cond_resched();/* VMA not registered with userfaultfd. */-if(!vma->vm_userfaultfd_ctx.ctx)+if(!vma->vm_uffd_state.ctx)gotoskip;-VM_WARN_ON_ONCE(vma->vm_userfaultfd_ctx.ctx!=ctx);+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->vm_flags&VM_MAYWRITE));
@@ -4041,7 +4041,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,structuserfaultfd_wake_rangerange;range.start=start;range.len=vma_end-start;-wake_userfault(vma->vm_userfaultfd_ctx.ctx,&range);+wake_userfault(vma->vm_uffd_state.ctx,&range);}vma=userfaultfd_clear_vma(&vmi,prev,vma,
@@ -4382,7 +4382,7 @@ static int userfaultfd_set_mode(struct userfaultfd_ctx *ctx,VMA_ITERATOR(vmi,mm,0);for_each_vma(vmi,vma){-if(vma->vm_userfaultfd_ctx.ctx==ctx)+if(vma->vm_uffd_state.ctx==ctx)vma_start_write(vma);}}
On Sun, Aug 23, 2026 at 03:17:42PM +0300, Mike Rapoport (Microsoft) wrote:
quoted hunk
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(-)
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?
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.
@@ -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
See below for some actual practical justification...
quoted hunk
struct uffdio_api {
/* userland asks for an API number and the features to enable */
@@ -2684,13 +2684,13 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, * If VMA has UFFD WP faults enabled and WP fault, wait for userspace to * resolve the fault. */- if (!huge_pte_write(pte) && (reason & VM_UFFD_WP))+ if (!huge_pte_write(pte) && (reason & USERFAULT_WP)) return true; /* * PTE is still RW-protected (protnone with uffd bit), wait for * resolution. Plain PROT_NONE without the marker is not an RWP fault. */- if (pte_protnone(pte) && huge_pte_uffd(pte) && (reason & VM_UFFD_RWP))+ if (pte_protnone(pte) && huge_pte_uffd(pte) && (reason & USERFAULT_RWP)) return true; return false;
@@ -2698,7 +2698,7 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, #else static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, struct vm_fault *vmf,- unsigned long reason)+ enum uf_reason reason) { /* Should never get here. */ VM_WARN_ON_ONCE(1);
@@ -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))
quoted hunk
goto out; /* * PTE is still RW-protected (protnone with uffd bit), wait for * userspace to resolve. Plain PROT_NONE without the marker is not * an RWP fault. */- if (pte_protnone(ptent) && pte_uffd(ptent) && (reason & VM_UFFD_RWP))+ if (pte_protnone(ptent) && pte_uffd(ptent) && (reason & USERFAULT_RWP)) goto out; ret = false;
@@ -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?
@@ -2861,7 +2861,7 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason) VM_WARN_ON_ONCE(ctx->mm != mm); /* Any unrecognized flag is a bug. */- VM_WARN_ON_ONCE(reason & ~__VM_UFFD_FLAGS);+ VM_WARN_ON_ONCE(reason & ~USERFAULT_ANY); /* 0 or > 1 flags set is a bug; we expect exactly 1. */ VM_WARN_ON_ONCE(!reason || (reason & (reason - 1)));--
On Sun Aug 23, 2026 at 8:17 AM EDT, Mike Rapoport (Microsoft) wrote:
gup_can_follow_protnone() is defined in include/linux/mm.h but only used
by mm/gup.c.
First, there is no reason to have it in already gigantic header.
Next, the upcoming refactoring of userfaultfd flags will make
gup_can_follow_protnone() depend on userfaultfd_k.h which would cause a
cyclic header dependency.
Move gup_can_follow_protnone() to mm/gup.c.
No functional change.
Assisted-by: copilot:claude-opus-5
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
include/linux/mm.h | 38 --------------------------------------
mm/gup.c | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 38 insertions(+), 38 deletions(-)
LGTM.
Reviewed-by: Zi Yan <ziy@nvidia.com>
--
Best Regards,
Yan, Zi
On Sun Aug 23, 2026 at 8:17 AM EDT, Mike Rapoport (Microsoft) wrote:
userfaultfd_{missing,wp,minor,rwp}() and userfaultfd_protected() only
read the VMA.
Make their vma parameter const.
No functional change.
Assisted-by: copilot:claude-opus-5
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
include/linux/userfaultfd_k.h | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
LGTM.
Reviewed-by: Zi Yan <ziy@nvidia.com>
--
Best Regards,
Yan, Zi
From: Mike Rapoport <rppt@kernel.org> Date: 2026-08-25 10:10:18
On Mon, Aug 24, 2026 at 04:42:56PM +0200, David Hildenbrand (Arm) wrote:
On 8/23/26 14:17, Mike Rapoport (Microsoft) wrote:
quoted
gup_can_follow_protnone() is defined in include/linux/mm.h but only used
by mm/gup.c.
First, there is no reason to have it in already gigantic header.
Once upon a time there was a user in mm/huge_memory.c, in a beautifully named
function called follow_trans_huge_pmd().
quoted
Next, the upcoming refactoring of userfaultfd flags will make
gup_can_follow_protnone() depend on userfaultfd_k.h which would cause a
cyclic header dependency.
Move gup_can_follow_protnone() to mm/gup.c.
No functional change.
Assisted-by: copilot:claude-opus-5
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
[...]
quoted
typedef int (*pte_fn_t)(pte_t *pte, unsigned long addr, void *data);
extern int apply_to_page_range(struct mm_struct *mm, unsigned long address,
unsigned long size, pte_fn_t fn, void *data);
From: Mike Rapoport <rppt@kernel.org> Date: 2026-08-25 10:37:44
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:
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(-)
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.
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" :)
quoted
@@ -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.
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.
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.
@@ -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.
Yeah, that looks alright to me as well.
--
Cheers,
David
From: Mike Rapoport <rppt@kernel.org> Date: 2026-08-25 11:20:07
On Mon, Aug 24, 2026 at 04:10:51PM +0100, Lorenzo Stoakes (ARM) wrote:
On Sun, Aug 23, 2026 at 03:17:40PM +0300, Mike Rapoport (Microsoft) wrote:
quoted
Move userfaultfd_{missing,wp,minor,rwp}() and userfaultfd_protected()
ahead of uffd_disable_huge_pmd_share() and uffd_disable_fault_around()
and make the latter two use the helpers rather than open coded VMA flag
masks.
Convert open coded VMA flag test in mfill_get_vma() to userfaultfd_wp()
as well.
It'd be better to do the moves and the reworks separately. We don't have a limit
on patch count :)
quoted
With every user of the per-VMA uffd modes going through the helpers,
their underlying representation can be changed in the next step.
No functional change.
There is a functional change, or at least seems to be, see below.
This is changing the logic.
Before we were testing only the flags, now we have:
static inline bool userfaultfd_rwp(const struct vm_area_struct *vma)
{
/*
* Callers gate PAGE_NONE usage on this; PAGE_NONE is a BUILD_BUG()
* without CONFIG_ARCH_HAS_PTE_PROTNONE, so fold to false.
*/
if (!IS_ENABLED(CONFIG_ARCH_HAS_PTE_PROTNONE))
return false;
return vma_test_single_mask(vma, VMA_UFFD_RWP);
}
I.e. adding in a CONFIG_ARCH_HAS_PTE_PROTNONE check.
Without CONFIG_ARCH_HAS_PTE_PROTNONE VMA_UFFD_RWP is hardwired to VM_NONE
so it's functionally the same ;-)
On Tue, Aug 25, 2026 at 02:19:52PM +0300, Mike Rapoport wrote:
quoted
quoted
+/*+ * Don't do fault around for WP, RWP or MINOR registered uffd range. For+ * MINOR registered range, fault around will be a total disaster and ptes can+ * be installed without notifications; for WP it should mostly be fine as long+ * as the fault around checks for pte_none() before the installation, however+ * to be super safe we just forbid it; for RWP, pre-faulted neighbours would+ * be indistinguishable from accessed pages in PAGEMAP_SCAN (PAGE_IS_ACCESSED)+ * and pollute the tracked working set, so each page must be populated by its+ * own fault.+ */+static inline bool uffd_disable_fault_around(struct vm_area_struct *vma)+{+ return userfaultfd_minor(vma) || userfaultfd_wp(vma) ||+ userfaultfd_rwp(vma);
This is changing the logic.
Before we were testing only the flags, now we have:
static inline bool userfaultfd_rwp(const struct vm_area_struct *vma)
{
/*
* Callers gate PAGE_NONE usage on this; PAGE_NONE is a BUILD_BUG()
* without CONFIG_ARCH_HAS_PTE_PROTNONE, so fold to false.
*/
if (!IS_ENABLED(CONFIG_ARCH_HAS_PTE_PROTNONE))
return false;
return vma_test_single_mask(vma, VMA_UFFD_RWP);
}
I.e. adding in a CONFIG_ARCH_HAS_PTE_PROTNONE check.
Without CONFIG_ARCH_HAS_PTE_PROTNONE VMA_UFFD_RWP is hardwired to VM_NONE
so it's functionally the same ;-)
Well then you're explicitly removing logic and not mentioning it anywhere
with a NFC commit.
So please say so in the commit message.
On Tue, Aug 25, 2026 at 01:08:33PM +0200, David Hildenbrand (Arm) wrote:
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.
Yeah, that looks alright to me as well.
I find the general inconsistent different sets of flags/bits but not
really/naming all a bit of a mess.
But these are largely aesthetic and I don't maintain this file so I guess
you guys can live without my tag here...
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).
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.
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.
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.
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?
Update the relevant code to use UFFD_MODE_* instead of VM_UFFD_* flags.
USERFAULT_, UF_, UFFD_... Can we settle on one?
Remove VM_UFFD_WP and VM_UFFD_RWP from VM_COPY_ON_FORK, adding an
explicit userfaultfd_protected() check in vma_needs_copy() instead.
/proc/pid/smaps representation of VmFlags is slightly changed:
- any VMA registered with UFFD shows 'uf'
- the existing userfault markers ('um', 'uw', 'ui', 'ur') are shown after
VmFlags rather than in the middle
@@ -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?
Presumably nommu will never set/use VMA_UFFD_BIT (CONFIG_USERFAULTFD won't
be set) and it'll make everything easier this way.
quoted hunk
#else
/* nommu: R/O MAP_PRIVATE mapping that might overlay a file mapping */
DECLARE_VMA_BIT(MAYOVERLAY, 9),
@@ -311,7 +311,7 @@ enum { /* Page-ranges managed without "struct page", just pure PFN */ DECLARE_VMA_BIT(PFNMAP, 10), DECLARE_VMA_BIT(MAYBE_GUARD, 11),- DECLARE_VMA_BIT(UFFD_WP, 12), /* wrprotect pages tracking */+ /* Bit 12 is free */ DECLARE_VMA_BIT(LOCKED, 13), DECLARE_VMA_BIT(IO, 14), /* Memory mapped I/O or similar */ DECLARE_VMA_BIT(SEQ_READ, 15), /* App will access data sequentially */
See above, in general it's annoying to figure out whether the flag is
available and it's easy to slip bugs in.
It's also weird to have flags before that were always declared, and now one that
is not.
But I'm not sure you're even using this now?
@@ -499,36 +498,6 @@ enum { #define VM_MTE VM_NONE #define VM_MTE_ALLOWED VM_NONE #endif-#ifdef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR-#define VM_UFFD_MINOR INIT_VM_FLAG(UFFD_MINOR)-#else-#define VM_UFFD_MINOR VM_NONE-#endif-#ifdef CONFIG_USERFAULTFD_RWP-#define VM_UFFD_RWP INIT_VM_FLAG(UFFD_RWP)-#else-#define VM_UFFD_RWP VM_NONE-#endif--/*- * vma_flags_t masks for the userfaultfd VMA flags. The two high-bit modes are- * gated on the same configs as their VM_* flags above -- both of which imply- * 64BIT -- so an out-of-range bit is never fed to mk_vma_flags() on a build- * whose bitmap cannot hold it.- */-#define VMA_UFFD_MISSING mk_vma_flags(VMA_UFFD_MISSING_BIT)-#define VMA_UFFD_WP mk_vma_flags(VMA_UFFD_WP_BIT)-#ifdef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR-#define VMA_UFFD_MINOR mk_vma_flags(VMA_UFFD_MINOR_BIT)-#else-#define VMA_UFFD_MINOR EMPTY_VMA_FLAGS-#endif-#ifdef CONFIG_USERFAULTFD_RWP-#define VMA_UFFD_RWP mk_vma_flags(VMA_UFFD_RWP_BIT)-#else-#define VMA_UFFD_RWP EMPTY_VMA_FLAGS-#endif- #ifdef CONFIG_64BIT #define VM_ALLOW_ANY_UNCACHED INIT_VM_FLAG(ALLOW_ANY_UNCACHED) #define VM_SEALED INIT_VM_FLAG(SEALED)
@@ -668,32 +637,26 @@ enum { * reconsistuted upon page fault, so necessitate page table copying upon fork. * * Note that these flags should be compared with the DESTINATION VMA not the- * source: VM_UFFD_WP and VM_UFFD_RWP may be cleared on the destination+ * source: uffd WP/RWP mode may be cleared on the destination * (dup_userfaultfd() -> userfaultfd_reset_ctx() when the parent context did * not negotiate UFFD_FEATURE_EVENT_FORK), while all other flags propagate. * * VM_PFNMAP / VM_MIXEDMAP - These contain kernel-mapped data which cannot be * reasonably reconstructed on page fault. *- * VM_UFFD_WP - Encodes metadata about an installed uffd- * VM_UFFD_RWP write- or read-write-protect handler, which- * cannot be reconstructed on page fault.- *- * We always copy pgtables when dst_vma has the- * uffd PTE bit in use even if it's file-backed- * (e.g. shmem). Because when the uffd bit is- * in use, the pgtable contains the protection- * information, that's something we can't- * retrieve from page cache, and skip copying- * will lose those info.- * * VM_MAYBE_GUARD - Could contain page guard region markers which * by design are a property of the page tables * only and thus cannot be reconstructed on page * fault.+ *+ * uffd WP/RWP modes - Encode metadata about an installed uffd+ * write- or read-write-protect handler, which+ * cannot be reconstructed on page fault.+ * This is checked separately via+ * userfaultfd_protected() in vma_needs_copy().+ * */-#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.
quoted hunk
/*
* mapping from the currently active vm_flags protection bits (the
Hmm this is adding 4 bytes at least to every VMA is that OK?
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.
And does all of the NULL stuff now actually still work?
@@ -32,12 +32,13 @@ enum uf_reason {#include<asm-generic/pgtable_uffd.h>#include<linux/hugetlb_inline.h>-/* The set of all possible UFFD-related VM flags. */-#define __VM_UFFD_FLAGS (VM_UFFD_MISSING | VM_UFFD_MINOR | \-VM_UFFD_WP|VM_UFFD_RWP)--#define __VMA_UFFD_FLAGS mk_vma_flags_from_masks(VMA_UFFD_MISSING, VMA_UFFD_WP, \-VMA_UFFD_MINOR,VMA_UFFD_RWP)+/* 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?
And now we're flitting from USERFAULT_ to UFFD_ for some reason...
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?
And weird/inconsistent to declare the USERFAULT_xxx as an enum and #define's
here as well as the naming?
I have to say it's confusing.
quoted hunk
/*
* CAREFUL: Check include/uapi/asm-generic/fcntl.h when defining
@@ -99,7 +100,7 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, enum uf_reason reason); /* VMA userfaultfd operations */ struct vm_uffd_ops { /* Checks if a VMA can support userfaultfd */- bool (*can_userfault)(struct vm_area_struct *vma, vm_flags_t vm_flags);+ bool (*can_userfault)(struct vm_area_struct *vma, unsigned int mode); /* * Called to resolve UFFDIO_CONTINUE request. * Should return the folio found at pgoff in the VMA's pagecache if it
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...
This is broken assuming this can be executed in a context where VMA_UFFD can be
VMA_NONE.
You should use vma_test_single_mask(). Or preferably, as above, just always
provide VMA_UFFD_BIT.
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).
mm/userfaultfd.c: vma_flags_clear_mask(&new_vma_flags, VMA_UFFD);
mm/userfaultfd.c: vma_flags_set_mask(&new_vma_flags, VMA_UFFD);
@@ -50,10 +50,10 @@ struct mfill_state {pmd_t*pmd;};-staticboolanon_can_userfault(structvm_area_struct*vma,vm_flags_tvm_flags)+staticboolanon_can_userfault(structvm_area_struct*vma,unsignedintmode){/* anonymous memory does not support MINOR mode */-if(vm_flags&VM_UFFD_MINOR)+if(mode&UFFD_MODE_MINOR)returnfalse;returntrue;}
@@ -462,7 +462,7 @@ static int mfill_copy_folio_locked(struct folio *folio, unsigned long src_addr)}#define MFILL_RETRY_STATE_VMA_FLAGS \-append_vma_flags(__VMA_UFFD_FLAGS,VMA_SHARED_BIT)+append_vma_flags(VMA_UFFD,VMA_SHARED_BIT)/**VMAstatesavedbeforedroppingthelocksinmfill_copy_folio_retry().
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.
At any rate 'modes' or 'flags' or something would be clearer here.
quoted hunk
return true;
/* For any other mode reject VMAs that don't implement vm_uffd_ops */
@@ -2222,19 +2220,31 @@ static bool vma_can_userfault(struct vm_area_struct *vma, vm_flags_t vm_flags, * If user requested uffd-wp but not enabled pte markers for * uffd-wp, then only anonymous memory is supported */- if (!uffd_supports_wp_marker() && (vm_flags & VM_UFFD_WP) &&+ if (!uffd_supports_wp_marker() && (mode & UFFD_MODE_WP) && !vma_is_anonymous(vma)) return false;- return ops->can_userfault(vma, vm_flags);+ return ops->can_userfault(vma, mode); }-static void userfaultfd_set_vm_flags(struct vm_area_struct *vma,- vm_flags_t vm_flags)+static void userfaultfd_set_ctx(struct vm_area_struct *vma,+ struct userfaultfd_ctx *ctx,+ unsigned int mode) {- const bool uffd_wp_changed = (vma->vm_flags ^ vm_flags) & VM_UFFD_WP;+ const bool uffd_wp_changed = (uffd_mode(vma) ^ mode) & UFFD_MODE_WP;++ vma_start_write(vma);++ vma->vm_uffd_state = (struct vm_uffd_state){+ .ctx = ctx,+ .mode = mode,+ };++ if (mode)+ vma_set_flags(vma, VMA_UFFD_BIT);+ else+ vma_clear_flags(vma, VMA_UFFD_BIT);- vm_flags_reset(vma, vm_flags); /* * For shared mappings, we want to enable writenotify while * userfaultfd-wp is enabled (see vma_wants_writenotify()). We'll simply
@@ -2269,7 +2269,7 @@ static struct vm_area_struct *userfaultfd_clear_vma(struct vma_iterator *vmi, bool give_up_on_oom = false; vma_flags_t new_vma_flags = vma->flags;- vma_flags_clear_mask(&new_vma_flags, __VMA_UFFD_FLAGS);+ vma_flags_clear_mask(&new_vma_flags, VMA_UFFD); /* * If we are modifying only and not splitting, just give up on the merge
@@ -2313,11 +2313,10 @@ static struct vm_area_struct *userfaultfd_clear_vma(struct vma_iterator *vmi, /* Assumes mmap write lock taken, and mm_struct pinned. */ static int userfaultfd_register_range(struct userfaultfd_ctx *ctx, struct vm_area_struct *vma,- vm_flags_t vm_flags,+ unsigned int mode, unsigned long start, unsigned long end, bool wp_async) {- vma_flags_t vma_flags = legacy_to_vma_flags(vm_flags); VMA_ITERATOR(vmi, ctx->mm, start); struct vm_area_struct *prev = vma_prev(&vmi); unsigned long vma_end;
@@ -2339,28 +2338,31 @@ static int userfaultfd_register_range(struct userfaultfd_ctx *ctx, * userfaultfd and with the right tracking mode too. */ if (vma->vm_uffd_state.ctx == ctx &&- vma_test_all_mask(vma, vma_flags))+ (uffd_mode(vma) & mode) == mode) goto skip; /* * Pre-scan in userfaultfd_register() already rejected mode- * switches that would drop VM_UFFD_WP or VM_UFFD_RWP, so a- * stray bit here is a bug.+ * switches that would drop WP or RWP, so a stray bit here+ * is a bug. */ VM_WARN_ON_ONCE(vma->vm_uffd_state.ctx == ctx &&- vma->vm_flags & (VM_UFFD_WP | VM_UFFD_RWP) & ~vm_flags);+ uffd_mode(vma) &+ (UFFD_MODE_WP | UFFD_MODE_RWP) & ~mode); if (vma->vm_start > start) start = vma->vm_start; vma_end = min(end, vma->vm_end); 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.
@@ -2370,7 +2372,7 @@ static int userfaultfd_register_range(struct userfaultfd_ctx *ctx, * the next vma was merged into the current one and * the current one has not been updated yet. */- userfaultfd_set_ctx(vma, ctx, vm_flags);+ userfaultfd_set_ctx(vma, ctx, mode); if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma)) hugetlb_unshare_all_pmds(vma);
@@ -2876,9 +2878,9 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, enum uf_reason reason) * NOTE: it should become possible to return VM_FAULT_RETRY * even if FAULT_FLAG_TRIED is set without leading to gup() * -EBUSY failures, if the userfaultfd is to be extended for- * VM_UFFD_WP tracking and we intend to arm the userfault+ * WP tracking and we intend to arm the userfault * without first stopping userland access to the memory. For- * VM_UFFD_MISSING userfaults this is enough for now.+ * MISSING userfaults this is enough for now. */ if (unlikely(!(vmf->flags & FAULT_FLAG_ALLOW_RETRY))) { /*
@@ -3723,7 +3725,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, int ret; struct uffdio_register uffdio_register; struct uffdio_register __user *user_uffdio_register;- vm_flags_t vm_flags;+ unsigned int mode; bool found; bool basic_ioctls; unsigned long start, end;
@@ -3742,21 +3744,22 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, goto out; if (uffdio_register.mode & ~UFFD_API_REGISTER_MODES) goto out;- vm_flags = 0;+ mode = 0; if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MISSING)- vm_flags |= VM_UFFD_MISSING;+ mode |= UFFD_MODE_MISSING; if (uffdio_register.mode & UFFDIO_REGISTER_MODE_WP) { if (!pgtable_supports_uffd()) goto out;- vm_flags |= VM_UFFD_WP;+ mode |= UFFD_MODE_WP; } if (uffdio_register.mode & UFFDIO_REGISTER_MODE_RWP) {- if (!pgtable_supports_uffd() || VM_UFFD_RWP == VM_NONE)+ if (!pgtable_supports_uffd() ||+ !IS_ENABLED(CONFIG_USERFAULTFD_RWP)) goto out; if (!(userfaultfd_features(ctx) & UFFD_FEATURE_RWP)) goto out;- vm_flags |= VM_UFFD_RWP;+ mode |= UFFD_MODE_RWP; } /*
@@ -3764,14 +3767,14 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, * cannot coexist in the same VMA — the bit would carry ambiguous * semantics. Reject the combination up front. */- if ((vm_flags & VM_UFFD_WP) && (vm_flags & VM_UFFD_RWP))+ if ((mode & UFFD_MODE_WP) && (mode & UFFD_MODE_RWP)) goto out; if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR) { #ifndef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR goto out; #endif- vm_flags |= VM_UFFD_MINOR;+ mode |= UFFD_MODE_MINOR; } ret = validate_range(mm, uffdio_register.range.start,
Same comments as elsewhere re vma_test() on VMA_UFFD_BIT.
quoted hunk
/* check not compatible vmas */ ret = -EINVAL;- if (!vma_can_userfault(cur, vm_flags, wp_async))+ if (!vma_can_userfault(cur, mode, wp_async)) goto out_unlock; /*
@@ -3829,7 +3832,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, * mprotect() must still be unregisterable, so this is not * part of vma_can_userfault(). */- if ((vm_flags & VM_UFFD_RWP) && !vma_is_accessible(cur))+ if ((mode & UFFD_MODE_RWP) && !vma_is_accessible(cur)) goto out_unlock; /*
@@ -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?
Thanks for switching to new VMA flags model though!
quoted hunk
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.
Same objection to the use of the word 'mode'. You really need to say flags
here somehow.
quoted hunk
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:
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.
But it'd make life a lot easier to just use a bit number that isn't
predicated on CONFIG_MMU.
quoted hunk
/*
* 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;
Nit but pretty horrible alignment. Gues it can 't be helped
quoted hunk
VM_WARN_ON_ONCE(!(vma->vm_flags & VM_MAYWRITE));
if (vma->vm_start > start)
@@ -4329,12 +4334,12 @@ static __u64 uffd_api_available_features(void) UFFD_FEATURE_WP_ASYNC); /* * RWP needs both PROT_NONE support and the uffd PTE bit. The- * VM_UFFD_RWP check covers compile-time unavailability; the+ * IS_ENABLED check covers compile-time unavailability; the * pgtable_supports_uffd() check covers runtime (e.g. riscv * without the SVRSW60T59B extension) where the PTE bit is declared * but not actually usable. */- if (VM_UFFD_RWP == VM_NONE || !pgtable_supports_uffd())+ if (!IS_ENABLED(CONFIG_USERFAULTFD_RWP) || !pgtable_supports_uffd()) f &= ~(UFFD_FEATURE_RWP | UFFD_FEATURE_RWP_ASYNC); return f; }
@@ -109,7 +109,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 */#else/* nommu: R/O MAP_PRIVATE mapping that might overlay a file mapping */DECLARE_VMA_BIT(MAYOVERLAY,9),
@@ -117,7 +117,7 @@ enum {/* Page-ranges managed without "struct page", just pure PFN */DECLARE_VMA_BIT(PFNMAP,10),DECLARE_VMA_BIT(MAYBE_GUARD,11),-DECLARE_VMA_BIT(UFFD_WP,12),/* wrprotect pages tracking */+/* Bit 12 is free */DECLARE_VMA_BIT(LOCKED,13),DECLARE_VMA_BIT(IO,14),/* Memory mapped I/O or similar */DECLARE_VMA_BIT(SEQ_READ,15),/* App will access data sequentially */
@@ -158,7 +158,7 @@ enum {#elseDECLARE_VMA_BIT(DROPPABLE,40),#endif-DECLARE_VMA_BIT(UFFD_MINOR,41),+/* Bit 41 is free */DECLARE_VMA_BIT(SEALED,42),/* Flags that reuse flags above. */DECLARE_VMA_BIT_ALIAS(PKEY_BIT0,HIGH_ARCH_0),
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... Could even be with placeholder flag names potentially.
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.
b. Fix the VMA_UFFD_BIT stuff ideally with a bit that's just always set not
predicated on CONFIG_MMU.
c. Improve the engineering so this stuff actually makes the code clearer
rather than just reimplementing the same old confusing uffd mess.
d. Fix the naming... modes, flags, uffd, userfault, uf, etc. let's stick
with one and be consistent.
IMO before it can move forwards.
--
Cheers, Lorenzo
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:
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(-)
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 ;)
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...
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...
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.
quoted
quoted
@@ -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.
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.
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...
From: Mike Rapoport <rppt@kernel.org> Date: 2026-08-27 07:14:23
On Tue, Aug 25, 2026 at 12:26:39PM +0100, Lorenzo Stoakes (ARM) wrote:
On Tue, Aug 25, 2026 at 02:19:52PM +0300, Mike Rapoport wrote:
quoted
quoted
quoted
+/*+ * Don't do fault around for WP, RWP or MINOR registered uffd range. For+ * MINOR registered range, fault around will be a total disaster and ptes can+ * be installed without notifications; for WP it should mostly be fine as long+ * as the fault around checks for pte_none() before the installation, however+ * to be super safe we just forbid it; for RWP, pre-faulted neighbours would+ * be indistinguishable from accessed pages in PAGEMAP_SCAN (PAGE_IS_ACCESSED)+ * and pollute the tracked working set, so each page must be populated by its+ * own fault.+ */+static inline bool uffd_disable_fault_around(struct vm_area_struct *vma)+{+ return userfaultfd_minor(vma) || userfaultfd_wp(vma) ||+ userfaultfd_rwp(vma);
This is changing the logic.
Before we were testing only the flags, now we have:
static inline bool userfaultfd_rwp(const struct vm_area_struct *vma)
{
/*
* Callers gate PAGE_NONE usage on this; PAGE_NONE is a BUILD_BUG()
* without CONFIG_ARCH_HAS_PTE_PROTNONE, so fold to false.
*/
if (!IS_ENABLED(CONFIG_ARCH_HAS_PTE_PROTNONE))
return false;
return vma_test_single_mask(vma, VMA_UFFD_RWP);
}
I.e. adding in a CONFIG_ARCH_HAS_PTE_PROTNONE check.
Without CONFIG_ARCH_HAS_PTE_PROTNONE VMA_UFFD_RWP is hardwired to VM_NONE
so it's functionally the same ;-)
Well then you're explicitly removing logic and not mentioning it anywhere
with a NFC commit.
So please say so in the commit message.
From: Mike Rapoport <rppt@kernel.org> Date: 2026-08-27 07:42:31
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(-)
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.
From: Mike Rapoport <rppt@kernel.org> Date: 2026-08-27 07:49:44
On Mon, Aug 24, 2026 at 04:46:14PM +0200, David Hildenbrand (Arm) wrote:
On 8/23/26 14:17, 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(-)
Can we just call this "userfault_reason" or "uffd_reason" ? Maybe the latter is
actually what we want?
userfault_reason sounds better to me.
It describes what kind of user fault we are handling and the 'fd' part has
nothing to do with it.
We do use uffd as a short name for the subsystem, but still most if not all
userfaultfd "external" APIs use userfault_ prefix.
uf_ was an attempt to make it wee shorter :)
On Mon, Aug 24, 2026 at 04:46:14PM +0200, David Hildenbrand (Arm) wrote:
quoted
On 8/23/26 14:17, 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(-)
Can we just call this "userfault_reason" or "uffd_reason" ? Maybe the latter is
actually what we want?
userfault_reason sounds better to me.
It describes what kind of user fault we are handling and the 'fd' part has
nothing to do with it.
We do use uffd as a short name for the subsystem, but still most if not all
userfaultfd "external" APIs use userfault_ prefix.
uf_ was an attempt to make it wee shorter :)
Yeah, I got that; while uffd is a known acronym, the uf_ not so much (and also I
wouldn't suggest it to become a thing, lol :) )
I've been wondering for a while whether it really should be called
handle_userfault()
And not instead
handle_userfaultfd()
Or maybe even better
handle_uffd_fault()
And then have
uffd_fault_reason
... but just a thought.
--
Cheers,
David
From: Mike Rapoport <rppt@kernel.org> Date: 2026-08-27 09:03:19
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:
quoted
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.
quoted
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.
quoted
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().
quoted
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.
@@ -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.
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?
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.
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...
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).
Shouldn't you update the tracing logic to obtain these from uffd
modes/flags?
Yep, will do.
quoted
/* * 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.
Same comments as elsewhere re vma_test() on VMA_UFFD_BIT.
See above.
quoted
@@ -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 :)
quoted
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.
quoted
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:
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
quoted
/*
* 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;
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
From: Mike Rapoport <rppt@kernel.org> Date: 2026-08-27 09:09:27
On Thu, Aug 27, 2026 at 10:10:24AM +0200, David Hildenbrand (Arm) wrote:
On 8/27/26 09:49, Mike Rapoport wrote:
quoted
On Mon, Aug 24, 2026 at 04:46:14PM +0200, David Hildenbrand (Arm) wrote:
quoted
On 8/23/26 14:17, 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(-)
Can we just call this "userfault_reason" or "uffd_reason" ? Maybe the latter is
actually what we want?
userfault_reason sounds better to me.
It describes what kind of user fault we are handling and the 'fd' part has
nothing to do with it.
We do use uffd as a short name for the subsystem, but still most if not all
userfaultfd "external" APIs use userfault_ prefix.
uf_ was an attempt to make it wee shorter :)
Yeah, I got that; while uffd is a known acronym, the uf_ not so much (and also I
wouldn't suggest it to become a thing, lol :) )
I've been wondering for a while whether it really should be called
handle_userfault()
And not instead
handle_userfaultfd()
The 'fd' part here sounds really weird :)
Or maybe even better
handle_uffd_fault()
That's somehow tautological, but maybe using uffd_ as prefix would make it
a "subsystem namespace", so tautology won't be as blunt:
uffd_handle_fault()
And then have
uffd_fault_reason
Could work, yes. No strong feelings between this one and userfault_reason.
On Thu, Aug 27, 2026 at 10:10:24AM +0200, David Hildenbrand (Arm) wrote:
quoted
On 8/27/26 09:49, Mike Rapoport wrote:
quoted
userfault_reason sounds better to me.
It describes what kind of user fault we are handling and the 'fd' part has
nothing to do with it.
We do use uffd as a short name for the subsystem, but still most if not all
userfaultfd "external" APIs use userfault_ prefix.
uf_ was an attempt to make it wee shorter :)
Yeah, I got that; while uffd is a known acronym, the uf_ not so much (and also I
wouldn't suggest it to become a thing, lol :) )
I've been wondering for a while whether it really should be called
handle_userfault()
And not instead
handle_userfaultfd()
The 'fd' part here sounds really weird :)
quoted
Or maybe even better
handle_uffd_fault()
That's somehow tautological, but maybe using uffd_ as prefix would make it
a "subsystem namespace", so tautology won't be as blunt:
uffd_handle_fault()
quoted
And then have
uffd_fault_reason
Could work, yes. No strong feelings between this one and userfault_reason.
I lean towards just calling stuff uffd consistently might be cleanest.
--
Cheers,
David
On Thu, Aug 27, 2026 at 12:03:03PM +0300, Mike Rapoport wrote:
On Tue, Aug 25, 2026 at 01:44:23PM +0100, Lorenzo Stoakes (ARM) wrote:
quoted
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:
quoted
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.
Thanks.
quoted
quoted
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.
Thanks.
I feel this series is not really clarifying things very well at this point,
it's all still very confusing.
It feels more like taking the existing UFFD approach and just putting a
'store the same state elsewhere' overlay on top, which isn't really helping
anything.
I am left a. confused about how all these flags interact (which suggests
the bad design is still in place) and b. wondering what the benefits are.
Which indicates that the design could be much improved and clarified here.
quoted
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.
There's no hole there see above. I'm not sure if rearranging can fix it?
Maybe there is some other way.
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.
quoted
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().
Yup one of the inconsistently named checks, userfaultfd_protected().
This was from before but really the majority of vma checks are vma_xxx() so
let's actually clean this up and call it vma_is_uffd_protected() or
something like this, please.
quoted
quoted
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.
I think this naming inconsistency is really horrible and confusing, that
has to change. 'uffd' everywhere please.
@@ -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.
Thanks!
quoted
Presumably nommu will never set/use VMA_UFFD_BIT (CONFIG_USERFAULTFD won't
be set) and it'll make everything easier this way.
Hmm this is adding 4 bytes at least to every VMA is that OK?
See above.
See pahole results above.
quoted
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.
Yeah I assumed as much.
I wonder if we can grab the lower bits in the context pointer for this
state actually?
quoted
And does all of the NULL stuff now actually still work?
I think really the point is that it's not clear what each of these distinct
flags are for, I guess I'm repeating myself on that :)
I think the constructive way forward is to make it super clear, whether
through types, commit msg, comments etc.
quoted
And now we're flitting from USERFAULT_ to UFFD_ for some reason...
See above.
Yeah I really think keeping this consistent works best.
quoted
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.
Thanks!
quoted
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.
I mean or we could just put the damn stuff in mm_types.h :) I really hate
usserfaultfd_k.h as a header honestly. It makes little sense.
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...
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.
OK but now that means there's a subtle hidden dependency there that is
really easy to break in future.
(I assume this is true of every other case where I've flagged this, I guess
I could ask AI to check :)
In any case using bit 12 eliminates all this so moot.
quoted
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.
Yeah I agree that it's an unfortunate aspect of the VMA flags design,
unfortunately it's kind of inherent - to have a bitmap VMA flags type you
have to define by bit and lose the VM_NONE stuff, and this solution is
really a bit of a hack.
Really the problem only emerges in practice in two cases:
1. A 64-bit value in a 32-bit kernel (not the case here)
2. An if-deffed bit value (the issue here0
Case 2 is pretty rare, so (and I think you agreed elsewhere) avoiding this
is the right solution.
I am thinking of solving case 1 by just switching 32-bit kernels to a
64-bit vma_flags_t :) but still mulling that one!
Moving forwards I think avoiding the pattern is probably the better way,
rather determining whether or not a feature is a thing based on whether a
VMA flag bit is set
quoted
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).
Obviously I don't love the &&, & but that seems a moot point.
We agreed to disagree, didn't we? :)
Yup but the pattern repeats all over the place and in each case you're
checking _both_ VMA_UFFD_BIT _and_ a uffd mode.
So that's open-coded and duplicated, it'd therefore be nice to separate it
out.
E.g.:
static inline bool vma_test_uffd_flags(const struct vm_area_struct *vma,
unsigned int uffd_flags)
{
return vma_test(vma, VMA_UFFD_BIT) &&
(vma_uffd_flags(vma) & uffd_flags);
}
Then this kind of pattern simply becomes:
return vma_test_uffd_flags(vma, UFFD_FLAG_RWP);
Nicer right?
[If I didn't say elsewhere also it'd be nice to rename uffd_mode() to
vma_uffd_flags() or something so the vma_xxx() pattern is kept consistent.]
Shouldn't you update the tracing logic to obtain these from uffd
modes/flags?
Yep, will do.
Thanks!
quoted
quoted
/* * 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?
Because you are testing for something very distinct from every other
instance where you are checking for a 'mode'.
In every other case it's 'is this flag set'. In this case it's 'is this
flag distinctly set'.
Renaming this from mode to flags obviously helps make things clearer.
But given there's a comment there and you're renaming mode -> flags anyway
that should be fine.
quoted
At any rate 'modes' or 'flags' or something would be clearer here.
This is oddly arbitrarily using VMA_UFFD inconsistent from all uses of
VMA_UFFD_BIT.
Can't say I follow you here.
In every other instance of referencing the flag you are using VMA_UFFD_BIT,
here you are arbitrarily using the VMA_UFFD value.
Given we agreed on bit 12 anyway, let's drop VMA_UFFD altogether and use:
vma_flags_set(&new_vma_flags, VMA_UFFD_BIT);
Here please!
Same comments as elsewhere re vma_test() on VMA_UFFD_BIT.
See above.
quoted
quoted
@@ -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.
quoted
Thanks for switching to new VMA flags model though!
Welcome :)
:)
quoted
quoted
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.
Oh something I didn't notice before also:
uffd_mode(cur) & (UFFD_MODE_WP | UFFD_MODE_RWP) & ~mode
Is pretty ambiguous no? Is ~mode applied to the result of (uffd_mode(cur) &
(UFFD_MODE_WP | UFFD_MODE_RWP)) or just to (UFFD_MODE_WP | UFFD_MODE_RWP)?
Probably should be e.g.:
((uffd_mode(cur) & (UFFD_MODE_WP | UFFD_MODE_RWP)) & ~mode)
Or:
uffd_mode(cur) & ((UFFD_MODE_WP | UFFD_MODE_RWP) & ~mode)
I guess it ultimately doesn't matter either way since you're checking for
either flag being set but then that leads on to how confusing this pattern
is:
You're checking for either flag only if those flags were not set in mode?
So in English, it's 'check to see if UFFD_MODE_WP or UFFD_MODE_RWP are
set if and only if those modes were not already set in mode'.
So you have to decode this to 'are the WP flags dropped?'
So I'm sorry that is pretty confusing I would say, especially kept all in
one line!
(I personally confused myself by looking at this, not sure what kind of
barometer that is :)
I disagree that we can't improve it, something like:
static inline bool vma_is_uffd_ctx(const struct vm_area_struct *vma,
const struct userfaultfd_ctx *ctx)
{
return vma->vm_uffd_state.ctx == ctx;
}
...
const unsigned int old_wp_flags =
uffd_mode(cur) & (UFFD_MODE_WP | UFFD_MODE_RWP);
const unsigned int new_wp_flags =
mode & (UFFD_MODE_WP | UFFD_MODE_RWP);
const bool drops_wp_flags = old_wp_flags & ~new_wp_flags;
...
if (vma_is_uffd_ctx(cur, ctx) && drops_wp_flags)
goto out_unlock;
Would be clearer I think right?
quoted
Same objection to the use of the word 'mode'. You really need to say flags
here somehow.
quoted
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:
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
Yup bit 12 resolves.
quoted
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
Explained above why it's challenging. It does suck yes!
quoted
quoted
/*
* 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;
Nit but pretty horrible alignment. Gues it can 't be helped
Would run out of 80 chars :(
Yup not a big deal obviously!
But I'm thinking now to add one more patch to get rid from passing wp_async
to vma_can_userfault().
quoted
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?
Let's drop it altogether if it's not used anywhere thanks!
quoted
Could even be with placeholder flag names potentially.
Like VMA_FLAG_A, VMA_FLAG_B?
quoted
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.
quoted
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.
Yeah it's odd that, but the bits themselves being #ifdef'd is itself a
strange thing, I think in this case was bit stuffing for nommu (ugh).
quoted
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.
quoted
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.
Right, I think we keep coming around to this as being unclear.
I think having those flags that must be distinct define in mm_types.h or
mm.h together with some explanation would resolve this, rather than
slinging state around in different files just for the sake of maintaining
userfaultfd_k.h as a thing.
quoted
IMO before it can move forwards.
--
Cheers, Lorenzo
On Thu, Aug 27, 2026 at 12:16:32PM +0100, Lorenzo Stoakes (ARM) wrote:
So actually you're increasing by a cacheline and increasing the VMA size by
64 bytes, i.e. 1/3, which is unacceptable obviously.
Maybe there's something that can be done with:
/* forced alignments: 1 */
Perhaps? But that looks potentially ugly.
I say elsewhere (or think I do) but to highlight - I think probably we could fix
this by putting the flags in the low bits of vm_uffd_state.ctx?
--
Cheers, Lorenzo
On Thu, Aug 27, 2026 at 10:42:16AM +0300, Mike Rapoport wrote:
On Tue, Aug 25, 2026 at 02:00:10PM +0100, Lorenzo Stoakes (ARM) wrote:
quoted
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.
OK, thanks for that explanation.
I think summing that up in a comment and definitely the commit message would be
useful.
Also potentially gathering all this kind of state and putting it in mm.h or
mm_types.h would be nice too.
I say this elsewhere but I do think userfaultfd_k.h is a bit of a confused
mess and we shouldn't make things more confusing by wanting to keep state
explicitly there.
quoted
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 ;-)
Right, I do think in general if experienced(-ish ;) kernel maintainers find
things confusing, this is _usually_ a signal that things could be made more
clear in the series.
Of course not excluding the possibility that I am simply not bright enough
to figure it out :)
quoted
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 :)
I think you're missing the point, but I go into detail with examples in 6/6
that hopefully clarifies things.
The enum/bit number/keeping equality stuff was just me thinking out loud,
the point here is about abstraction and keeping things clear.
I mean, and give me some rope, by your argument, why have
vma_is_anonymous()? Just check for !vma->vm_ops everywhere right?
Well I'd argue that it is _far_ clearer, self-documents, abstracts the
_means_ by which a VMA is anonymous (no vm_ops) from the semantics of 'is
this VMA anonymous'.
Equally so here.
I actually think it'd not be unreasonable, given how few flags there are to
have e.g.:
vma_handles_uffd_missing()
vma_handles_uffd_minor()
vma_handles_uffd_wp()
vma_handles_uffd_rwp()
Or something like this?
And, as I say in 6/6, you are checking vma_test(vma, VMA_UFFD_BIT) each
time (or perhaps context != NULL? Not sure if equivalent) now you can
abstract that and remove duplication.
And _then_ the weird 'WP but not uffd' case can be self-documented and
called out like:
vma_was_uffd_wp()
Or whatever naming would make sense.
Hopefully that clarifies my point.
quoted
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.
Ack.
quoted
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?
I'm talking about the 'mode' naming, which I think we have reached
agreement upon in 6/6.
quoted
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 :)
See elsewhere.
quoted
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?
unsigned long :) but I think unsigned int is fine.
'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.
Or we could just move flags out of that horrible header :)
I hate how C headers can force us into difficult decisions that make life
harder...
On Thu, Aug 27, 2026 at 12:16:32PM +0100, Lorenzo Stoakes (ARM) wrote:
quoted
So actually you're increasing by a cacheline and increasing the VMA size by
64 bytes, i.e. 1/3, which is unacceptable obviously.
Maybe there's something that can be done with:
/* forced alignments: 1 */
Perhaps? But that looks potentially ugly.
I say elsewhere (or think I do) but to highlight - I think probably we could fix
this by putting the flags in the low bits of vm_uffd_state.ctx?
if that's possible that would be clearly preferable memory-wise.
--
Cheers,
David
From: Mike Rapoport <rppt@kernel.org> Date: 2026-08-29 11:00:17
On Thu, Aug 27, 2026 at 05:19:30PM +0200, David Hildenbrand (Arm) wrote:
On 8/27/26 13:18, Lorenzo Stoakes (ARM) wrote:
quoted
On Thu, Aug 27, 2026 at 12:16:32PM +0100, Lorenzo Stoakes (ARM) wrote:
quoted
So actually you're increasing by a cacheline and increasing the VMA size by
64 bytes, i.e. 1/3, which is unacceptable obviously.
Maybe there's something that can be done with:
/* forced alignments: 1 */
Perhaps? But that looks potentially ugly.
I say elsewhere (or think I do) but to highlight - I think probably we could fix
this by putting the flags in the low bits of vm_uffd_state.ctx?
if that's possible that would be clearly preferable memory-wise.
This gives only 4 bits and makes this completely not extendable.
So I think I'll drop this for now and wait until VMA grows another cache
line or until having per-VMA uffd state rather than a pointer to per-fd
context is a must.
From: Tal Zussman <hidden> Date: 2026-08-29 18:43:00
On 2026-08-29 14:00 +0300, Mike Rapoport wrote:
On Thu, Aug 27, 2026 at 05:19:30PM +0200, David Hildenbrand (Arm) wrote:
quoted
On 8/27/26 13:18, Lorenzo Stoakes (ARM) wrote:
quoted
On Thu, Aug 27, 2026 at 12:16:32PM +0100, Lorenzo Stoakes (ARM) wrote:
quoted
So actually you're increasing by a cacheline and increasing the VMA size by
64 bytes, i.e. 1/3, which is unacceptable obviously.
Maybe there's something that can be done with:
/* forced alignments: 1 */
Perhaps? But that looks potentially ugly.
I say elsewhere (or think I do) but to highlight - I think probably we could fix
this by putting the flags in the low bits of vm_uffd_state.ctx?
if that's possible that would be clearly preferable memory-wise.
This gives only 4 bits and makes this completely not extendable.
Wouldn't it be 6 bits? struct userfaultfd_ctx is allocated with
kmem_cache_create() and SLAB_HWCACHE_ALIGN, and most of the flags are
only available on 64 bit, so it should be 64-byte aligned in the
relevant cases.
(Not that 6 is that much better than 4... but it's a little more wiggle
room.)
It could in theory also be bumped up to 7 by setting align in
kmem_cache_create(). userfaultfd_ctx already takes 192 bytes due to
existing alignment. Aligning it to 128 bytes would make it 256 bytes,
adding 64 bytes to each uffd rather than each VMA. But this sounds like
more pain for little gain :)
So I think I'll drop this for now and wait until VMA grows another cache
line or until having per-VMA uffd state rather than a pointer to per-fd
context is a must.
From: Mike Rapoport <rppt@kernel.org> Date: 2026-08-30 05:36:16
On Sat, Aug 29, 2026 at 02:42:52PM -0400, Tal Zussman wrote:
On 2026-08-29 14:00 +0300, Mike Rapoport wrote:
quoted
On Thu, Aug 27, 2026 at 05:19:30PM +0200, David Hildenbrand (Arm) wrote:
quoted
On 8/27/26 13:18, Lorenzo Stoakes (ARM) wrote:
quoted
On Thu, Aug 27, 2026 at 12:16:32PM +0100, Lorenzo Stoakes (ARM) wrote:
quoted
So actually you're increasing by a cacheline and increasing the VMA size by
64 bytes, i.e. 1/3, which is unacceptable obviously.
Maybe there's something that can be done with:
/* forced alignments: 1 */
Perhaps? But that looks potentially ugly.
I say elsewhere (or think I do) but to highlight - I think probably we could fix
this by putting the flags in the low bits of vm_uffd_state.ctx?
if that's possible that would be clearly preferable memory-wise.
This gives only 4 bits and makes this completely not extendable.
Wouldn't it be 6 bits? struct userfaultfd_ctx is allocated with
kmem_cache_create() and SLAB_HWCACHE_ALIGN, and most of the flags are
only available on 64 bit, so it should be 64-byte aligned in the
relevant cases.
(Not that 6 is that much better than 4... but it's a little more wiggle
room.)
I did remember that SLAB_HWCACHE_ALIGN could be as small as 16 bytes, but I
didn't verify it for architectures that support fancy uffd modes.
It could in theory also be bumped up to 7 by setting align in
kmem_cache_create(). userfaultfd_ctx already takes 192 bytes due to
existing alignment. Aligning it to 128 bytes would make it 256 bytes,
adding 64 bytes to each uffd rather than each VMA. But this sounds like
more pain for little gain :)
Yeah, even with as plenty as 7 bits :)
quoted
So I think I'll drop this for now and wait until VMA grows another cache
line or until having per-VMA uffd state rather than a pointer to per-fd
context is a must.