On 07.06.25 08:40, Tal Zussman wrote:
Currently, a VMA registered with a uffd can be unregistered through a
different uffd associated with the same mm_struct.
The existing behavior is slightly broken and may incorrectly reject
unregistering some VMAs due to the following check:
if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
goto out_unlock;
where wp_async is derived from ctx, not from cur. For example, a file-backed
VMA registered with wp_async enabled and UFFD_WP mode cannot be unregistered
through a uffd that does not have wp_async enabled.
Rather than fix this and maintain this odd behavior, make unregistration
stricter by requiring VMAs to be unregistered through the same uffd they
were registered with. Additionally, reorder the WARN() checks to avoid
the aforementioned wp_async issue in the WARN()s.
This change slightly modifies the ABI. It should not be backported to
-stable.
Probably add that the expectation is that nobody really depends on this
behavior, and that no such cases are known.
While at it, correct the comment for the no userfaultfd case. This seems to
be a copy-paste artifact from the analogous userfaultfd_register() check.
Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
Fixes should come before anything else in a series (Andrew even prefers
a separate series for fixes vs. follow-up cleanups).
Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>
---
fs/userfaultfd.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 80c95c712266..10e8037f5216 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1466,6 +1466,16 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
VM_WARN_ON_ONCE(!!cur->vm_userfaultfd_ctx.ctx ^
!!(cur->vm_flags & __VM_UFFD_FLAGS));
+ /*
+ * Check that this VMA isn't already owned by a different
+ * userfaultfd. This provides for more strict behavior by
+ * preventing a VMA registered with a userfaultfd from being
+ * unregistered through a different userfaultfd.
+ */
Probably we can shorted to:
/*
* Prevent unregistering through another userfaultfd than used for
* registering.
*/
?
+ if (cur->vm_userfaultfd_ctx.ctx &&
+ cur->vm_userfaultfd_ctx.ctx != ctx)
+ goto out_unlock;
+
/*
* Check not compatible vmas, not strictly required
* here as not compatible vmas cannot have an
@@ -1489,15 +1499,14 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
for_each_vma_range(vmi, vma, end) {
cond_resched();
- VM_WARN_ON_ONCE(!vma_can_userfault(vma, vma->vm_flags, wp_async));
-
/*
- * Nothing to do: this vma is already registered into this
- * userfaultfd and with the right tracking mode too.
+ * Nothing to do: this vma is not registered with userfaultfd.
*/
Maybe
/* VMA not registered with userfaultfd. */
The "skip" below is rather clear. :)
if (!vma->vm_userfaultfd_ctx.ctx)
goto skip;
+ VM_WARN_ON_ONCE(vma->vm_userfaultfd_ctx.ctx != ctx);
+ VM_WARN_ON_ONCE(!vma_can_userfault(vma, vma->vm_flags, wp_async));
WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
if (vma->vm_start > start)
Acked-by: David Hildenbrand <david@xxxxxxxxxx>
--
Cheers,
David / dhildenb
