Performing userfaultfd operations (like copy/move etc.) in critical
section of mmap_lock (read-mode) causes significant contention on the
lock when operations requiring the lock in write-mode are taking place
concurrently. We can use per-vma locks instead to significantly reduce
the contention issue.
Android runtime's Garbage Collector uses userfaultfd for concurrent
compaction. mmap-lock contention during compaction potentially causes
jittery experience for the user. During one such reproducible scenario,
we observed the following improvements with this patch-set:
- Wall clock time of compaction phase came down from ~3s to <500ms
- Uninterruptible sleep time (across all threads in the process) was
~10ms (none in mmap_lock) during compaction, instead of >20s
Changes since v3 [3]:
- Rename function names to clearly reflect which lock is being taken,
per Liam Howlett.
- Have separate functions and abstractions in mm/userfaultfd.c to avoid
confusion around which lock is being acquired/released, per Liam Howlett.
- Prepare anon_vma for all private vmas, anonymous or file-backed,
per Jann Horn.
Changes since v2 [2]:
- Implement and use lock_vma() which uses mmap_lock critical section
to lock the VMA using per-vma lock if lock_vma_under_rcu() fails,
per Liam R. Howlett. This helps simplify the code and also avoids
performing the entire userfaultfd operation under mmap_lock.
Changes since v1 [1]:
- rebase patches on 'mm-unstable' branch
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
Lokesh Gidra (3):
userfaultfd: move userfaultfd_ctx struct to header file
userfaultfd: protect mmap_changing with rw_sem in userfaulfd_ctx
userfaultfd: use per-vma locks in userfaultfd operations
fs/userfaultfd.c | 86 ++-----
include/linux/userfaultfd_k.h | 75 ++++--
mm/userfaultfd.c | 414 +++++++++++++++++++++++++---------
3 files changed, 384 insertions(+), 191 deletions(-)
--
2.43.0.687.g38aa6559b0-goog
All userfaultfd operations, except write-protect, opportunistically use
per-vma locks to lock vmas. On failure, attempt again inside mmap_lock
critical section.
Write-protect operation requires mmap_lock as it iterates over multiple
vmas.
Signed-off-by: Lokesh Gidra <[email protected]>
---
fs/userfaultfd.c | 13 +-
include/linux/userfaultfd_k.h | 5 +-
mm/userfaultfd.c | 356 ++++++++++++++++++++++++++--------
3 files changed, 275 insertions(+), 99 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index c00a021bcce4..60dcfafdc11a 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -2005,17 +2005,8 @@ static int userfaultfd_move(struct userfaultfd_ctx *ctx,
return -EINVAL;
if (mmget_not_zero(mm)) {
- mmap_read_lock(mm);
-
- /* Re-check after taking map_changing_lock */
- down_read(&ctx->map_changing_lock);
- if (likely(!atomic_read(&ctx->mmap_changing)))
- ret = move_pages(ctx, mm, uffdio_move.dst, uffdio_move.src,
- uffdio_move.len, uffdio_move.mode);
- else
- ret = -EAGAIN;
- up_read(&ctx->map_changing_lock);
- mmap_read_unlock(mm);
+ ret = move_pages(ctx, uffdio_move.dst, uffdio_move.src,
+ uffdio_move.len, uffdio_move.mode);
mmput(mm);
} else {
return -ESRCH;
diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
index 3210c3552976..05d59f74fc88 100644
--- a/include/linux/userfaultfd_k.h
+++ b/include/linux/userfaultfd_k.h
@@ -138,9 +138,8 @@ extern long uffd_wp_range(struct vm_area_struct *vma,
/* move_pages */
void double_pt_lock(spinlock_t *ptl1, spinlock_t *ptl2);
void double_pt_unlock(spinlock_t *ptl1, spinlock_t *ptl2);
-ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
- unsigned long dst_start, unsigned long src_start,
- unsigned long len, __u64 flags);
+ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
+ unsigned long src_start, unsigned long len, __u64 flags);
int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pmd_t dst_pmdval,
struct vm_area_struct *dst_vma,
struct vm_area_struct *src_vma,
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 74aad0831e40..1e25768b2136 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -19,20 +19,12 @@
#include <asm/tlb.h>
#include "internal.h"
-static __always_inline
-struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm,
- unsigned long dst_start,
- unsigned long len)
+static bool validate_dst_vma(struct vm_area_struct *dst_vma,
+ unsigned long dst_end)
{
- /*
- * Make sure that the dst range is both valid and fully within a
- * single existing vma.
- */
- struct vm_area_struct *dst_vma;
-
- dst_vma = find_vma(dst_mm, dst_start);
- if (!range_in_vma(dst_vma, dst_start, dst_start + len))
- return NULL;
+ /* Make sure that the dst range is fully within dst_vma. */
+ if (dst_end > dst_vma->vm_end)
+ return false;
/*
* Check the vma is registered in uffd, this is required to
@@ -40,11 +32,125 @@ struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm,
* time.
*/
if (!dst_vma->vm_userfaultfd_ctx.ctx)
- return NULL;
+ return false;
+
+ return true;
+}
+
+#ifdef CONFIG_PER_VMA_LOCK
+/*
+ * lock_vma() - Lookup and lock vma corresponding to @address.
+ * @mm: mm to search vma in.
+ * @address: address that the vma should contain.
+ * @prepare_anon: If true, then prepare the vma (if private) with anon_vma.
+ *
+ * Should be called without holding mmap_lock. vma should be unlocked after use
+ * with unlock_vma().
+ *
+ * Return: A locked vma containing @address, NULL if no vma is found, or
+ * -ENOMEM if anon_vma couldn't be allocated.
+ */
+static struct vm_area_struct *lock_vma(struct mm_struct *mm,
+ unsigned long address,
+ bool prepare_anon)
+{
+ struct vm_area_struct *vma;
+
+ vma = lock_vma_under_rcu(mm, address);
+ if (vma) {
+ /*
+ * lock_vma_under_rcu() only checks anon_vma for private
+ * anonymous mappings. But we need to ensure it is assigned in
+ * private file-backed vmas as well.
+ */
+ if (prepare_anon && !(vma->vm_flags & VM_SHARED) &&
+ !vma->anon_vma)
+ vma_end_read(vma);
+ else
+ return vma;
+ }
+
+ mmap_read_lock(mm);
+ vma = vma_lookup(mm, address);
+ if (vma) {
+ if (prepare_anon && !(vma->vm_flags & VM_SHARED) &&
+ anon_vma_prepare(vma)) {
+ vma = ERR_PTR(-ENOMEM);
+ } else {
+ /*
+ * We cannot use vma_start_read() as it may fail due to
+ * false locked (see comment in vma_start_read()). We
+ * can avoid that by directly locking vm_lock under
+ * mmap_lock, which guarantees that nobody can lock the
+ * vma for write (vma_start_write()) under us.
+ */
+ down_read(&vma->vm_lock->lock);
+ }
+ }
+
+ mmap_read_unlock(mm);
+ return vma;
+}
+
+static void unlock_vma(struct vm_area_struct *vma)
+{
+ vma_end_read(vma);
+}
+
+static struct vm_area_struct *find_and_lock_dst_vma(struct mm_struct *dst_mm,
+ unsigned long dst_start,
+ unsigned long len)
+{
+ struct vm_area_struct *dst_vma;
+
+ /* Ensure anon_vma is assigned for private vmas */
+ dst_vma = lock_vma(dst_mm, dst_start, true);
+
+ if (!dst_vma)
+ return ERR_PTR(-ENOENT);
+
+ if (PTR_ERR(dst_vma) == -ENOMEM)
+ return dst_vma;
+
+ if (!validate_dst_vma(dst_vma, dst_start + len))
+ goto out_unlock;
return dst_vma;
+out_unlock:
+ unlock_vma(dst_vma);
+ return ERR_PTR(-ENOENT);
}
+#else
+
+static struct vm_area_struct *lock_mm_and_find_dst_vma(struct mm_struct *dst_mm,
+ unsigned long dst_start,
+ unsigned long len)
+{
+ struct vm_area_struct *dst_vma;
+ int err = -ENOENT;
+
+ mmap_read_lock(dst_mm);
+ dst_vma = vma_lookup(dst_mm, dst_start);
+ if (!dst_vma)
+ goto out_unlock;
+
+ /* Ensure anon_vma is assigned for private vmas */
+ if (!(dst_vma->vm_flags & VM_SHARED) && anon_vma_prepare(dst_vma)) {
+ err = -ENOMEM;
+ goto out_unlock;
+ }
+
+ if (!validate_dst_vma(dst_vma, dst_start + len))
+ goto out_unlock;
+
+ return dst_vma;
+out_unlock:
+ mmap_read_unlock(dst_mm);
+ return ERR_PTR(err);
+}
+#endif
+
/* Check if dst_addr is outside of file's size. Must be called with ptl held. */
static bool mfill_file_over_size(struct vm_area_struct *dst_vma,
unsigned long dst_addr)
@@ -350,7 +456,8 @@ static pmd_t *mm_alloc_pmd(struct mm_struct *mm, unsigned long address)
#ifdef CONFIG_HUGETLB_PAGE
/*
* mfill_atomic processing for HUGETLB vmas. Note that this routine is
- * called with mmap_lock held, it will release mmap_lock before returning.
+ * called with either vma-lock or mmap_lock held, it will release the lock
+ * before returning.
*/
static __always_inline ssize_t mfill_atomic_hugetlb(
struct userfaultfd_ctx *ctx,
@@ -361,7 +468,6 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
uffd_flags_t flags)
{
struct mm_struct *dst_mm = dst_vma->vm_mm;
- int vm_shared = dst_vma->vm_flags & VM_SHARED;
ssize_t err;
pte_t *dst_pte;
unsigned long src_addr, dst_addr;
@@ -380,7 +486,11 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
*/
if (uffd_flags_mode_is(flags, MFILL_ATOMIC_ZEROPAGE)) {
up_read(&ctx->map_changing_lock);
+#ifdef CONFIG_PER_VMA_LOCK
+ unlock_vma(dst_vma);
+#else
mmap_read_unlock(dst_mm);
+#endif
return -EINVAL;
}
@@ -403,24 +513,32 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
* retry, dst_vma will be set to NULL and we must lookup again.
*/
if (!dst_vma) {
+#ifdef CONFIG_PER_VMA_LOCK
+ dst_vma = find_and_lock_dst_vma(dst_mm, dst_start, len);
+#else
+ dst_vma = lock_mm_and_find_dst_vma(dst_mm, dst_start, len);
+#endif
+ if (IS_ERR(dst_vma)) {
+ err = PTR_ERR(dst_vma);
+ goto out;
+ }
+
err = -ENOENT;
- dst_vma = find_dst_vma(dst_mm, dst_start, len);
- if (!dst_vma || !is_vm_hugetlb_page(dst_vma))
- goto out_unlock;
+ if (!is_vm_hugetlb_page(dst_vma))
+ goto out_unlock_vma;
err = -EINVAL;
if (vma_hpagesize != vma_kernel_pagesize(dst_vma))
- goto out_unlock;
-
- vm_shared = dst_vma->vm_flags & VM_SHARED;
- }
+ goto out_unlock_vma;
- /*
- * If not shared, ensure the dst_vma has a anon_vma.
- */
- err = -ENOMEM;
- if (!vm_shared) {
- if (unlikely(anon_vma_prepare(dst_vma)))
+ /*
+ * If memory mappings are changing because of non-cooperative
+ * operation (e.g. mremap) running in parallel, bail out and
+ * request the user to retry later
+ */
+ down_read(&ctx->map_changing_lock);
+ err = -EAGAIN;
+ if (atomic_read(&ctx->mmap_changing))
goto out_unlock;
}
@@ -465,7 +583,11 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
if (unlikely(err == -ENOENT)) {
up_read(&ctx->map_changing_lock);
+#ifdef CONFIG_PER_VMA_LOCK
+ unlock_vma(dst_vma);
+#else
mmap_read_unlock(dst_mm);
+#endif
BUG_ON(!folio);
err = copy_folio_from_user(folio,
@@ -474,17 +596,6 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
err = -EFAULT;
goto out;
}
- mmap_read_lock(dst_mm);
- down_read(&ctx->map_changing_lock);
- /*
- * If memory mappings are changing because of non-cooperative
- * operation (e.g. mremap) running in parallel, bail out and
- * request the user to retry later
- */
- if (atomic_read(&ctx->mmap_changing)) {
- err = -EAGAIN;
- break;
- }
dst_vma = NULL;
goto retry;
@@ -505,7 +616,12 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
out_unlock:
up_read(&ctx->map_changing_lock);
+out_unlock_vma:
+#ifdef CONFIG_PER_VMA_LOCK
+ unlock_vma(dst_vma);
+#else
mmap_read_unlock(dst_mm);
+#endif
out:
if (folio)
folio_put(folio);
@@ -597,7 +713,19 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
copied = 0;
folio = NULL;
retry:
- mmap_read_lock(dst_mm);
+ /*
+ * Make sure the vma is not shared, that the dst range is
+ * both valid and fully within a single existing vma.
+ */
+#ifdef CONFIG_PER_VMA_LOCK
+ dst_vma = find_and_lock_dst_vma(dst_mm, dst_start, len);
+#else
+ dst_vma = lock_mm_and_find_dst_vma(dst_mm, dst_start, len);
+#endif
+ if (IS_ERR(dst_vma)) {
+ err = PTR_ERR(dst_vma);
+ goto out;
+ }
/*
* If memory mappings are changing because of non-cooperative
@@ -609,15 +737,6 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
if (atomic_read(&ctx->mmap_changing))
goto out_unlock;
- /*
- * Make sure the vma is not shared, that the dst range is
- * both valid and fully within a single existing vma.
- */
- err = -ENOENT;
- dst_vma = find_dst_vma(dst_mm, dst_start, len);
- if (!dst_vma)
- goto out_unlock;
-
err = -EINVAL;
/*
* shmem_zero_setup is invoked in mmap for MAP_ANONYMOUS|MAP_SHARED but
@@ -647,16 +766,6 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
uffd_flags_mode_is(flags, MFILL_ATOMIC_CONTINUE))
goto out_unlock;
- /*
- * Ensure the dst_vma has a anon_vma or this page
- * would get a NULL anon_vma when moved in the
- * dst_vma.
- */
- err = -ENOMEM;
- if (!(dst_vma->vm_flags & VM_SHARED) &&
- unlikely(anon_vma_prepare(dst_vma)))
- goto out_unlock;
-
while (src_addr < src_start + len) {
pmd_t dst_pmdval;
@@ -699,7 +808,11 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
void *kaddr;
up_read(&ctx->map_changing_lock);
+#ifdef CONFIG_PER_VMA_LOCK
+ unlock_vma(dst_vma);
+#else
mmap_read_unlock(dst_mm);
+#endif
BUG_ON(!folio);
kaddr = kmap_local_folio(folio, 0);
@@ -730,7 +843,11 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
out_unlock:
up_read(&ctx->map_changing_lock);
+#ifdef CONFIG_PER_VMA_LOCK
+ unlock_vma(dst_vma);
+#else
mmap_read_unlock(dst_mm);
+#endif
out:
if (folio)
folio_put(folio);
@@ -1267,16 +1384,67 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,
if (!vma_is_anonymous(src_vma) || !vma_is_anonymous(dst_vma))
return -EINVAL;
- /*
- * Ensure the dst_vma has a anon_vma or this page
- * would get a NULL anon_vma when moved in the
- * dst_vma.
- */
- if (unlikely(anon_vma_prepare(dst_vma)))
- return -ENOMEM;
+ return 0;
+}
+
+#ifdef CONFIG_PER_VMA_LOCK
+static int find_and_lock_vmas(struct mm_struct *mm,
+ unsigned long dst_start,
+ unsigned long src_start,
+ struct vm_area_struct **dst_vmap,
+ struct vm_area_struct **src_vmap)
+{
+ int err;
+
+ /* There is no need to prepare anon_vma for src_vma */
+ *src_vmap = lock_vma(mm, src_start, false);
+ if (!*src_vmap)
+ return -ENOENT;
+
+ /* Ensure anon_vma is assigned for anonymous vma */
+ *dst_vmap = lock_vma(mm, dst_start, true);
+ err = -ENOENT;
+ if (!*dst_vmap)
+ goto out_unlock;
+
+ err = -ENOMEM;
+ if (PTR_ERR(*dst_vmap) == -ENOMEM)
+ goto out_unlock;
return 0;
+out_unlock:
+ unlock_vma(*src_vmap);
+ return err;
}
+#else
+static int lock_mm_and_find_vmas(struct mm_struct *mm,
+ unsigned long dst_start,
+ unsigned long src_start,
+ struct vm_area_struct **dst_vmap,
+ struct vm_area_struct **src_vmap)
+{
+ int err = -ENOENT;
+ mmap_read_lock(mm);
+
+ *src_vmap = vma_lookup(mm, src_start);
+ if (!*src_vmap)
+ goto out_unlock;
+
+ *dst_vmap = vma_lookup(mm, dst_start);
+ if (!*dst_vmap)
+ goto out_unlock;
+
+ /* Ensure anon_vma is assigned */
+ err = -ENOMEM;
+ if (vma_is_anonymous(*dst_vmap) && anon_vma_prepare(*dst_vmap))
+ goto out_unlock;
+
+ return 0;
+out_unlock:
+ mmap_read_unlock(mm);
+ return err;
+}
+#endif
/**
* move_pages - move arbitrary anonymous pages of an existing vma
@@ -1287,8 +1455,6 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,
* @len: length of the virtual memory range
* @mode: flags from uffdio_move.mode
*
- * Must be called with mmap_lock held for read.
- *
* move_pages() remaps arbitrary anonymous pages atomically in zero
* copy. It only works on non shared anonymous pages because those can
* be relocated without generating non linear anon_vmas in the rmap
@@ -1355,10 +1521,10 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,
* could be obtained. This is the only additional complexity added to
* the rmap code to provide this anonymous page remapping functionality.
*/
-ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
- unsigned long dst_start, unsigned long src_start,
- unsigned long len, __u64 mode)
+ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
+ unsigned long src_start, unsigned long len, __u64 mode)
{
+ struct mm_struct *mm = ctx->mm;
struct vm_area_struct *src_vma, *dst_vma;
unsigned long src_addr, dst_addr;
pmd_t *src_pmd, *dst_pmd;
@@ -1376,28 +1542,40 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
WARN_ON_ONCE(dst_start + len <= dst_start))
goto out;
+#ifdef CONFIG_PER_VMA_LOCK
+ err = find_and_lock_vmas(mm, dst_start, src_start,
+ &dst_vma, &src_vma);
+#else
+ err = lock_mm_and_find_vmas(mm, dst_start, src_start,
+ &dst_vma, &src_vma);
+#endif
+ if (err)
+ goto out;
+
+ /* Re-check after taking map_changing_lock */
+ down_read(&ctx->map_changing_lock);
+ if (likely(atomic_read(&ctx->mmap_changing))) {
+ err = -EAGAIN;
+ goto out_unlock;
+ }
/*
* Make sure the vma is not shared, that the src and dst remap
* ranges are both valid and fully within a single existing
* vma.
*/
- src_vma = find_vma(mm, src_start);
- if (!src_vma || (src_vma->vm_flags & VM_SHARED))
- goto out;
- if (src_start < src_vma->vm_start ||
- src_start + len > src_vma->vm_end)
- goto out;
+ if (src_vma->vm_flags & VM_SHARED)
+ goto out_unlock;
+ if (src_start + len > src_vma->vm_end)
+ goto out_unlock;
- dst_vma = find_vma(mm, dst_start);
- if (!dst_vma || (dst_vma->vm_flags & VM_SHARED))
- goto out;
- if (dst_start < dst_vma->vm_start ||
- dst_start + len > dst_vma->vm_end)
- goto out;
+ if (dst_vma->vm_flags & VM_SHARED)
+ goto out_unlock;
+ if (dst_start + len > dst_vma->vm_end)
+ goto out_unlock;
err = validate_move_areas(ctx, src_vma, dst_vma);
if (err)
- goto out;
+ goto out_unlock;
for (src_addr = src_start, dst_addr = dst_start;
src_addr < src_start + len;) {
@@ -1514,6 +1692,14 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
moved += step_size;
}
+out_unlock:
+ up_read(&ctx->map_changing_lock);
+#ifdef CONFIG_PER_VMA_LOCK
+ unlock_vma(dst_vma);
+ unlock_vma(src_vma);
+#else
+ mmap_read_unlock(mm);
+#endif
out:
VM_WARN_ON(moved < 0);
VM_WARN_ON(err > 0);
--
2.43.0.687.g38aa6559b0-goog
Moving the struct to userfaultfd_k.h to be accessible from
mm/userfaultfd.c. There are no other changes in the struct.
This is required to prepare for using per-vma locks in userfaultfd
operations.
Signed-off-by: Lokesh Gidra <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
---
fs/userfaultfd.c | 39 -----------------------------------
include/linux/userfaultfd_k.h | 39 +++++++++++++++++++++++++++++++++++
2 files changed, 39 insertions(+), 39 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 05c8e8a05427..58331b83d648 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -50,45 +50,6 @@ static struct ctl_table vm_userfaultfd_table[] = {
static struct kmem_cache *userfaultfd_ctx_cachep __ro_after_init;
-/*
- * Start with fault_pending_wqh and fault_wqh so they're more likely
- * to be in the same cacheline.
- *
- * Locking order:
- * fd_wqh.lock
- * fault_pending_wqh.lock
- * fault_wqh.lock
- * event_wqh.lock
- *
- * To avoid deadlocks, IRQs must be disabled when taking any of the above locks,
- * since fd_wqh.lock is taken by aio_poll() while it's holding a lock that's
- * also taken in IRQ context.
- */
-struct userfaultfd_ctx {
- /* waitqueue head for the pending (i.e. not read) userfaults */
- wait_queue_head_t fault_pending_wqh;
- /* waitqueue head for the userfaults */
- wait_queue_head_t fault_wqh;
- /* waitqueue head for the pseudo fd to wakeup poll/read */
- wait_queue_head_t fd_wqh;
- /* waitqueue head for events */
- wait_queue_head_t event_wqh;
- /* a refile sequence protected by fault_pending_wqh lock */
- seqcount_spinlock_t refile_seq;
- /* pseudo fd refcounting */
- refcount_t refcount;
- /* userfaultfd syscall flags */
- unsigned int flags;
- /* features requested from the userspace */
- unsigned int features;
- /* released */
- bool released;
- /* memory mappings are changing because of non-cooperative event */
- atomic_t mmap_changing;
- /* mm with one ore more vmas attached to this userfaultfd_ctx */
- struct mm_struct *mm;
-};
-
struct userfaultfd_fork_ctx {
struct userfaultfd_ctx *orig;
struct userfaultfd_ctx *new;
diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
index e4056547fbe6..691d928ee864 100644
--- a/include/linux/userfaultfd_k.h
+++ b/include/linux/userfaultfd_k.h
@@ -36,6 +36,45 @@
#define UFFD_SHARED_FCNTL_FLAGS (O_CLOEXEC | O_NONBLOCK)
#define UFFD_FLAGS_SET (EFD_SHARED_FCNTL_FLAGS)
+/*
+ * Start with fault_pending_wqh and fault_wqh so they're more likely
+ * to be in the same cacheline.
+ *
+ * Locking order:
+ * fd_wqh.lock
+ * fault_pending_wqh.lock
+ * fault_wqh.lock
+ * event_wqh.lock
+ *
+ * To avoid deadlocks, IRQs must be disabled when taking any of the above locks,
+ * since fd_wqh.lock is taken by aio_poll() while it's holding a lock that's
+ * also taken in IRQ context.
+ */
+struct userfaultfd_ctx {
+ /* waitqueue head for the pending (i.e. not read) userfaults */
+ wait_queue_head_t fault_pending_wqh;
+ /* waitqueue head for the userfaults */
+ wait_queue_head_t fault_wqh;
+ /* waitqueue head for the pseudo fd to wakeup poll/read */
+ wait_queue_head_t fd_wqh;
+ /* waitqueue head for events */
+ wait_queue_head_t event_wqh;
+ /* a refile sequence protected by fault_pending_wqh lock */
+ seqcount_spinlock_t refile_seq;
+ /* pseudo fd refcounting */
+ refcount_t refcount;
+ /* userfaultfd syscall flags */
+ unsigned int flags;
+ /* features requested from the userspace */
+ unsigned int features;
+ /* released */
+ bool released;
+ /* memory mappings are changing because of non-cooperative event */
+ atomic_t mmap_changing;
+ /* mm with one ore more vmas attached to this userfaultfd_ctx */
+ struct mm_struct *mm;
+};
+
extern vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason);
/* A combined operation mode + behavior flags. */
--
2.43.0.687.g38aa6559b0-goog
Increments and loads to mmap_changing are always in mmap_lock
critical section. This ensures that if userspace requests event
notification for non-cooperative operations (e.g. mremap), userfaultfd
operations don't occur concurrently.
This can be achieved by using a separate read-write semaphore in
userfaultfd_ctx such that increments are done in write-mode and loads
in read-mode, thereby eliminating the dependency on mmap_lock for this
purpose.
This is a preparatory step before we replace mmap_lock usage with
per-vma locks in fill/move ioctls.
Signed-off-by: Lokesh Gidra <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
---
fs/userfaultfd.c | 40 ++++++++++++----------
include/linux/userfaultfd_k.h | 31 ++++++++++--------
mm/userfaultfd.c | 62 ++++++++++++++++++++---------------
3 files changed, 75 insertions(+), 58 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 58331b83d648..c00a021bcce4 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -685,12 +685,15 @@ int dup_userfaultfd(struct vm_area_struct *vma, struct list_head *fcs)
ctx->flags = octx->flags;
ctx->features = octx->features;
ctx->released = false;
+ init_rwsem(&ctx->map_changing_lock);
atomic_set(&ctx->mmap_changing, 0);
ctx->mm = vma->vm_mm;
mmgrab(ctx->mm);
userfaultfd_ctx_get(octx);
+ down_write(&octx->map_changing_lock);
atomic_inc(&octx->mmap_changing);
+ up_write(&octx->map_changing_lock);
fctx->orig = octx;
fctx->new = ctx;
list_add_tail(&fctx->list, fcs);
@@ -737,7 +740,9 @@ void mremap_userfaultfd_prep(struct vm_area_struct *vma,
if (ctx->features & UFFD_FEATURE_EVENT_REMAP) {
vm_ctx->ctx = ctx;
userfaultfd_ctx_get(ctx);
+ down_write(&ctx->map_changing_lock);
atomic_inc(&ctx->mmap_changing);
+ up_write(&ctx->map_changing_lock);
} else {
/* Drop uffd context if remap feature not enabled */
vma_start_write(vma);
@@ -783,7 +788,9 @@ bool userfaultfd_remove(struct vm_area_struct *vma,
return true;
userfaultfd_ctx_get(ctx);
+ down_write(&ctx->map_changing_lock);
atomic_inc(&ctx->mmap_changing);
+ up_write(&ctx->map_changing_lock);
mmap_read_unlock(mm);
msg_init(&ewq.msg);
@@ -825,7 +832,9 @@ int userfaultfd_unmap_prep(struct vm_area_struct *vma, unsigned long start,
return -ENOMEM;
userfaultfd_ctx_get(ctx);
+ down_write(&ctx->map_changing_lock);
atomic_inc(&ctx->mmap_changing);
+ up_write(&ctx->map_changing_lock);
unmap_ctx->ctx = ctx;
unmap_ctx->start = start;
unmap_ctx->end = end;
@@ -1709,9 +1718,8 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
if (uffdio_copy.mode & UFFDIO_COPY_MODE_WP)
flags |= MFILL_ATOMIC_WP;
if (mmget_not_zero(ctx->mm)) {
- ret = mfill_atomic_copy(ctx->mm, uffdio_copy.dst, uffdio_copy.src,
- uffdio_copy.len, &ctx->mmap_changing,
- flags);
+ ret = mfill_atomic_copy(ctx, uffdio_copy.dst, uffdio_copy.src,
+ uffdio_copy.len, flags);
mmput(ctx->mm);
} else {
return -ESRCH;
@@ -1761,9 +1769,8 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx,
goto out;
if (mmget_not_zero(ctx->mm)) {
- ret = mfill_atomic_zeropage(ctx->mm, uffdio_zeropage.range.start,
- uffdio_zeropage.range.len,
- &ctx->mmap_changing);
+ ret = mfill_atomic_zeropage(ctx, uffdio_zeropage.range.start,
+ uffdio_zeropage.range.len);
mmput(ctx->mm);
} else {
return -ESRCH;
@@ -1818,9 +1825,8 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
return -EINVAL;
if (mmget_not_zero(ctx->mm)) {
- ret = mwriteprotect_range(ctx->mm, uffdio_wp.range.start,
- uffdio_wp.range.len, mode_wp,
- &ctx->mmap_changing);
+ ret = mwriteprotect_range(ctx, uffdio_wp.range.start,
+ uffdio_wp.range.len, mode_wp);
mmput(ctx->mm);
} else {
return -ESRCH;
@@ -1870,9 +1876,8 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg)
flags |= MFILL_ATOMIC_WP;
if (mmget_not_zero(ctx->mm)) {
- ret = mfill_atomic_continue(ctx->mm, uffdio_continue.range.start,
- uffdio_continue.range.len,
- &ctx->mmap_changing, flags);
+ ret = mfill_atomic_continue(ctx, uffdio_continue.range.start,
+ uffdio_continue.range.len, flags);
mmput(ctx->mm);
} else {
return -ESRCH;
@@ -1925,9 +1930,8 @@ static inline int userfaultfd_poison(struct userfaultfd_ctx *ctx, unsigned long
goto out;
if (mmget_not_zero(ctx->mm)) {
- ret = mfill_atomic_poison(ctx->mm, uffdio_poison.range.start,
- uffdio_poison.range.len,
- &ctx->mmap_changing, 0);
+ ret = mfill_atomic_poison(ctx, uffdio_poison.range.start,
+ uffdio_poison.range.len, 0);
mmput(ctx->mm);
} else {
return -ESRCH;
@@ -2003,13 +2007,14 @@ static int userfaultfd_move(struct userfaultfd_ctx *ctx,
if (mmget_not_zero(mm)) {
mmap_read_lock(mm);
- /* Re-check after taking mmap_lock */
+ /* Re-check after taking map_changing_lock */
+ down_read(&ctx->map_changing_lock);
if (likely(!atomic_read(&ctx->mmap_changing)))
ret = move_pages(ctx, mm, uffdio_move.dst, uffdio_move.src,
uffdio_move.len, uffdio_move.mode);
else
ret = -EAGAIN;
-
+ up_read(&ctx->map_changing_lock);
mmap_read_unlock(mm);
mmput(mm);
} else {
@@ -2216,6 +2221,7 @@ static int new_userfaultfd(int flags)
ctx->flags = flags;
ctx->features = 0;
ctx->released = false;
+ init_rwsem(&ctx->map_changing_lock);
atomic_set(&ctx->mmap_changing, 0);
ctx->mm = current->mm;
/* prevent the mm struct to be freed */
diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
index 691d928ee864..3210c3552976 100644
--- a/include/linux/userfaultfd_k.h
+++ b/include/linux/userfaultfd_k.h
@@ -69,6 +69,13 @@ struct userfaultfd_ctx {
unsigned int features;
/* released */
bool released;
+ /*
+ * Prevents userfaultfd operations (fill/move/wp) from happening while
+ * some non-cooperative event(s) is taking place. Increments are done
+ * in write-mode. Whereas, userfaultfd operations, which includes
+ * reading mmap_changing, is done under read-mode.
+ */
+ struct rw_semaphore map_changing_lock;
/* memory mappings are changing because of non-cooperative event */
atomic_t mmap_changing;
/* mm with one ore more vmas attached to this userfaultfd_ctx */
@@ -113,22 +120,18 @@ extern int mfill_atomic_install_pte(pmd_t *dst_pmd,
unsigned long dst_addr, struct page *page,
bool newly_allocated, uffd_flags_t flags);
-extern ssize_t mfill_atomic_copy(struct mm_struct *dst_mm, unsigned long dst_start,
+extern ssize_t mfill_atomic_copy(struct userfaultfd_ctx *ctx, unsigned long dst_start,
unsigned long src_start, unsigned long len,
- atomic_t *mmap_changing, uffd_flags_t flags);
-extern ssize_t mfill_atomic_zeropage(struct mm_struct *dst_mm,
+ uffd_flags_t flags);
+extern ssize_t mfill_atomic_zeropage(struct userfaultfd_ctx *ctx,
unsigned long dst_start,
- unsigned long len,
- atomic_t *mmap_changing);
-extern ssize_t mfill_atomic_continue(struct mm_struct *dst_mm, unsigned long dst_start,
- unsigned long len, atomic_t *mmap_changing,
- uffd_flags_t flags);
-extern ssize_t mfill_atomic_poison(struct mm_struct *dst_mm, unsigned long start,
- unsigned long len, atomic_t *mmap_changing,
- uffd_flags_t flags);
-extern int mwriteprotect_range(struct mm_struct *dst_mm,
- unsigned long start, unsigned long len,
- bool enable_wp, atomic_t *mmap_changing);
+ unsigned long len);
+extern ssize_t mfill_atomic_continue(struct userfaultfd_ctx *ctx, unsigned long dst_start,
+ unsigned long len, uffd_flags_t flags);
+extern ssize_t mfill_atomic_poison(struct userfaultfd_ctx *ctx, unsigned long start,
+ unsigned long len, uffd_flags_t flags);
+extern int mwriteprotect_range(struct userfaultfd_ctx *ctx, unsigned long start,
+ unsigned long len, bool enable_wp);
extern long uffd_wp_range(struct vm_area_struct *vma,
unsigned long start, unsigned long len, bool enable_wp);
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 9cc93cc1330b..74aad0831e40 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -353,11 +353,11 @@ static pmd_t *mm_alloc_pmd(struct mm_struct *mm, unsigned long address)
* called with mmap_lock held, it will release mmap_lock before returning.
*/
static __always_inline ssize_t mfill_atomic_hugetlb(
+ struct userfaultfd_ctx *ctx,
struct vm_area_struct *dst_vma,
unsigned long dst_start,
unsigned long src_start,
unsigned long len,
- atomic_t *mmap_changing,
uffd_flags_t flags)
{
struct mm_struct *dst_mm = dst_vma->vm_mm;
@@ -379,6 +379,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
* feature is not supported.
*/
if (uffd_flags_mode_is(flags, MFILL_ATOMIC_ZEROPAGE)) {
+ up_read(&ctx->map_changing_lock);
mmap_read_unlock(dst_mm);
return -EINVAL;
}
@@ -463,6 +464,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
cond_resched();
if (unlikely(err == -ENOENT)) {
+ up_read(&ctx->map_changing_lock);
mmap_read_unlock(dst_mm);
BUG_ON(!folio);
@@ -473,12 +475,13 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
goto out;
}
mmap_read_lock(dst_mm);
+ down_read(&ctx->map_changing_lock);
/*
* If memory mappings are changing because of non-cooperative
* operation (e.g. mremap) running in parallel, bail out and
* request the user to retry later
*/
- if (mmap_changing && atomic_read(mmap_changing)) {
+ if (atomic_read(&ctx->mmap_changing)) {
err = -EAGAIN;
break;
}
@@ -501,6 +504,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
}
out_unlock:
+ up_read(&ctx->map_changing_lock);
mmap_read_unlock(dst_mm);
out:
if (folio)
@@ -512,11 +516,11 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
}
#else /* !CONFIG_HUGETLB_PAGE */
/* fail at build time if gcc attempts to use this */
-extern ssize_t mfill_atomic_hugetlb(struct vm_area_struct *dst_vma,
+extern ssize_t mfill_atomic_hugetlb(struct userfaultfd_ctx *ctx,
+ struct vm_area_struct *dst_vma,
unsigned long dst_start,
unsigned long src_start,
unsigned long len,
- atomic_t *mmap_changing,
uffd_flags_t flags);
#endif /* CONFIG_HUGETLB_PAGE */
@@ -564,13 +568,13 @@ static __always_inline ssize_t mfill_atomic_pte(pmd_t *dst_pmd,
return err;
}
-static __always_inline ssize_t mfill_atomic(struct mm_struct *dst_mm,
+static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
unsigned long dst_start,
unsigned long src_start,
unsigned long len,
- atomic_t *mmap_changing,
uffd_flags_t flags)
{
+ struct mm_struct *dst_mm = ctx->mm;
struct vm_area_struct *dst_vma;
ssize_t err;
pmd_t *dst_pmd;
@@ -600,8 +604,9 @@ static __always_inline ssize_t mfill_atomic(struct mm_struct *dst_mm,
* operation (e.g. mremap) running in parallel, bail out and
* request the user to retry later
*/
+ down_read(&ctx->map_changing_lock);
err = -EAGAIN;
- if (mmap_changing && atomic_read(mmap_changing))
+ if (atomic_read(&ctx->mmap_changing))
goto out_unlock;
/*
@@ -633,8 +638,8 @@ static __always_inline ssize_t mfill_atomic(struct mm_struct *dst_mm,
* If this is a HUGETLB vma, pass off to appropriate routine
*/
if (is_vm_hugetlb_page(dst_vma))
- return mfill_atomic_hugetlb(dst_vma, dst_start, src_start,
- len, mmap_changing, flags);
+ return mfill_atomic_hugetlb(ctx, dst_vma, dst_start,
+ src_start, len, flags);
if (!vma_is_anonymous(dst_vma) && !vma_is_shmem(dst_vma))
goto out_unlock;
@@ -693,6 +698,7 @@ static __always_inline ssize_t mfill_atomic(struct mm_struct *dst_mm,
if (unlikely(err == -ENOENT)) {
void *kaddr;
+ up_read(&ctx->map_changing_lock);
mmap_read_unlock(dst_mm);
BUG_ON(!folio);
@@ -723,6 +729,7 @@ static __always_inline ssize_t mfill_atomic(struct mm_struct *dst_mm,
}
out_unlock:
+ up_read(&ctx->map_changing_lock);
mmap_read_unlock(dst_mm);
out:
if (folio)
@@ -733,34 +740,33 @@ static __always_inline ssize_t mfill_atomic(struct mm_struct *dst_mm,
return copied ? copied : err;
}
-ssize_t mfill_atomic_copy(struct mm_struct *dst_mm, unsigned long dst_start,
+ssize_t mfill_atomic_copy(struct userfaultfd_ctx *ctx, unsigned long dst_start,
unsigned long src_start, unsigned long len,
- atomic_t *mmap_changing, uffd_flags_t flags)
+ uffd_flags_t flags)
{
- return mfill_atomic(dst_mm, dst_start, src_start, len, mmap_changing,
+ return mfill_atomic(ctx, dst_start, src_start, len,
uffd_flags_set_mode(flags, MFILL_ATOMIC_COPY));
}
-ssize_t mfill_atomic_zeropage(struct mm_struct *dst_mm, unsigned long start,
- unsigned long len, atomic_t *mmap_changing)
+ssize_t mfill_atomic_zeropage(struct userfaultfd_ctx *ctx,
+ unsigned long start,
+ unsigned long len)
{
- return mfill_atomic(dst_mm, start, 0, len, mmap_changing,
+ return mfill_atomic(ctx, start, 0, len,
uffd_flags_set_mode(0, MFILL_ATOMIC_ZEROPAGE));
}
-ssize_t mfill_atomic_continue(struct mm_struct *dst_mm, unsigned long start,
- unsigned long len, atomic_t *mmap_changing,
- uffd_flags_t flags)
+ssize_t mfill_atomic_continue(struct userfaultfd_ctx *ctx, unsigned long start,
+ unsigned long len, uffd_flags_t flags)
{
- return mfill_atomic(dst_mm, start, 0, len, mmap_changing,
+ return mfill_atomic(ctx, start, 0, len,
uffd_flags_set_mode(flags, MFILL_ATOMIC_CONTINUE));
}
-ssize_t mfill_atomic_poison(struct mm_struct *dst_mm, unsigned long start,
- unsigned long len, atomic_t *mmap_changing,
- uffd_flags_t flags)
+ssize_t mfill_atomic_poison(struct userfaultfd_ctx *ctx, unsigned long start,
+ unsigned long len, uffd_flags_t flags)
{
- return mfill_atomic(dst_mm, start, 0, len, mmap_changing,
+ return mfill_atomic(ctx, start, 0, len,
uffd_flags_set_mode(flags, MFILL_ATOMIC_POISON));
}
@@ -793,10 +799,10 @@ long uffd_wp_range(struct vm_area_struct *dst_vma,
return ret;
}
-int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start,
- unsigned long len, bool enable_wp,
- atomic_t *mmap_changing)
+int mwriteprotect_range(struct userfaultfd_ctx *ctx, unsigned long start,
+ unsigned long len, bool enable_wp)
{
+ struct mm_struct *dst_mm = ctx->mm;
unsigned long end = start + len;
unsigned long _start, _end;
struct vm_area_struct *dst_vma;
@@ -820,8 +826,9 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start,
* operation (e.g. mremap) running in parallel, bail out and
* request the user to retry later
*/
+ down_read(&ctx->map_changing_lock);
err = -EAGAIN;
- if (mmap_changing && atomic_read(mmap_changing))
+ if (atomic_read(&ctx->mmap_changing))
goto out_unlock;
err = -ENOENT;
@@ -850,6 +857,7 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start,
err = 0;
}
out_unlock:
+ up_read(&ctx->map_changing_lock);
mmap_read_unlock(dst_mm);
return err;
}
--
2.43.0.687.g38aa6559b0-goog
* Lokesh Gidra <[email protected]> [240208 16:22]:
> All userfaultfd operations, except write-protect, opportunistically use
> per-vma locks to lock vmas. On failure, attempt again inside mmap_lock
> critical section.
>
> Write-protect operation requires mmap_lock as it iterates over multiple
> vmas.
>
> Signed-off-by: Lokesh Gidra <[email protected]>
> ---
> fs/userfaultfd.c | 13 +-
> include/linux/userfaultfd_k.h | 5 +-
> mm/userfaultfd.c | 356 ++++++++++++++++++++++++++--------
> 3 files changed, 275 insertions(+), 99 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index c00a021bcce4..60dcfafdc11a 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -2005,17 +2005,8 @@ static int userfaultfd_move(struct userfaultfd_ctx *ctx,
> return -EINVAL;
>
> if (mmget_not_zero(mm)) {
> - mmap_read_lock(mm);
> -
> - /* Re-check after taking map_changing_lock */
> - down_read(&ctx->map_changing_lock);
> - if (likely(!atomic_read(&ctx->mmap_changing)))
> - ret = move_pages(ctx, mm, uffdio_move.dst, uffdio_move.src,
> - uffdio_move.len, uffdio_move.mode);
> - else
> - ret = -EAGAIN;
> - up_read(&ctx->map_changing_lock);
> - mmap_read_unlock(mm);
> + ret = move_pages(ctx, uffdio_move.dst, uffdio_move.src,
> + uffdio_move.len, uffdio_move.mode);
> mmput(mm);
> } else {
> return -ESRCH;
> diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> index 3210c3552976..05d59f74fc88 100644
> --- a/include/linux/userfaultfd_k.h
> +++ b/include/linux/userfaultfd_k.h
> @@ -138,9 +138,8 @@ extern long uffd_wp_range(struct vm_area_struct *vma,
> /* move_pages */
> void double_pt_lock(spinlock_t *ptl1, spinlock_t *ptl2);
> void double_pt_unlock(spinlock_t *ptl1, spinlock_t *ptl2);
> -ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
> - unsigned long dst_start, unsigned long src_start,
> - unsigned long len, __u64 flags);
> +ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> + unsigned long src_start, unsigned long len, __u64 flags);
> int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pmd_t dst_pmdval,
> struct vm_area_struct *dst_vma,
> struct vm_area_struct *src_vma,
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 74aad0831e40..1e25768b2136 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -19,20 +19,12 @@
> #include <asm/tlb.h>
> #include "internal.h"
>
> -static __always_inline
> -struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm,
> - unsigned long dst_start,
> - unsigned long len)
You could probably leave the __always_inline for this.
> +static bool validate_dst_vma(struct vm_area_struct *dst_vma,
> + unsigned long dst_end)
> {
> - /*
> - * Make sure that the dst range is both valid and fully within a
> - * single existing vma.
> - */
> - struct vm_area_struct *dst_vma;
> -
> - dst_vma = find_vma(dst_mm, dst_start);
> - if (!range_in_vma(dst_vma, dst_start, dst_start + len))
> - return NULL;
> + /* Make sure that the dst range is fully within dst_vma. */
> + if (dst_end > dst_vma->vm_end)
> + return false;
>
> /*
> * Check the vma is registered in uffd, this is required to
> @@ -40,11 +32,125 @@ struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm,
> * time.
> */
> if (!dst_vma->vm_userfaultfd_ctx.ctx)
> - return NULL;
> + return false;
> +
> + return true;
> +}
> +
> +#ifdef CONFIG_PER_VMA_LOCK
> +/*
> + * lock_vma() - Lookup and lock vma corresponding to @address.
> + * @mm: mm to search vma in.
> + * @address: address that the vma should contain.
> + * @prepare_anon: If true, then prepare the vma (if private) with anon_vma.
> + *
> + * Should be called without holding mmap_lock. vma should be unlocked after use
> + * with unlock_vma().
> + *
> + * Return: A locked vma containing @address, NULL if no vma is found, or
> + * -ENOMEM if anon_vma couldn't be allocated.
> + */
> +static struct vm_area_struct *lock_vma(struct mm_struct *mm,
> + unsigned long address,
> + bool prepare_anon)
> +{
> + struct vm_area_struct *vma;
> +
> + vma = lock_vma_under_rcu(mm, address);
> + if (vma) {
> + /*
> + * lock_vma_under_rcu() only checks anon_vma for private
> + * anonymous mappings. But we need to ensure it is assigned in
> + * private file-backed vmas as well.
> + */
> + if (prepare_anon && !(vma->vm_flags & VM_SHARED) &&
> + !vma->anon_vma)
> + vma_end_read(vma);
> + else
> + return vma;
> + }
> +
> + mmap_read_lock(mm);
> + vma = vma_lookup(mm, address);
> + if (vma) {
> + if (prepare_anon && !(vma->vm_flags & VM_SHARED) &&
> + anon_vma_prepare(vma)) {
> + vma = ERR_PTR(-ENOMEM);
> + } else {
> + /*
> + * We cannot use vma_start_read() as it may fail due to
> + * false locked (see comment in vma_start_read()). We
> + * can avoid that by directly locking vm_lock under
> + * mmap_lock, which guarantees that nobody can lock the
> + * vma for write (vma_start_write()) under us.
> + */
> + down_read(&vma->vm_lock->lock);
> + }
> + }
> +
> + mmap_read_unlock(mm);
> + return vma;
> +}
> +
> +static void unlock_vma(struct vm_area_struct *vma)
> +{
> + vma_end_read(vma);
> +}
> +
> +static struct vm_area_struct *find_and_lock_dst_vma(struct mm_struct *dst_mm,
> + unsigned long dst_start,
> + unsigned long len)
> +{
> + struct vm_area_struct *dst_vma;
> +
> + /* Ensure anon_vma is assigned for private vmas */
> + dst_vma = lock_vma(dst_mm, dst_start, true);
> +
> + if (!dst_vma)
> + return ERR_PTR(-ENOENT);
> +
> + if (PTR_ERR(dst_vma) == -ENOMEM)
> + return dst_vma;
> +
> + if (!validate_dst_vma(dst_vma, dst_start + len))
> + goto out_unlock;
>
> return dst_vma;
> +out_unlock:
> + unlock_vma(dst_vma);
> + return ERR_PTR(-ENOENT);
> }
>
> +#else
> +
> +static struct vm_area_struct *lock_mm_and_find_dst_vma(struct mm_struct *dst_mm,
> + unsigned long dst_start,
> + unsigned long len)
> +{
> + struct vm_area_struct *dst_vma;
> + int err = -ENOENT;
> +
> + mmap_read_lock(dst_mm);
> + dst_vma = vma_lookup(dst_mm, dst_start);
> + if (!dst_vma)
> + goto out_unlock;
> +
> + /* Ensure anon_vma is assigned for private vmas */
> + if (!(dst_vma->vm_flags & VM_SHARED) && anon_vma_prepare(dst_vma)) {
> + err = -ENOMEM;
> + goto out_unlock;
> + }
> +
> + if (!validate_dst_vma(dst_vma, dst_start + len))
> + goto out_unlock;
> +
> + return dst_vma;
> +out_unlock:
> + mmap_read_unlock(dst_mm);
> + return ERR_PTR(err);
> +}
> +#endif
> +
> /* Check if dst_addr is outside of file's size. Must be called with ptl held. */
> static bool mfill_file_over_size(struct vm_area_struct *dst_vma,
> unsigned long dst_addr)
> @@ -350,7 +456,8 @@ static pmd_t *mm_alloc_pmd(struct mm_struct *mm, unsigned long address)
> #ifdef CONFIG_HUGETLB_PAGE
> /*
> * mfill_atomic processing for HUGETLB vmas. Note that this routine is
> - * called with mmap_lock held, it will release mmap_lock before returning.
> + * called with either vma-lock or mmap_lock held, it will release the lock
> + * before returning.
> */
> static __always_inline ssize_t mfill_atomic_hugetlb(
> struct userfaultfd_ctx *ctx,
> @@ -361,7 +468,6 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> uffd_flags_t flags)
> {
> struct mm_struct *dst_mm = dst_vma->vm_mm;
> - int vm_shared = dst_vma->vm_flags & VM_SHARED;
> ssize_t err;
> pte_t *dst_pte;
> unsigned long src_addr, dst_addr;
> @@ -380,7 +486,11 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> */
> if (uffd_flags_mode_is(flags, MFILL_ATOMIC_ZEROPAGE)) {
> up_read(&ctx->map_changing_lock);
> +#ifdef CONFIG_PER_VMA_LOCK
> + unlock_vma(dst_vma);
> +#else
> mmap_read_unlock(dst_mm);
> +#endif
> return -EINVAL;
> }
>
> @@ -403,24 +513,32 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> * retry, dst_vma will be set to NULL and we must lookup again.
> */
> if (!dst_vma) {
> +#ifdef CONFIG_PER_VMA_LOCK
> + dst_vma = find_and_lock_dst_vma(dst_mm, dst_start, len);
> +#else
> + dst_vma = lock_mm_and_find_dst_vma(dst_mm, dst_start, len);
> +#endif
> + if (IS_ERR(dst_vma)) {
> + err = PTR_ERR(dst_vma);
> + goto out;
> + }
> +
> err = -ENOENT;
> - dst_vma = find_dst_vma(dst_mm, dst_start, len);
> - if (!dst_vma || !is_vm_hugetlb_page(dst_vma))
> - goto out_unlock;
> + if (!is_vm_hugetlb_page(dst_vma))
> + goto out_unlock_vma;
>
> err = -EINVAL;
> if (vma_hpagesize != vma_kernel_pagesize(dst_vma))
> - goto out_unlock;
> -
> - vm_shared = dst_vma->vm_flags & VM_SHARED;
> - }
> + goto out_unlock_vma;
>
> - /*
> - * If not shared, ensure the dst_vma has a anon_vma.
> - */
> - err = -ENOMEM;
> - if (!vm_shared) {
> - if (unlikely(anon_vma_prepare(dst_vma)))
> + /*
> + * If memory mappings are changing because of non-cooperative
> + * operation (e.g. mremap) running in parallel, bail out and
> + * request the user to retry later
> + */
> + down_read(&ctx->map_changing_lock);
> + err = -EAGAIN;
> + if (atomic_read(&ctx->mmap_changing))
> goto out_unlock;
> }
>
> @@ -465,7 +583,11 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
>
> if (unlikely(err == -ENOENT)) {
> up_read(&ctx->map_changing_lock);
> +#ifdef CONFIG_PER_VMA_LOCK
> + unlock_vma(dst_vma);
> +#else
> mmap_read_unlock(dst_mm);
> +#endif
> BUG_ON(!folio);
>
> err = copy_folio_from_user(folio,
> @@ -474,17 +596,6 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> err = -EFAULT;
> goto out;
> }
> - mmap_read_lock(dst_mm);
> - down_read(&ctx->map_changing_lock);
> - /*
> - * If memory mappings are changing because of non-cooperative
> - * operation (e.g. mremap) running in parallel, bail out and
> - * request the user to retry later
> - */
> - if (atomic_read(&ctx->mmap_changing)) {
> - err = -EAGAIN;
> - break;
> - }
>
> dst_vma = NULL;
> goto retry;
> @@ -505,7 +616,12 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
>
> out_unlock:
> up_read(&ctx->map_changing_lock);
> +out_unlock_vma:
> +#ifdef CONFIG_PER_VMA_LOCK
> + unlock_vma(dst_vma);
> +#else
> mmap_read_unlock(dst_mm);
> +#endif
> out:
> if (folio)
> folio_put(folio);
> @@ -597,7 +713,19 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> copied = 0;
> folio = NULL;
> retry:
> - mmap_read_lock(dst_mm);
> + /*
> + * Make sure the vma is not shared, that the dst range is
> + * both valid and fully within a single existing vma.
> + */
> +#ifdef CONFIG_PER_VMA_LOCK
> + dst_vma = find_and_lock_dst_vma(dst_mm, dst_start, len);
> +#else
> + dst_vma = lock_mm_and_find_dst_vma(dst_mm, dst_start, len);
> +#endif
> + if (IS_ERR(dst_vma)) {
> + err = PTR_ERR(dst_vma);
> + goto out;
> + }
>
> /*
> * If memory mappings are changing because of non-cooperative
> @@ -609,15 +737,6 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> if (atomic_read(&ctx->mmap_changing))
> goto out_unlock;
>
> - /*
> - * Make sure the vma is not shared, that the dst range is
> - * both valid and fully within a single existing vma.
> - */
> - err = -ENOENT;
> - dst_vma = find_dst_vma(dst_mm, dst_start, len);
> - if (!dst_vma)
> - goto out_unlock;
> -
> err = -EINVAL;
> /*
> * shmem_zero_setup is invoked in mmap for MAP_ANONYMOUS|MAP_SHARED but
> @@ -647,16 +766,6 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> uffd_flags_mode_is(flags, MFILL_ATOMIC_CONTINUE))
> goto out_unlock;
>
> - /*
> - * Ensure the dst_vma has a anon_vma or this page
> - * would get a NULL anon_vma when moved in the
> - * dst_vma.
> - */
> - err = -ENOMEM;
> - if (!(dst_vma->vm_flags & VM_SHARED) &&
> - unlikely(anon_vma_prepare(dst_vma)))
> - goto out_unlock;
> -
> while (src_addr < src_start + len) {
> pmd_t dst_pmdval;
>
> @@ -699,7 +808,11 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> void *kaddr;
>
> up_read(&ctx->map_changing_lock);
> +#ifdef CONFIG_PER_VMA_LOCK
> + unlock_vma(dst_vma);
> +#else
> mmap_read_unlock(dst_mm);
> +#endif
> BUG_ON(!folio);
>
> kaddr = kmap_local_folio(folio, 0);
> @@ -730,7 +843,11 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
>
> out_unlock:
> up_read(&ctx->map_changing_lock);
> +#ifdef CONFIG_PER_VMA_LOCK
> + unlock_vma(dst_vma);
> +#else
> mmap_read_unlock(dst_mm);
> +#endif
> out:
> if (folio)
> folio_put(folio);
> @@ -1267,16 +1384,67 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,
> if (!vma_is_anonymous(src_vma) || !vma_is_anonymous(dst_vma))
> return -EINVAL;
>
> - /*
> - * Ensure the dst_vma has a anon_vma or this page
> - * would get a NULL anon_vma when moved in the
> - * dst_vma.
> - */
> - if (unlikely(anon_vma_prepare(dst_vma)))
> - return -ENOMEM;
> + return 0;
> +}
> +
> +#ifdef CONFIG_PER_VMA_LOCK
> +static int find_and_lock_vmas(struct mm_struct *mm,
> + unsigned long dst_start,
> + unsigned long src_start,
> + struct vm_area_struct **dst_vmap,
> + struct vm_area_struct **src_vmap)
> +{
> + int err;
> +
> + /* There is no need to prepare anon_vma for src_vma */
> + *src_vmap = lock_vma(mm, src_start, false);
> + if (!*src_vmap)
> + return -ENOENT;
> +
> + /* Ensure anon_vma is assigned for anonymous vma */
> + *dst_vmap = lock_vma(mm, dst_start, true);
> + err = -ENOENT;
> + if (!*dst_vmap)
> + goto out_unlock;
> +
> + err = -ENOMEM;
> + if (PTR_ERR(*dst_vmap) == -ENOMEM)
> + goto out_unlock;
If you change lock_vma() to return the vma or ERR_PTR(-ENOENT) /
ERR_PTR(-ENOMEM), then you could change this to check IS_ERR() and
return the PTR_ERR().
You could also use IS_ERR_OR_NULL here, but the first suggestion will
simplify your life for find_and_lock_dst_vma() and the error type to
return.
What you have here will work though.
>
> return 0;
> +out_unlock:
> + unlock_vma(*src_vmap);
> + return err;
> }
> +#else
> +static int lock_mm_and_find_vmas(struct mm_struct *mm,
> + unsigned long dst_start,
> + unsigned long src_start,
> + struct vm_area_struct **dst_vmap,
> + struct vm_area_struct **src_vmap)
> +{
> + int err = -ENOENT;
Nit: new line after declarations.
> + mmap_read_lock(mm);
> +
> + *src_vmap = vma_lookup(mm, src_start);
> + if (!*src_vmap)
> + goto out_unlock;
> +
> + *dst_vmap = vma_lookup(mm, dst_start);
> + if (!*dst_vmap)
> + goto out_unlock;
> +
> + /* Ensure anon_vma is assigned */
> + err = -ENOMEM;
> + if (vma_is_anonymous(*dst_vmap) && anon_vma_prepare(*dst_vmap))
> + goto out_unlock;
> +
> + return 0;
> +out_unlock:
> + mmap_read_unlock(mm);
> + return err;
> +}
> +#endif
>
> /**
> * move_pages - move arbitrary anonymous pages of an existing vma
> @@ -1287,8 +1455,6 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,
> * @len: length of the virtual memory range
> * @mode: flags from uffdio_move.mode
> *
> - * Must be called with mmap_lock held for read.
> - *
Will either use the mmap_lock in read mode or per-vma locking ?
> * move_pages() remaps arbitrary anonymous pages atomically in zero
> * copy. It only works on non shared anonymous pages because those can
> * be relocated without generating non linear anon_vmas in the rmap
> @@ -1355,10 +1521,10 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,
> * could be obtained. This is the only additional complexity added to
> * the rmap code to provide this anonymous page remapping functionality.
> */
> -ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
> - unsigned long dst_start, unsigned long src_start,
> - unsigned long len, __u64 mode)
> +ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> + unsigned long src_start, unsigned long len, __u64 mode)
> {
> + struct mm_struct *mm = ctx->mm;
You dropped the argument, but left the comment for the argument.
> struct vm_area_struct *src_vma, *dst_vma;
> unsigned long src_addr, dst_addr;
> pmd_t *src_pmd, *dst_pmd;
> @@ -1376,28 +1542,40 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
> WARN_ON_ONCE(dst_start + len <= dst_start))
> goto out;
>
> +#ifdef CONFIG_PER_VMA_LOCK
> + err = find_and_lock_vmas(mm, dst_start, src_start,
> + &dst_vma, &src_vma);
> +#else
> + err = lock_mm_and_find_vmas(mm, dst_start, src_start,
> + &dst_vma, &src_vma);
> +#endif
I was hoping you could hide this completely, but it's probably better to
show what's going on and the function names document it well.
> + if (err)
> + goto out;
> +
> + /* Re-check after taking map_changing_lock */
> + down_read(&ctx->map_changing_lock);
> + if (likely(atomic_read(&ctx->mmap_changing))) {
> + err = -EAGAIN;
> + goto out_unlock;
> + }
> /*
> * Make sure the vma is not shared, that the src and dst remap
> * ranges are both valid and fully within a single existing
> * vma.
> */
> - src_vma = find_vma(mm, src_start);
> - if (!src_vma || (src_vma->vm_flags & VM_SHARED))
> - goto out;
> - if (src_start < src_vma->vm_start ||
> - src_start + len > src_vma->vm_end)
> - goto out;
> + if (src_vma->vm_flags & VM_SHARED)
> + goto out_unlock;
> + if (src_start + len > src_vma->vm_end)
> + goto out_unlock;
>
> - dst_vma = find_vma(mm, dst_start);
> - if (!dst_vma || (dst_vma->vm_flags & VM_SHARED))
> - goto out;
> - if (dst_start < dst_vma->vm_start ||
> - dst_start + len > dst_vma->vm_end)
> - goto out;
> + if (dst_vma->vm_flags & VM_SHARED)
> + goto out_unlock;
> + if (dst_start + len > dst_vma->vm_end)
> + goto out_unlock;
>
> err = validate_move_areas(ctx, src_vma, dst_vma);
> if (err)
> - goto out;
> + goto out_unlock;
>
> for (src_addr = src_start, dst_addr = dst_start;
> src_addr < src_start + len;) {
> @@ -1514,6 +1692,14 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
> moved += step_size;
> }
>
> +out_unlock:
> + up_read(&ctx->map_changing_lock);
> +#ifdef CONFIG_PER_VMA_LOCK
> + unlock_vma(dst_vma);
> + unlock_vma(src_vma);
> +#else
> + mmap_read_unlock(mm);
> +#endif
> out:
> VM_WARN_ON(moved < 0);
> VM_WARN_ON(err > 0);
> --
> 2.43.0.687.g38aa6559b0-goog
>
* Lokesh Gidra <[email protected]> [240209 13:02]:
> On Thu, Feb 8, 2024 at 7:07 PM Liam R. Howlett <[email protected]> wrote:
> >
> > * Lokesh Gidra <[email protected]> [240208 16:22]:
> > > All userfaultfd operations, except write-protect, opportunistically use
> > > per-vma locks to lock vmas. On failure, attempt again inside mmap_lock
> > > critical section.
> > >
> > > Write-protect operation requires mmap_lock as it iterates over multiple
> > > vmas.
> > >
> > > Signed-off-by: Lokesh Gidra <[email protected]>
> > > ---
> > > fs/userfaultfd.c | 13 +-
> > > include/linux/userfaultfd_k.h | 5 +-
> > > mm/userfaultfd.c | 356 ++++++++++++++++++++++++++--------
> > > 3 files changed, 275 insertions(+), 99 deletions(-)
> > >
> > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > > index c00a021bcce4..60dcfafdc11a 100644
> > > --- a/fs/userfaultfd.c
> > > +++ b/fs/userfaultfd.c
> > > @@ -2005,17 +2005,8 @@ static int userfaultfd_move(struct userfaultfd_ctx *ctx,
> > > return -EINVAL;
> > >
> > > if (mmget_not_zero(mm)) {
> > > - mmap_read_lock(mm);
> > > -
> > > - /* Re-check after taking map_changing_lock */
> > > - down_read(&ctx->map_changing_lock);
> > > - if (likely(!atomic_read(&ctx->mmap_changing)))
> > > - ret = move_pages(ctx, mm, uffdio_move.dst, uffdio_move.src,
> > > - uffdio_move.len, uffdio_move.mode);
> > > - else
> > > - ret = -EAGAIN;
> > > - up_read(&ctx->map_changing_lock);
> > > - mmap_read_unlock(mm);
> > > + ret = move_pages(ctx, uffdio_move.dst, uffdio_move.src,
> > > + uffdio_move.len, uffdio_move.mode);
> > > mmput(mm);
> > > } else {
> > > return -ESRCH;
> > > diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> > > index 3210c3552976..05d59f74fc88 100644
> > > --- a/include/linux/userfaultfd_k.h
> > > +++ b/include/linux/userfaultfd_k.h
> > > @@ -138,9 +138,8 @@ extern long uffd_wp_range(struct vm_area_struct *vma,
> > > /* move_pages */
> > > void double_pt_lock(spinlock_t *ptl1, spinlock_t *ptl2);
> > > void double_pt_unlock(spinlock_t *ptl1, spinlock_t *ptl2);
> > > -ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
> > > - unsigned long dst_start, unsigned long src_start,
> > > - unsigned long len, __u64 flags);
> > > +ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > + unsigned long src_start, unsigned long len, __u64 flags);
> > > int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pmd_t dst_pmdval,
> > > struct vm_area_struct *dst_vma,
> > > struct vm_area_struct *src_vma,
> > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > index 74aad0831e40..1e25768b2136 100644
> > > --- a/mm/userfaultfd.c
> > > +++ b/mm/userfaultfd.c
> > > @@ -19,20 +19,12 @@
> > > #include <asm/tlb.h>
> > > #include "internal.h"
> > >
> > > -static __always_inline
> > > -struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm,
> > > - unsigned long dst_start,
> > > - unsigned long len)
> >
> > You could probably leave the __always_inline for this.
>
> Sure
> >
> > > +static bool validate_dst_vma(struct vm_area_struct *dst_vma,
> > > + unsigned long dst_end)
> > > {
> > > - /*
> > > - * Make sure that the dst range is both valid and fully within a
> > > - * single existing vma.
> > > - */
> > > - struct vm_area_struct *dst_vma;
> > > -
> > > - dst_vma = find_vma(dst_mm, dst_start);
> > > - if (!range_in_vma(dst_vma, dst_start, dst_start + len))
> > > - return NULL;
> > > + /* Make sure that the dst range is fully within dst_vma. */
> > > + if (dst_end > dst_vma->vm_end)
> > > + return false;
> > >
> > > /*
> > > * Check the vma is registered in uffd, this is required to
> > > @@ -40,11 +32,125 @@ struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm,
> > > * time.
> > > */
> > > if (!dst_vma->vm_userfaultfd_ctx.ctx)
> > > - return NULL;
> > > + return false;
> > > +
> > > + return true;
> > > +}
> > > +
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > +/*
> > > + * lock_vma() - Lookup and lock vma corresponding to @address.
> > > + * @mm: mm to search vma in.
> > > + * @address: address that the vma should contain.
> > > + * @prepare_anon: If true, then prepare the vma (if private) with anon_vma.
> > > + *
> > > + * Should be called without holding mmap_lock. vma should be unlocked after use
> > > + * with unlock_vma().
> > > + *
> > > + * Return: A locked vma containing @address, NULL if no vma is found, or
> > > + * -ENOMEM if anon_vma couldn't be allocated.
> > > + */
> > > +static struct vm_area_struct *lock_vma(struct mm_struct *mm,
> > > + unsigned long address,
> > > + bool prepare_anon)
> > > +{
> > > + struct vm_area_struct *vma;
> > > +
> > > + vma = lock_vma_under_rcu(mm, address);
> > > + if (vma) {
> > > + /*
> > > + * lock_vma_under_rcu() only checks anon_vma for private
> > > + * anonymous mappings. But we need to ensure it is assigned in
> > > + * private file-backed vmas as well.
> > > + */
> > > + if (prepare_anon && !(vma->vm_flags & VM_SHARED) &&
> > > + !vma->anon_vma)
> > > + vma_end_read(vma);
> > > + else
> > > + return vma;
> > > + }
> > > +
> > > + mmap_read_lock(mm);
> > > + vma = vma_lookup(mm, address);
> > > + if (vma) {
> > > + if (prepare_anon && !(vma->vm_flags & VM_SHARED) &&
> > > + anon_vma_prepare(vma)) {
> > > + vma = ERR_PTR(-ENOMEM);
> > > + } else {
> > > + /*
> > > + * We cannot use vma_start_read() as it may fail due to
> > > + * false locked (see comment in vma_start_read()). We
> > > + * can avoid that by directly locking vm_lock under
> > > + * mmap_lock, which guarantees that nobody can lock the
> > > + * vma for write (vma_start_write()) under us.
> > > + */
> > > + down_read(&vma->vm_lock->lock);
> > > + }
> > > + }
> > > +
> > > + mmap_read_unlock(mm);
> > > + return vma;
> > > +}
> > > +
> > > +static void unlock_vma(struct vm_area_struct *vma)
> > > +{
> > > + vma_end_read(vma);
> > > +}
> > > +
> > > +static struct vm_area_struct *find_and_lock_dst_vma(struct mm_struct *dst_mm,
> > > + unsigned long dst_start,
> > > + unsigned long len)
> > > +{
> > > + struct vm_area_struct *dst_vma;
> > > +
> > > + /* Ensure anon_vma is assigned for private vmas */
> > > + dst_vma = lock_vma(dst_mm, dst_start, true);
> > > +
> > > + if (!dst_vma)
> > > + return ERR_PTR(-ENOENT);
> > > +
> > > + if (PTR_ERR(dst_vma) == -ENOMEM)
> > > + return dst_vma;
> > > +
> > > + if (!validate_dst_vma(dst_vma, dst_start + len))
> > > + goto out_unlock;
> > >
> > > return dst_vma;
> > > +out_unlock:
> > > + unlock_vma(dst_vma);
> > > + return ERR_PTR(-ENOENT);
> > > }
> > >
> > > +#else
> > > +
> > > +static struct vm_area_struct *lock_mm_and_find_dst_vma(struct mm_struct *dst_mm,
> > > + unsigned long dst_start,
> > > + unsigned long len)
> > > +{
> > > + struct vm_area_struct *dst_vma;
> > > + int err = -ENOENT;
> > > +
> > > + mmap_read_lock(dst_mm);
> > > + dst_vma = vma_lookup(dst_mm, dst_start);
> > > + if (!dst_vma)
> > > + goto out_unlock;
> > > +
> > > + /* Ensure anon_vma is assigned for private vmas */
> > > + if (!(dst_vma->vm_flags & VM_SHARED) && anon_vma_prepare(dst_vma)) {
> > > + err = -ENOMEM;
> > > + goto out_unlock;
> > > + }
> > > +
> > > + if (!validate_dst_vma(dst_vma, dst_start + len))
> > > + goto out_unlock;
> > > +
> > > + return dst_vma;
> > > +out_unlock:
> > > + mmap_read_unlock(dst_mm);
> > > + return ERR_PTR(err);
> > > +}
> > > +#endif
> > > +
> > > /* Check if dst_addr is outside of file's size. Must be called with ptl held. */
> > > static bool mfill_file_over_size(struct vm_area_struct *dst_vma,
> > > unsigned long dst_addr)
> > > @@ -350,7 +456,8 @@ static pmd_t *mm_alloc_pmd(struct mm_struct *mm, unsigned long address)
> > > #ifdef CONFIG_HUGETLB_PAGE
> > > /*
> > > * mfill_atomic processing for HUGETLB vmas. Note that this routine is
> > > - * called with mmap_lock held, it will release mmap_lock before returning.
> > > + * called with either vma-lock or mmap_lock held, it will release the lock
> > > + * before returning.
> > > */
> > > static __always_inline ssize_t mfill_atomic_hugetlb(
> > > struct userfaultfd_ctx *ctx,
> > > @@ -361,7 +468,6 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > > uffd_flags_t flags)
> > > {
> > > struct mm_struct *dst_mm = dst_vma->vm_mm;
> > > - int vm_shared = dst_vma->vm_flags & VM_SHARED;
> > > ssize_t err;
> > > pte_t *dst_pte;
> > > unsigned long src_addr, dst_addr;
> > > @@ -380,7 +486,11 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > > */
> > > if (uffd_flags_mode_is(flags, MFILL_ATOMIC_ZEROPAGE)) {
> > > up_read(&ctx->map_changing_lock);
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > + unlock_vma(dst_vma);
> > > +#else
> > > mmap_read_unlock(dst_mm);
> > > +#endif
> > > return -EINVAL;
> > > }
> > >
> > > @@ -403,24 +513,32 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > > * retry, dst_vma will be set to NULL and we must lookup again.
> > > */
> > > if (!dst_vma) {
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > + dst_vma = find_and_lock_dst_vma(dst_mm, dst_start, len);
> > > +#else
> > > + dst_vma = lock_mm_and_find_dst_vma(dst_mm, dst_start, len);
> > > +#endif
> > > + if (IS_ERR(dst_vma)) {
> > > + err = PTR_ERR(dst_vma);
> > > + goto out;
> > > + }
> > > +
> > > err = -ENOENT;
> > > - dst_vma = find_dst_vma(dst_mm, dst_start, len);
> > > - if (!dst_vma || !is_vm_hugetlb_page(dst_vma))
> > > - goto out_unlock;
> > > + if (!is_vm_hugetlb_page(dst_vma))
> > > + goto out_unlock_vma;
> > >
> > > err = -EINVAL;
> > > if (vma_hpagesize != vma_kernel_pagesize(dst_vma))
> > > - goto out_unlock;
> > > -
> > > - vm_shared = dst_vma->vm_flags & VM_SHARED;
> > > - }
> > > + goto out_unlock_vma;
> > >
> > > - /*
> > > - * If not shared, ensure the dst_vma has a anon_vma.
> > > - */
> > > - err = -ENOMEM;
> > > - if (!vm_shared) {
> > > - if (unlikely(anon_vma_prepare(dst_vma)))
> > > + /*
> > > + * If memory mappings are changing because of non-cooperative
> > > + * operation (e.g. mremap) running in parallel, bail out and
> > > + * request the user to retry later
> > > + */
> > > + down_read(&ctx->map_changing_lock);
> > > + err = -EAGAIN;
> > > + if (atomic_read(&ctx->mmap_changing))
> > > goto out_unlock;
> > > }
> > >
> > > @@ -465,7 +583,11 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > >
> > > if (unlikely(err == -ENOENT)) {
> > > up_read(&ctx->map_changing_lock);
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > + unlock_vma(dst_vma);
> > > +#else
> > > mmap_read_unlock(dst_mm);
> > > +#endif
> > > BUG_ON(!folio);
> > >
> > > err = copy_folio_from_user(folio,
> > > @@ -474,17 +596,6 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > > err = -EFAULT;
> > > goto out;
> > > }
> > > - mmap_read_lock(dst_mm);
> > > - down_read(&ctx->map_changing_lock);
> > > - /*
> > > - * If memory mappings are changing because of non-cooperative
> > > - * operation (e.g. mremap) running in parallel, bail out and
> > > - * request the user to retry later
> > > - */
> > > - if (atomic_read(&ctx->mmap_changing)) {
> > > - err = -EAGAIN;
> > > - break;
> > > - }
> > >
> > > dst_vma = NULL;
> > > goto retry;
> > > @@ -505,7 +616,12 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > >
> > > out_unlock:
> > > up_read(&ctx->map_changing_lock);
> > > +out_unlock_vma:
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > + unlock_vma(dst_vma);
> > > +#else
> > > mmap_read_unlock(dst_mm);
> > > +#endif
> > > out:
> > > if (folio)
> > > folio_put(folio);
> > > @@ -597,7 +713,19 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> > > copied = 0;
> > > folio = NULL;
> > > retry:
> > > - mmap_read_lock(dst_mm);
> > > + /*
> > > + * Make sure the vma is not shared, that the dst range is
> > > + * both valid and fully within a single existing vma.
> > > + */
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > + dst_vma = find_and_lock_dst_vma(dst_mm, dst_start, len);
> > > +#else
> > > + dst_vma = lock_mm_and_find_dst_vma(dst_mm, dst_start, len);
> > > +#endif
> > > + if (IS_ERR(dst_vma)) {
> > > + err = PTR_ERR(dst_vma);
> > > + goto out;
> > > + }
> > >
> > > /*
> > > * If memory mappings are changing because of non-cooperative
> > > @@ -609,15 +737,6 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> > > if (atomic_read(&ctx->mmap_changing))
> > > goto out_unlock;
> > >
> > > - /*
> > > - * Make sure the vma is not shared, that the dst range is
> > > - * both valid and fully within a single existing vma.
> > > - */
> > > - err = -ENOENT;
> > > - dst_vma = find_dst_vma(dst_mm, dst_start, len);
> > > - if (!dst_vma)
> > > - goto out_unlock;
> > > -
> > > err = -EINVAL;
> > > /*
> > > * shmem_zero_setup is invoked in mmap for MAP_ANONYMOUS|MAP_SHARED but
> > > @@ -647,16 +766,6 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> > > uffd_flags_mode_is(flags, MFILL_ATOMIC_CONTINUE))
> > > goto out_unlock;
> > >
> > > - /*
> > > - * Ensure the dst_vma has a anon_vma or this page
> > > - * would get a NULL anon_vma when moved in the
> > > - * dst_vma.
> > > - */
> > > - err = -ENOMEM;
> > > - if (!(dst_vma->vm_flags & VM_SHARED) &&
> > > - unlikely(anon_vma_prepare(dst_vma)))
> > > - goto out_unlock;
> > > -
> > > while (src_addr < src_start + len) {
> > > pmd_t dst_pmdval;
> > >
> > > @@ -699,7 +808,11 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> > > void *kaddr;
> > >
> > > up_read(&ctx->map_changing_lock);
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > + unlock_vma(dst_vma);
> > > +#else
> > > mmap_read_unlock(dst_mm);
> > > +#endif
> > > BUG_ON(!folio);
> > >
> > > kaddr = kmap_local_folio(folio, 0);
> > > @@ -730,7 +843,11 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> > >
> > > out_unlock:
> > > up_read(&ctx->map_changing_lock);
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > + unlock_vma(dst_vma);
> > > +#else
> > > mmap_read_unlock(dst_mm);
> > > +#endif
> > > out:
> > > if (folio)
> > > folio_put(folio);
> > > @@ -1267,16 +1384,67 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,
> > > if (!vma_is_anonymous(src_vma) || !vma_is_anonymous(dst_vma))
> > > return -EINVAL;
> > >
> > > - /*
> > > - * Ensure the dst_vma has a anon_vma or this page
> > > - * would get a NULL anon_vma when moved in the
> > > - * dst_vma.
> > > - */
> > > - if (unlikely(anon_vma_prepare(dst_vma)))
> > > - return -ENOMEM;
> > > + return 0;
> > > +}
> > > +
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > +static int find_and_lock_vmas(struct mm_struct *mm,
> > > + unsigned long dst_start,
> > > + unsigned long src_start,
> > > + struct vm_area_struct **dst_vmap,
> > > + struct vm_area_struct **src_vmap)
> > > +{
> > > + int err;
> > > +
> > > + /* There is no need to prepare anon_vma for src_vma */
> > > + *src_vmap = lock_vma(mm, src_start, false);
> > > + if (!*src_vmap)
> > > + return -ENOENT;
> > > +
> > > + /* Ensure anon_vma is assigned for anonymous vma */
> > > + *dst_vmap = lock_vma(mm, dst_start, true);
> > > + err = -ENOENT;
> > > + if (!*dst_vmap)
> > > + goto out_unlock;
> > > +
> > > + err = -ENOMEM;
> > > + if (PTR_ERR(*dst_vmap) == -ENOMEM)
> > > + goto out_unlock;
> >
> > If you change lock_vma() to return the vma or ERR_PTR(-ENOENT) /
> > ERR_PTR(-ENOMEM), then you could change this to check IS_ERR() and
> > return the PTR_ERR().
> >
> > You could also use IS_ERR_OR_NULL here, but the first suggestion will
> > simplify your life for find_and_lock_dst_vma() and the error type to
> > return.
>
> Good suggestion. I'll make the change. Thanks
> >
> > What you have here will work though.
> >
> > >
> > > return 0;
> > > +out_unlock:
> > > + unlock_vma(*src_vmap);
> > > + return err;
> > > }
> > > +#else
> > > +static int lock_mm_and_find_vmas(struct mm_struct *mm,
> > > + unsigned long dst_start,
> > > + unsigned long src_start,
> > > + struct vm_area_struct **dst_vmap,
> > > + struct vm_area_struct **src_vmap)
> > > +{
> > > + int err = -ENOENT;
> >
> > Nit: new line after declarations.
> >
> > > + mmap_read_lock(mm);
> > > +
> > > + *src_vmap = vma_lookup(mm, src_start);
> > > + if (!*src_vmap)
> > > + goto out_unlock;
> > > +
> > > + *dst_vmap = vma_lookup(mm, dst_start);
> > > + if (!*dst_vmap)
> > > + goto out_unlock;
> > > +
> > > + /* Ensure anon_vma is assigned */
> > > + err = -ENOMEM;
> > > + if (vma_is_anonymous(*dst_vmap) && anon_vma_prepare(*dst_vmap))
> > > + goto out_unlock;
> > > +
> > > + return 0;
> > > +out_unlock:
> > > + mmap_read_unlock(mm);
> > > + return err;
> > > +}
> > > +#endif
> > >
> > > /**
> > > * move_pages - move arbitrary anonymous pages of an existing vma
> > > @@ -1287,8 +1455,6 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,
> > > * @len: length of the virtual memory range
> > > * @mode: flags from uffdio_move.mode
> > > *
> > > - * Must be called with mmap_lock held for read.
> > > - *
> >
> > Will either use the mmap_lock in read mode or per-vma locking ?
>
> Makes sense. Will add it.
> >
> > > * move_pages() remaps arbitrary anonymous pages atomically in zero
> > > * copy. It only works on non shared anonymous pages because those can
> > > * be relocated without generating non linear anon_vmas in the rmap
> > > @@ -1355,10 +1521,10 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,
> > > * could be obtained. This is the only additional complexity added to
> > > * the rmap code to provide this anonymous page remapping functionality.
> > > */
> > > -ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
> > > - unsigned long dst_start, unsigned long src_start,
> > > - unsigned long len, __u64 mode)
> > > +ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > + unsigned long src_start, unsigned long len, __u64 mode)
> > > {
> > > + struct mm_struct *mm = ctx->mm;
> >
> > You dropped the argument, but left the comment for the argument.
>
> Thanks, will fix it.
> >
> > > struct vm_area_struct *src_vma, *dst_vma;
> > > unsigned long src_addr, dst_addr;
> > > pmd_t *src_pmd, *dst_pmd;
> > > @@ -1376,28 +1542,40 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
> > > WARN_ON_ONCE(dst_start + len <= dst_start))
> > > goto out;
> > >
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > + err = find_and_lock_vmas(mm, dst_start, src_start,
> > > + &dst_vma, &src_vma);
> > > +#else
> > > + err = lock_mm_and_find_vmas(mm, dst_start, src_start,
> > > + &dst_vma, &src_vma);
> > > +#endif
> >
> > I was hoping you could hide this completely, but it's probably better to
> > show what's going on and the function names document it well.
>
> I wanted to hide unlock as it's called several times, but then I
> thought you wanted explicit calls to mmap_read_unlock() so didn't hide
> it. If you are ok can I define unlock_vma() for !CONFIG_PER_VMA_LOCK
> as well, calling mmap_read_unlock()?
My bigger problem was with the name. We can't have unlock_vma()
just unlock the mm - it is confusing to read and I think it'll lead to
misunderstandings of what is really going on here.
Whatever you decide to do is fine as long as it's clear what's going on.
I think this is clear while hiding it could also be clear with the right
function name - I'm not sure what that would be; naming is hard.
Thanks,
Liam
* Lokesh Gidra <[email protected]> [240209 14:21]:
> On Fri, Feb 9, 2024 at 11:06 AM Liam R. Howlett <[email protected]> wrote:
> >
> > * Lokesh Gidra <[email protected]> [240209 13:02]:
> > > On Thu, Feb 8, 2024 at 7:07 PM Liam R. Howlett <[email protected]> wrote:
> > > >
> > > > * Lokesh Gidra <[email protected]> [240208 16:22]:
> > > > > All userfaultfd operations, except write-protect, opportunistically use
> > > > > per-vma locks to lock vmas. On failure, attempt again inside mmap_lock
> > > > > critical section.
> > > > >
> > > > > Write-protect operation requires mmap_lock as it iterates over multiple
> > > > > vmas.
> > > > >
> > > > > Signed-off-by: Lokesh Gidra <[email protected]>
> > > > > ---
> > > > > fs/userfaultfd.c | 13 +-
> > > > > include/linux/userfaultfd_k.h | 5 +-
> > > > > mm/userfaultfd.c | 356 ++++++++++++++++++++++++++--------
> > > > > 3 files changed, 275 insertions(+), 99 deletions(-)
> > > > >
> > > > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > > > > index c00a021bcce4..60dcfafdc11a 100644
> > > > > --- a/fs/userfaultfd.c
> > > > > +++ b/fs/userfaultfd.c
> > > > > @@ -2005,17 +2005,8 @@ static int userfaultfd_move(struct userfaultfd_ctx *ctx,
> > > > > return -EINVAL;
> > > > >
> > > > > if (mmget_not_zero(mm)) {
> > > > > - mmap_read_lock(mm);
> > > > > -
> > > > > - /* Re-check after taking map_changing_lock */
> > > > > - down_read(&ctx->map_changing_lock);
> > > > > - if (likely(!atomic_read(&ctx->mmap_changing)))
> > > > > - ret = move_pages(ctx, mm, uffdio_move.dst, uffdio_move.src,
> > > > > - uffdio_move.len, uffdio_move.mode);
> > > > > - else
> > > > > - ret = -EAGAIN;
> > > > > - up_read(&ctx->map_changing_lock);
> > > > > - mmap_read_unlock(mm);
> > > > > + ret = move_pages(ctx, uffdio_move.dst, uffdio_move.src,
> > > > > + uffdio_move.len, uffdio_move.mode);
> > > > > mmput(mm);
> > > > > } else {
> > > > > return -ESRCH;
> > > > > diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> > > > > index 3210c3552976..05d59f74fc88 100644
> > > > > --- a/include/linux/userfaultfd_k.h
> > > > > +++ b/include/linux/userfaultfd_k.h
> > > > > @@ -138,9 +138,8 @@ extern long uffd_wp_range(struct vm_area_struct *vma,
> > > > > /* move_pages */
> > > > > void double_pt_lock(spinlock_t *ptl1, spinlock_t *ptl2);
> > > > > void double_pt_unlock(spinlock_t *ptl1, spinlock_t *ptl2);
> > > > > -ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
> > > > > - unsigned long dst_start, unsigned long src_start,
> > > > > - unsigned long len, __u64 flags);
> > > > > +ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > + unsigned long src_start, unsigned long len, __u64 flags);
> > > > > int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pmd_t dst_pmdval,
> > > > > struct vm_area_struct *dst_vma,
> > > > > struct vm_area_struct *src_vma,
> > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > > index 74aad0831e40..1e25768b2136 100644
> > > > > --- a/mm/userfaultfd.c
> > > > > +++ b/mm/userfaultfd.c
> > > > > @@ -19,20 +19,12 @@
> > > > > #include <asm/tlb.h>
> > > > > #include "internal.h"
> > > > >
> > > > > -static __always_inline
> > > > > -struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm,
> > > > > - unsigned long dst_start,
> > > > > - unsigned long len)
> > > >
> > > > You could probably leave the __always_inline for this.
> > >
> > > Sure
> > > >
> > > > > +static bool validate_dst_vma(struct vm_area_struct *dst_vma,
> > > > > + unsigned long dst_end)
> > > > > {
> > > > > - /*
> > > > > - * Make sure that the dst range is both valid and fully within a
> > > > > - * single existing vma.
> > > > > - */
> > > > > - struct vm_area_struct *dst_vma;
> > > > > -
> > > > > - dst_vma = find_vma(dst_mm, dst_start);
> > > > > - if (!range_in_vma(dst_vma, dst_start, dst_start + len))
> > > > > - return NULL;
> > > > > + /* Make sure that the dst range is fully within dst_vma. */
> > > > > + if (dst_end > dst_vma->vm_end)
> > > > > + return false;
> > > > >
> > > > > /*
> > > > > * Check the vma is registered in uffd, this is required to
> > > > > @@ -40,11 +32,125 @@ struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm,
> > > > > * time.
> > > > > */
> > > > > if (!dst_vma->vm_userfaultfd_ctx.ctx)
> > > > > - return NULL;
> > > > > + return false;
> > > > > +
> > > > > + return true;
> > > > > +}
> > > > > +
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > +/*
> > > > > + * lock_vma() - Lookup and lock vma corresponding to @address.
> > > > > + * @mm: mm to search vma in.
> > > > > + * @address: address that the vma should contain.
> > > > > + * @prepare_anon: If true, then prepare the vma (if private) with anon_vma.
> > > > > + *
> > > > > + * Should be called without holding mmap_lock. vma should be unlocked after use
> > > > > + * with unlock_vma().
> > > > > + *
> > > > > + * Return: A locked vma containing @address, NULL if no vma is found, or
> > > > > + * -ENOMEM if anon_vma couldn't be allocated.
> > > > > + */
> > > > > +static struct vm_area_struct *lock_vma(struct mm_struct *mm,
> > > > > + unsigned long address,
> > > > > + bool prepare_anon)
> > > > > +{
> > > > > + struct vm_area_struct *vma;
> > > > > +
> > > > > + vma = lock_vma_under_rcu(mm, address);
> > > > > + if (vma) {
> > > > > + /*
> > > > > + * lock_vma_under_rcu() only checks anon_vma for private
> > > > > + * anonymous mappings. But we need to ensure it is assigned in
> > > > > + * private file-backed vmas as well.
> > > > > + */
> > > > > + if (prepare_anon && !(vma->vm_flags & VM_SHARED) &&
> > > > > + !vma->anon_vma)
> > > > > + vma_end_read(vma);
> > > > > + else
> > > > > + return vma;
> > > > > + }
> > > > > +
> > > > > + mmap_read_lock(mm);
> > > > > + vma = vma_lookup(mm, address);
> > > > > + if (vma) {
> > > > > + if (prepare_anon && !(vma->vm_flags & VM_SHARED) &&
> > > > > + anon_vma_prepare(vma)) {
> > > > > + vma = ERR_PTR(-ENOMEM);
> > > > > + } else {
> > > > > + /*
> > > > > + * We cannot use vma_start_read() as it may fail due to
> > > > > + * false locked (see comment in vma_start_read()). We
> > > > > + * can avoid that by directly locking vm_lock under
> > > > > + * mmap_lock, which guarantees that nobody can lock the
> > > > > + * vma for write (vma_start_write()) under us.
> > > > > + */
> > > > > + down_read(&vma->vm_lock->lock);
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + mmap_read_unlock(mm);
> > > > > + return vma;
> > > > > +}
> > > > > +
> > > > > +static void unlock_vma(struct vm_area_struct *vma)
> > > > > +{
> > > > > + vma_end_read(vma);
> > > > > +}
> > > > > +
> > > > > +static struct vm_area_struct *find_and_lock_dst_vma(struct mm_struct *dst_mm,
> > > > > + unsigned long dst_start,
> > > > > + unsigned long len)
> > > > > +{
> > > > > + struct vm_area_struct *dst_vma;
> > > > > +
> > > > > + /* Ensure anon_vma is assigned for private vmas */
> > > > > + dst_vma = lock_vma(dst_mm, dst_start, true);
> > > > > +
> > > > > + if (!dst_vma)
> > > > > + return ERR_PTR(-ENOENT);
> > > > > +
> > > > > + if (PTR_ERR(dst_vma) == -ENOMEM)
> > > > > + return dst_vma;
> > > > > +
> > > > > + if (!validate_dst_vma(dst_vma, dst_start + len))
> > > > > + goto out_unlock;
> > > > >
> > > > > return dst_vma;
> > > > > +out_unlock:
> > > > > + unlock_vma(dst_vma);
> > > > > + return ERR_PTR(-ENOENT);
> > > > > }
> > > > >
> > > > > +#else
> > > > > +
> > > > > +static struct vm_area_struct *lock_mm_and_find_dst_vma(struct mm_struct *dst_mm,
> > > > > + unsigned long dst_start,
> > > > > + unsigned long len)
> > > > > +{
> > > > > + struct vm_area_struct *dst_vma;
> > > > > + int err = -ENOENT;
> > > > > +
> > > > > + mmap_read_lock(dst_mm);
> > > > > + dst_vma = vma_lookup(dst_mm, dst_start);
> > > > > + if (!dst_vma)
> > > > > + goto out_unlock;
> > > > > +
> > > > > + /* Ensure anon_vma is assigned for private vmas */
> > > > > + if (!(dst_vma->vm_flags & VM_SHARED) && anon_vma_prepare(dst_vma)) {
> > > > > + err = -ENOMEM;
> > > > > + goto out_unlock;
> > > > > + }
> > > > > +
> > > > > + if (!validate_dst_vma(dst_vma, dst_start + len))
> > > > > + goto out_unlock;
> > > > > +
> > > > > + return dst_vma;
> > > > > +out_unlock:
> > > > > + mmap_read_unlock(dst_mm);
> > > > > + return ERR_PTR(err);
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > > /* Check if dst_addr is outside of file's size. Must be called with ptl held. */
> > > > > static bool mfill_file_over_size(struct vm_area_struct *dst_vma,
> > > > > unsigned long dst_addr)
> > > > > @@ -350,7 +456,8 @@ static pmd_t *mm_alloc_pmd(struct mm_struct *mm, unsigned long address)
> > > > > #ifdef CONFIG_HUGETLB_PAGE
> > > > > /*
> > > > > * mfill_atomic processing for HUGETLB vmas. Note that this routine is
> > > > > - * called with mmap_lock held, it will release mmap_lock before returning.
> > > > > + * called with either vma-lock or mmap_lock held, it will release the lock
> > > > > + * before returning.
> > > > > */
> > > > > static __always_inline ssize_t mfill_atomic_hugetlb(
> > > > > struct userfaultfd_ctx *ctx,
> > > > > @@ -361,7 +468,6 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > > > > uffd_flags_t flags)
> > > > > {
> > > > > struct mm_struct *dst_mm = dst_vma->vm_mm;
> > > > > - int vm_shared = dst_vma->vm_flags & VM_SHARED;
> > > > > ssize_t err;
> > > > > pte_t *dst_pte;
> > > > > unsigned long src_addr, dst_addr;
> > > > > @@ -380,7 +486,11 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > > > > */
> > > > > if (uffd_flags_mode_is(flags, MFILL_ATOMIC_ZEROPAGE)) {
> > > > > up_read(&ctx->map_changing_lock);
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > + unlock_vma(dst_vma);
> > > > > +#else
> > > > > mmap_read_unlock(dst_mm);
> > > > > +#endif
> > > > > return -EINVAL;
> > > > > }
> > > > >
> > > > > @@ -403,24 +513,32 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > > > > * retry, dst_vma will be set to NULL and we must lookup again.
> > > > > */
> > > > > if (!dst_vma) {
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > + dst_vma = find_and_lock_dst_vma(dst_mm, dst_start, len);
> > > > > +#else
> > > > > + dst_vma = lock_mm_and_find_dst_vma(dst_mm, dst_start, len);
> > > > > +#endif
> > > > > + if (IS_ERR(dst_vma)) {
> > > > > + err = PTR_ERR(dst_vma);
> > > > > + goto out;
> > > > > + }
> > > > > +
> > > > > err = -ENOENT;
> > > > > - dst_vma = find_dst_vma(dst_mm, dst_start, len);
> > > > > - if (!dst_vma || !is_vm_hugetlb_page(dst_vma))
> > > > > - goto out_unlock;
> > > > > + if (!is_vm_hugetlb_page(dst_vma))
> > > > > + goto out_unlock_vma;
> > > > >
> > > > > err = -EINVAL;
> > > > > if (vma_hpagesize != vma_kernel_pagesize(dst_vma))
> > > > > - goto out_unlock;
> > > > > -
> > > > > - vm_shared = dst_vma->vm_flags & VM_SHARED;
> > > > > - }
> > > > > + goto out_unlock_vma;
> > > > >
> > > > > - /*
> > > > > - * If not shared, ensure the dst_vma has a anon_vma.
> > > > > - */
> > > > > - err = -ENOMEM;
> > > > > - if (!vm_shared) {
> > > > > - if (unlikely(anon_vma_prepare(dst_vma)))
> > > > > + /*
> > > > > + * If memory mappings are changing because of non-cooperative
> > > > > + * operation (e.g. mremap) running in parallel, bail out and
> > > > > + * request the user to retry later
> > > > > + */
> > > > > + down_read(&ctx->map_changing_lock);
> > > > > + err = -EAGAIN;
> > > > > + if (atomic_read(&ctx->mmap_changing))
> > > > > goto out_unlock;
> > > > > }
> > > > >
> > > > > @@ -465,7 +583,11 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > > > >
> > > > > if (unlikely(err == -ENOENT)) {
> > > > > up_read(&ctx->map_changing_lock);
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > + unlock_vma(dst_vma);
> > > > > +#else
> > > > > mmap_read_unlock(dst_mm);
> > > > > +#endif
> > > > > BUG_ON(!folio);
> > > > >
> > > > > err = copy_folio_from_user(folio,
> > > > > @@ -474,17 +596,6 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > > > > err = -EFAULT;
> > > > > goto out;
> > > > > }
> > > > > - mmap_read_lock(dst_mm);
> > > > > - down_read(&ctx->map_changing_lock);
> > > > > - /*
> > > > > - * If memory mappings are changing because of non-cooperative
> > > > > - * operation (e.g. mremap) running in parallel, bail out and
> > > > > - * request the user to retry later
> > > > > - */
> > > > > - if (atomic_read(&ctx->mmap_changing)) {
> > > > > - err = -EAGAIN;
> > > > > - break;
> > > > > - }
> > > > >
> > > > > dst_vma = NULL;
> > > > > goto retry;
> > > > > @@ -505,7 +616,12 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > > > >
> > > > > out_unlock:
> > > > > up_read(&ctx->map_changing_lock);
> > > > > +out_unlock_vma:
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > + unlock_vma(dst_vma);
> > > > > +#else
> > > > > mmap_read_unlock(dst_mm);
> > > > > +#endif
> > > > > out:
> > > > > if (folio)
> > > > > folio_put(folio);
> > > > > @@ -597,7 +713,19 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> > > > > copied = 0;
> > > > > folio = NULL;
> > > > > retry:
> > > > > - mmap_read_lock(dst_mm);
> > > > > + /*
> > > > > + * Make sure the vma is not shared, that the dst range is
> > > > > + * both valid and fully within a single existing vma.
> > > > > + */
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > + dst_vma = find_and_lock_dst_vma(dst_mm, dst_start, len);
> > > > > +#else
> > > > > + dst_vma = lock_mm_and_find_dst_vma(dst_mm, dst_start, len);
> > > > > +#endif
> > > > > + if (IS_ERR(dst_vma)) {
> > > > > + err = PTR_ERR(dst_vma);
> > > > > + goto out;
> > > > > + }
> > > > >
> > > > > /*
> > > > > * If memory mappings are changing because of non-cooperative
> > > > > @@ -609,15 +737,6 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> > > > > if (atomic_read(&ctx->mmap_changing))
> > > > > goto out_unlock;
> > > > >
> > > > > - /*
> > > > > - * Make sure the vma is not shared, that the dst range is
> > > > > - * both valid and fully within a single existing vma.
> > > > > - */
> > > > > - err = -ENOENT;
> > > > > - dst_vma = find_dst_vma(dst_mm, dst_start, len);
> > > > > - if (!dst_vma)
> > > > > - goto out_unlock;
> > > > > -
> > > > > err = -EINVAL;
> > > > > /*
> > > > > * shmem_zero_setup is invoked in mmap for MAP_ANONYMOUS|MAP_SHARED but
> > > > > @@ -647,16 +766,6 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> > > > > uffd_flags_mode_is(flags, MFILL_ATOMIC_CONTINUE))
> > > > > goto out_unlock;
> > > > >
> > > > > - /*
> > > > > - * Ensure the dst_vma has a anon_vma or this page
> > > > > - * would get a NULL anon_vma when moved in the
> > > > > - * dst_vma.
> > > > > - */
> > > > > - err = -ENOMEM;
> > > > > - if (!(dst_vma->vm_flags & VM_SHARED) &&
> > > > > - unlikely(anon_vma_prepare(dst_vma)))
> > > > > - goto out_unlock;
> > > > > -
> > > > > while (src_addr < src_start + len) {
> > > > > pmd_t dst_pmdval;
> > > > >
> > > > > @@ -699,7 +808,11 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> > > > > void *kaddr;
> > > > >
> > > > > up_read(&ctx->map_changing_lock);
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > + unlock_vma(dst_vma);
> > > > > +#else
> > > > > mmap_read_unlock(dst_mm);
> > > > > +#endif
> > > > > BUG_ON(!folio);
> > > > >
> > > > > kaddr = kmap_local_folio(folio, 0);
> > > > > @@ -730,7 +843,11 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> > > > >
> > > > > out_unlock:
> > > > > up_read(&ctx->map_changing_lock);
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > + unlock_vma(dst_vma);
> > > > > +#else
> > > > > mmap_read_unlock(dst_mm);
> > > > > +#endif
> > > > > out:
> > > > > if (folio)
> > > > > folio_put(folio);
> > > > > @@ -1267,16 +1384,67 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,
> > > > > if (!vma_is_anonymous(src_vma) || !vma_is_anonymous(dst_vma))
> > > > > return -EINVAL;
> > > > >
> > > > > - /*
> > > > > - * Ensure the dst_vma has a anon_vma or this page
> > > > > - * would get a NULL anon_vma when moved in the
> > > > > - * dst_vma.
> > > > > - */
> > > > > - if (unlikely(anon_vma_prepare(dst_vma)))
> > > > > - return -ENOMEM;
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > +static int find_and_lock_vmas(struct mm_struct *mm,
> > > > > + unsigned long dst_start,
> > > > > + unsigned long src_start,
> > > > > + struct vm_area_struct **dst_vmap,
> > > > > + struct vm_area_struct **src_vmap)
> > > > > +{
> > > > > + int err;
> > > > > +
> > > > > + /* There is no need to prepare anon_vma for src_vma */
> > > > > + *src_vmap = lock_vma(mm, src_start, false);
> > > > > + if (!*src_vmap)
> > > > > + return -ENOENT;
> > > > > +
> > > > > + /* Ensure anon_vma is assigned for anonymous vma */
> > > > > + *dst_vmap = lock_vma(mm, dst_start, true);
> > > > > + err = -ENOENT;
> > > > > + if (!*dst_vmap)
> > > > > + goto out_unlock;
> > > > > +
> > > > > + err = -ENOMEM;
> > > > > + if (PTR_ERR(*dst_vmap) == -ENOMEM)
> > > > > + goto out_unlock;
> > > >
> > > > If you change lock_vma() to return the vma or ERR_PTR(-ENOENT) /
> > > > ERR_PTR(-ENOMEM), then you could change this to check IS_ERR() and
> > > > return the PTR_ERR().
> > > >
> > > > You could also use IS_ERR_OR_NULL here, but the first suggestion will
> > > > simplify your life for find_and_lock_dst_vma() and the error type to
> > > > return.
> > >
> > > Good suggestion. I'll make the change. Thanks
> > > >
> > > > What you have here will work though.
> > > >
> > > > >
> > > > > return 0;
> > > > > +out_unlock:
> > > > > + unlock_vma(*src_vmap);
> > > > > + return err;
> > > > > }
> > > > > +#else
> > > > > +static int lock_mm_and_find_vmas(struct mm_struct *mm,
> > > > > + unsigned long dst_start,
> > > > > + unsigned long src_start,
> > > > > + struct vm_area_struct **dst_vmap,
> > > > > + struct vm_area_struct **src_vmap)
> > > > > +{
> > > > > + int err = -ENOENT;
> > > >
> > > > Nit: new line after declarations.
> > > >
> > > > > + mmap_read_lock(mm);
> > > > > +
> > > > > + *src_vmap = vma_lookup(mm, src_start);
> > > > > + if (!*src_vmap)
> > > > > + goto out_unlock;
> > > > > +
> > > > > + *dst_vmap = vma_lookup(mm, dst_start);
> > > > > + if (!*dst_vmap)
> > > > > + goto out_unlock;
> > > > > +
> > > > > + /* Ensure anon_vma is assigned */
> > > > > + err = -ENOMEM;
> > > > > + if (vma_is_anonymous(*dst_vmap) && anon_vma_prepare(*dst_vmap))
> > > > > + goto out_unlock;
> > > > > +
> > > > > + return 0;
> > > > > +out_unlock:
> > > > > + mmap_read_unlock(mm);
> > > > > + return err;
> > > > > +}
> > > > > +#endif
> > > > >
> > > > > /**
> > > > > * move_pages - move arbitrary anonymous pages of an existing vma
> > > > > @@ -1287,8 +1455,6 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,
> > > > > * @len: length of the virtual memory range
> > > > > * @mode: flags from uffdio_move.mode
> > > > > *
> > > > > - * Must be called with mmap_lock held for read.
> > > > > - *
> > > >
> > > > Will either use the mmap_lock in read mode or per-vma locking ?
> > >
> > > Makes sense. Will add it.
> > > >
> > > > > * move_pages() remaps arbitrary anonymous pages atomically in zero
> > > > > * copy. It only works on non shared anonymous pages because those can
> > > > > * be relocated without generating non linear anon_vmas in the rmap
> > > > > @@ -1355,10 +1521,10 @@ static int validate_move_areas(struct userfaultfd_ctx *ctx,
> > > > > * could be obtained. This is the only additional complexity added to
> > > > > * the rmap code to provide this anonymous page remapping functionality.
> > > > > */
> > > > > -ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
> > > > > - unsigned long dst_start, unsigned long src_start,
> > > > > - unsigned long len, __u64 mode)
> > > > > +ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > + unsigned long src_start, unsigned long len, __u64 mode)
> > > > > {
> > > > > + struct mm_struct *mm = ctx->mm;
> > > >
> > > > You dropped the argument, but left the comment for the argument.
> > >
> > > Thanks, will fix it.
> > > >
> > > > > struct vm_area_struct *src_vma, *dst_vma;
> > > > > unsigned long src_addr, dst_addr;
> > > > > pmd_t *src_pmd, *dst_pmd;
> > > > > @@ -1376,28 +1542,40 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm,
> > > > > WARN_ON_ONCE(dst_start + len <= dst_start))
> > > > > goto out;
> > > > >
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > + err = find_and_lock_vmas(mm, dst_start, src_start,
> > > > > + &dst_vma, &src_vma);
> > > > > +#else
> > > > > + err = lock_mm_and_find_vmas(mm, dst_start, src_start,
> > > > > + &dst_vma, &src_vma);
> > > > > +#endif
> > > >
> > > > I was hoping you could hide this completely, but it's probably better to
> > > > show what's going on and the function names document it well.
> > >
> > > I wanted to hide unlock as it's called several times, but then I
> > > thought you wanted explicit calls to mmap_read_unlock() so didn't hide
> > > it. If you are ok can I define unlock_vma() for !CONFIG_PER_VMA_LOCK
> > > as well, calling mmap_read_unlock()?
> >
> > My bigger problem was with the name. We can't have unlock_vma()
> > just unlock the mm - it is confusing to read and I think it'll lead to
> > misunderstandings of what is really going on here.
> >
> > Whatever you decide to do is fine as long as it's clear what's going on.
> > I think this is clear while hiding it could also be clear with the right
> > function name - I'm not sure what that would be; naming is hard.
>
> Maybe unlock_mm_or_vma() ? If not then I'll just keep it as is.
>
Maybe just leave it as it is unless someone else has issue with it.
Using some form of uffd_unlock name runs into the question of that
atomic and the new lock.
On Mon, Feb 12, 2024 at 7:20 AM Liam R. Howlett <[email protected]> wrote:
>
> * Lokesh Gidra <[email protected]> [240209 15:59]:
> > On Fri, Feb 9, 2024 at 11:31 AM Liam R. Howlett <[email protected]> wrote:
> ...
>
> > > > > > > > +static struct vm_area_struct *lock_vma(struct mm_struct *mm,
> > > > > > > > + unsigned long address,
> > > > > > > > + bool prepare_anon)
> > > > > > > > +{
> > > > > > > > + struct vm_area_struct *vma;
> > > > > > > > +
> > > > > > > > + vma = lock_vma_under_rcu(mm, address);
> > > > > > > > + if (vma) {
> > > > > > > > + /*
> > > > > > > > + * lock_vma_under_rcu() only checks anon_vma for private
> > > > > > > > + * anonymous mappings. But we need to ensure it is assigned in
> > > > > > > > + * private file-backed vmas as well.
> > > > > > > > + */
> > > > > > > > + if (prepare_anon && !(vma->vm_flags & VM_SHARED) &&
> > > > > > > > + !vma->anon_vma)
> > > > > > > > + vma_end_read(vma);
> > > > > > > > + else
> > > > > > > > + return vma;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + mmap_read_lock(mm);
> > > > > > > > + vma = vma_lookup(mm, address);
> > > > > > > > + if (vma) {
> > > > > > > > + if (prepare_anon && !(vma->vm_flags & VM_SHARED) &&
> > > > > > > > + anon_vma_prepare(vma)) {
> > > > > > > > + vma = ERR_PTR(-ENOMEM);
> > > > > > > > + } else {
> > > > > > > > + /*
> > > > > > > > + * We cannot use vma_start_read() as it may fail due to
> > > > > > > > + * false locked (see comment in vma_start_read()). We
> > > > > > > > + * can avoid that by directly locking vm_lock under
> > > > > > > > + * mmap_lock, which guarantees that nobody can lock the
> > > > > > > > + * vma for write (vma_start_write()) under us.
> > > > > > > > + */
> > > > > > > > + down_read(&vma->vm_lock->lock);
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + mmap_read_unlock(mm);
> > > > > > > > + return vma;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void unlock_vma(struct vm_area_struct *vma)
> > > > > > > > +{
> > > > > > > > + vma_end_read(vma);
> > > > > > > > +}
> > > > > > > > +
> ...
>
> >
> > The current implementation has a deadlock problem:
> >
> > thread 1
> > thread 2
> >
> > vma_start_read(dst_vma)
> >
> >
> > mmap_write_lock()
> >
> > vma_start_write(src_vma)
> > vma_start_read(src_vma) fails
> > mmap_read_lock() blocks
> >
> >
> > vma_start_write(dst_vma)
> > blocks
> >
> >
> > I think the solution is to implement it this way:
> >
> > long find_and_lock_vmas(...)
> > {
> > dst_vma = lock_vma(mm, dst_start, true);
> > if (IS_ERR(dst_vma))
> > return PTR_ERR(vma);
> >
> > src_vma = lock_vma_under_rcu(mm, src_start);
> > if (!src_vma) {
> > long err;
> > if (mmap_read_trylock(mm)) {
> > src_vma = vma_lookup(mm, src_start);
> > if (src_vma) {
> >
> > down_read(&src_vma->vm_lock->lock);
> > err = 0;
> > } else {
> > err = -ENOENT;
> > }
> > mmap_read_unlock(mm);
> > } else {
> > vma_end_read(dst_vma);
> > err = lock_mm_and_find_vmas(...);
> > if (!err) {
>
> Right now lock_mm_and_find_vmas() doesn't use the per-vma locking, so
> you'd have to lock those here too. I'm sure you realise that, but this
> is very convoluted.
That's right. I realized that after I sent this email.
>
> > mmap_read_unlock(mm);
> > }
> > }
> > return err;
>
> On contention you will now abort vs block.
Is it? On contention mmap_read_trylock() will fail and we do the whole
operation using lock_mm_and_find_vmas() which blocks on mmap_lock. Am
I missing something?
>
> > }
> > return 0;
> > }
> >
> > Of course this would need defining lock_mm_and_find_vmas() regardless
> > of CONFIG_PER_VMA_LOCK. I can also remove the prepare_anon condition
> > in lock_vma().
>
> You are adding a lot of complexity for a relatively rare case, which is
> probably not worth optimising.
>
> I think you'd be better served by something like :
>
> if (likely(src_vma))
> return 0;
>
> /* Undo any locking */
> vma_end_read(dst_vma);
>
> /* Fall back to locking both in mmap_lock critical section */
Agreed on reduced complexity. But as Suren pointed out in one of his
replies that lock_vma_under_rcu() may fail due to seq overflow. That's
why lock_vma() uses vma_lookup() followed by direct down_read() on
vma-lock. IMHO what we need here is exactly lock_mm_and_find_vmas()
and the code can be further simplified as follows:
err = lock_mm_and_find_vmas(...);
if (!err) {
down_read(dst_vma...);
if (dst_vma != src_vma)
down_read(src_vma....);
mmap_read_unlock(mm);
}
return err;
(another thing I'm fixing in the next version is avoiding locking the
vma twice if both src_start and dst_start are in same vma)
> mmap_read_lock();
> /*
> * probably worth creating an inline function for a single vma
> * find/populate that can be used in lock_vma() that does the anon vma
> * population?
> */
Good idea. This can simplify both lock_vma() as well as lock_mm_and_find_vmas().
> dst_vm = lock_vma_populate_anon();
> ...
>
> src_vma = lock_vma_under_rcu(mm, src_start);
> ...
>
>
> mmap_read_unlock();
> return 0;
>
> src_failed:
> vma_end_read(dst_vma);
> dst_failed:
> mmap_read_unlock();
> return err;
>
> Thanks,
> Liam
>
> ...
>
On Mon, Feb 12, 2024 at 12:11 PM Liam R. Howlett
<[email protected]> wrote:
>
> * Lokesh Gidra <[email protected]> [240212 13:08]:
> > On Mon, Feb 12, 2024 at 7:20 AM Liam R. Howlett <[email protected]> wrote:
> ...
>
> > > >
> > > > The current implementation has a deadlock problem:
> ...
>
> > > On contention you will now abort vs block.
> >
> > Is it? On contention mmap_read_trylock() will fail and we do the whole
> > operation using lock_mm_and_find_vmas() which blocks on mmap_lock. Am
> > I missing something?
>
> You are right, I missed the taking of the lock in the function call.
>
> > >
> > > > }
> > > > return 0;
> > > > }
> > > >
> > > > Of course this would need defining lock_mm_and_find_vmas() regardless
> > > > of CONFIG_PER_VMA_LOCK. I can also remove the prepare_anon condition
> > > > in lock_vma().
> > >
> > > You are adding a lot of complexity for a relatively rare case, which is
> > > probably not worth optimising.
> > >
> ...
>
> >
> > Agreed on reduced complexity. But as Suren pointed out in one of his
> > replies that lock_vma_under_rcu() may fail due to seq overflow. That's
> > why lock_vma() uses vma_lookup() followed by direct down_read() on
> > vma-lock.
>
> I'd rather see another function that doesn't care about anon (I think
> src is special that way?), and avoid splitting the locking across
> functions as much as possible.
>
Fair point about not splitting locking across functions.
>
> > IMHO what we need here is exactly lock_mm_and_find_vmas()
> > and the code can be further simplified as follows:
> >
> > err = lock_mm_and_find_vmas(...);
> > if (!err) {
> > down_read(dst_vma...);
> > if (dst_vma != src_vma)
> > down_read(src_vma....);
> > mmap_read_unlock(mm);
> > }
> > return err;
>
> If we exactly needed lock_mm_and_find_vmas(), there wouldn't be three
> lock/unlock calls depending on the return code.
>
> The fact that lock_mm_and_find_vmas() returns with the mm locked or
> unlocked depending on the return code is not reducing the complexity of
> this code.
>
> You could use a widget that does something with dst, and a different
> widget that does something with src (if they are different). The dst
> widget can be used for the lock_vma(), and in the
> lock_mm_and_find_vmas(), while the src one can be used in this and the
> lock_mm_and_find_vmas(). Neither widget would touch the locks. This way
> you can build your functions that have the locking and unlocking
> co-located (except the obvious necessity of holding the mmap_read lock
> for the !per-vma case).
>
I think I have managed to minimize the code duplication while not
complicating locking/unlocking.
I have added a find_vmas_mm_locked() handler which, as the name
suggests, expects mmap_lock is held and finds the two vmas and ensures
anon_vma for dst_vma is populated. I call this handler from
lock_mm_and_find_vmas() and find_and_lock_vmas() in the fallback case.
I have also introduced a handler for finding dst_vma and preparing its
anon_vma, which is used in lock_vma() and find_vmas_mm_locked().
Sounds good?
> I've also thought of how you can name the abstraction in the functions:
> use a 'prepare() and complete()' to find/lock and unlock what you need.
> Might be worth exploring? If we fail to 'prepare()' then we don't need
> to 'complete()', which means there won't be mismatched locking hanging
> around. Maybe it's too late to change to this sort of thing, but I
> thought I'd mention it.
>
Nice suggestion! But after (fortunately) finding the function names
that are self-explanatory, dropping them seems like going in the wrong
direction. Please let me know if you think this is a missing piece. I
am open to incorporating this.
> Thanks,
> Liam
* Lokesh Gidra <[email protected]> [240212 17:31]:
> I have also introduced a handler for finding dst_vma and preparing its
> anon_vma, which is used in lock_vma() and find_vmas_mm_locked().
>
> Sounds good?
>
> > I've also thought of how you can name the abstraction in the functions:
> > use a 'prepare() and complete()' to find/lock and unlock what you need.
> > Might be worth exploring? If we fail to 'prepare()' then we don't need
> > to 'complete()', which means there won't be mismatched locking hanging
> > around. Maybe it's too late to change to this sort of thing, but I
> > thought I'd mention it.
> >
> Nice suggestion! But after (fortunately) finding the function names
> that are self-explanatory, dropping them seems like going in the wrong
> direction. Please let me know if you think this is a missing piece. I
> am open to incorporating this.
This plan sounds good, thanks!
Regards,
Liam