On Tue, Jun 27, 2023 at 8:54 AM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> On Mon, Jun 26, 2023 at 09:23:21PM -0700, Suren Baghdasaryan wrote:
> > Enable handle_userfault to operate under VMA lock by releasing VMA lock
> > instead of mmap_lock and retrying.
>
> This mostly good to me (besides the new DROP flag.. of course), thanks.
> Still some nitpicks below.
>
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > ---
> > fs/userfaultfd.c | 42 ++++++++++++++++++++++--------------------
> > mm/memory.c | 9 ---------
> > 2 files changed, 22 insertions(+), 29 deletions(-)
> >
> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index 4e800bb7d2ab..b88632c404b6 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -277,17 +277,17 @@ static inline struct uffd_msg userfault_msg(unsigned long address,
> > * hugepmd ranges.
> > */
> > static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx,
> > - struct vm_area_struct *vma,
> > - unsigned long address,
> > - unsigned long flags,
> > - unsigned long reason)
> > + struct vm_fault *vmf,
> > + unsigned long reason)
> > {
> > + struct vm_area_struct *vma = vmf->vma;
> > pte_t *ptep, pte;
> > bool ret = true;
> >
> > - mmap_assert_locked(ctx->mm);
> > + if (!(vmf->flags & FAULT_FLAG_VMA_LOCK))
> > + mmap_assert_locked(ctx->mm);
>
> Maybe we can have a helper asserting proper vma protector locks (mmap for
> !VMA_LOCK and vma read lock for VMA_LOCK)? It basically tells the context
> the vma is still safe to access.
>
> >
> > - ptep = hugetlb_walk(vma, address, vma_mmu_pagesize(vma));
> > + ptep = hugetlb_walk(vma, vmf->address, vma_mmu_pagesize(vma));
> > if (!ptep)
> > goto out;
> >
> > @@ -308,10 +308,8 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx,
> > }
> > #else
> > static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx,
> > - struct vm_area_struct *vma,
> > - unsigned long address,
> > - unsigned long flags,
> > - unsigned long reason)
> > + struct vm_fault *vmf,
> > + unsigned long reason)
> > {
> > return false; /* should never get here */
> > }
> > @@ -325,11 +323,11 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx,
> > * threads.
> > */
> > static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx,
> > - unsigned long address,
> > - unsigned long flags,
> > + struct vm_fault *vmf,
> > unsigned long reason)
> > {
> > struct mm_struct *mm = ctx->mm;
> > + unsigned long address = vmf->address;
> > pgd_t *pgd;
> > p4d_t *p4d;
> > pud_t *pud;
> > @@ -337,7 +335,8 @@ static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx,
> > pte_t *pte;
> > bool ret = true;
> >
> > - mmap_assert_locked(mm);
> > + if (!(vmf->flags & FAULT_FLAG_VMA_LOCK))
> > + mmap_assert_locked(mm);
>
> (the assert helper can also be used here)
>
> >
> > pgd = pgd_offset(mm, address);
> > if (!pgd_present(*pgd))
> > @@ -445,7 +444,8 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason)
> > * Coredumping runs without mmap_lock so we can only check that
> > * the mmap_lock is held, if PF_DUMPCORE was not set.
> > */
> > - mmap_assert_locked(mm);
> > + if (!(vmf->flags & FAULT_FLAG_VMA_LOCK))
> > + mmap_assert_locked(mm);
> >
> > ctx = vma->vm_userfaultfd_ctx.ctx;
> > if (!ctx)
> > @@ -561,15 +561,17 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason)
> > spin_unlock_irq(&ctx->fault_pending_wqh.lock);
> >
> > if (!is_vm_hugetlb_page(vma))
> > - must_wait = userfaultfd_must_wait(ctx, vmf->address, vmf->flags,
> > - reason);
> > + must_wait = userfaultfd_must_wait(ctx, vmf, reason);
> > else
> > - must_wait = userfaultfd_huge_must_wait(ctx, vma,
> > - vmf->address,
> > - vmf->flags, reason);
> > + must_wait = userfaultfd_huge_must_wait(ctx, vmf, reason);
> > if (is_vm_hugetlb_page(vma))
> > hugetlb_vma_unlock_read(vma);
> > - mmap_read_unlock(mm);
> > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
> > + /* WARNING: VMA can't be used after this */
> > + vma_end_read(vma);
> > + } else
> > + mmap_read_unlock(mm);
>
> I also think maybe we should have a helper mm_release_fault_lock() just
> release different locks for with/without VMA_LOCK. It can also be used in
> the other patch of folio_lock_or_retry().
All seem to be good suggestions. I'll try implementing them in the
next version. Thanks!
>
> > + vmf->flags |= FAULT_FLAG_LOCK_DROPPED;
> >
> > if (likely(must_wait && !READ_ONCE(ctx->released))) {
> > wake_up_poll(&ctx->fd_wqh, EPOLLIN);
> > diff --git a/mm/memory.c b/mm/memory.c
> > index bdf46fdc58d6..923c1576bd14 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -5316,15 +5316,6 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> > if (!vma_start_read(vma))
> > goto inval;
> >
> > - /*
> > - * Due to the possibility of userfault handler dropping mmap_lock, avoid
> > - * it for now and fall back to page fault handling under mmap_lock.
> > - */
> > - if (userfaultfd_armed(vma)) {
> > - vma_end_read(vma);
> > - goto inval;
> > - }
> > -
> > /* Check since vm_start/vm_end might change before we lock the VMA */
> > if (unlikely(address < vma->vm_start || address >= vma->vm_end)) {
> > vma_end_read(vma);
> > --
> > 2.41.0.178.g377b9f9a00-goog
> >
>
> --
> Peter Xu
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@xxxxxxxxxxx.
>
