Re: [PATCH v4 3/3] userfaultfd: use per-vma locks in userfaultfd operations

From: Lokesh Gidra
Date: Fri Feb 09 2024 - 15:59:22 EST

Next message: Sergey Shtylyov: "Re: [PATCH net-next v2 5/5] net: ravb: Add runtime PM support"
Previous message: Dan Williams: "Re: [RFC PATCH v2 0/4] tsm: Runtime measurement registers ABI"
In reply to: Liam R. Howlett: "Re: [PATCH v4 3/3] userfaultfd: use per-vma locks in userfaultfd operations"
Next in thread: Liam R. Howlett: "Re: [PATCH v4 3/3] userfaultfd: use per-vma locks in userfaultfd operations"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Feb 9, 2024 at 11:31 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
>
> * Lokesh Gidra <lokeshgidra@xxxxxxxxxx> [240209 14:21]:
> > On Fri, Feb 9, 2024 at 11:06 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
> > >
> > > * Lokesh Gidra <lokeshgidra@xxxxxxxxxx> [240209 13:02]:
> > > > On Thu, Feb 8, 2024 at 7:07 PM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
> > > > >
> > > > > * Lokesh Gidra <lokeshgidra@xxxxxxxxxx> [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 <lokeshgidra@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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;
> > > > > > }

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) {
mmap_read_unlock(mm);
}
}
return err;
}
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().

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

Sure. I'll let it as it is.
>
>

Next message: Sergey Shtylyov: "Re: [PATCH net-next v2 5/5] net: ravb: Add runtime PM support"
Previous message: Dan Williams: "Re: [RFC PATCH v2 0/4] tsm: Runtime measurement registers ABI"
In reply to: Liam R. Howlett: "Re: [PATCH v4 3/3] userfaultfd: use per-vma locks in userfaultfd operations"
Next in thread: Liam R. Howlett: "Re: [PATCH v4 3/3] userfaultfd: use per-vma locks in userfaultfd operations"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]