[v5,6/6] mm: handle userfaults under VMA lock
Commit Message
Enable handle_userfault to operate under VMA lock by releasing VMA lock
instead of mmap_lock and retrying. Note that FAULT_FLAG_RETRY_NOWAIT
should never be used when handling faults under per-VMA lock protection
because that would break the assumption that lock is dropped on retry.
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
fs/userfaultfd.c | 34 ++++++++++++++--------------------
include/linux/mm.h | 26 ++++++++++++++++++++++++++
mm/memory.c | 20 +++++++++++---------
3 files changed, 51 insertions(+), 29 deletions(-)
Comments
On Wed, Jun 28, 2023 at 10:32 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Jun 28, 2023 at 10:25:29AM -0700, Suren Baghdasaryan wrote:
> > Enable handle_userfault to operate under VMA lock by releasing VMA lock
> > instead of mmap_lock and retrying. Note that FAULT_FLAG_RETRY_NOWAIT
> > should never be used when handling faults under per-VMA lock protection
> > because that would break the assumption that lock is dropped on retry.
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>
> Maybe the sanitize_fault_flags() changes suite more in patch 3, but not a
> big deal I guess.
IIUC FAULT_FLAG_RETRY_NOWAIT comes into play in this patchset only in
the context of uffds, therefore that check seems to be needed when we
enable per-VMA lock uffd support, which is this patch. Does that make
sense?
>
> Acked-by: Peter Xu <peterx@redhat.com>
Thanks!
>
> Thanks!
>
> --
> Peter Xu
>
On Wed, Jun 28, 2023 at 05:19:31PM -0700, Suren Baghdasaryan wrote:
> On Wed, Jun 28, 2023 at 10:32 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Wed, Jun 28, 2023 at 10:25:29AM -0700, Suren Baghdasaryan wrote:
> > > Enable handle_userfault to operate under VMA lock by releasing VMA lock
> > > instead of mmap_lock and retrying. Note that FAULT_FLAG_RETRY_NOWAIT
> > > should never be used when handling faults under per-VMA lock protection
> > > because that would break the assumption that lock is dropped on retry.
> > >
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> >
> > Maybe the sanitize_fault_flags() changes suite more in patch 3, but not a
> > big deal I guess.
>
> IIUC FAULT_FLAG_RETRY_NOWAIT comes into play in this patchset only in
> the context of uffds, therefore that check seems to be needed when we
> enable per-VMA lock uffd support, which is this patch. Does that make
> sense?
I don't see why uffd is special in this regard, as e.g. swap also checks
NOWAIT when folio_lock_or_retry() so I assume it's also used there.
IMHO the "NOWAIT should never apply with VMA_LOCK so far" assumption starts
from patch 3 where it conditionally releases the vma lock when
!(RETRY|COMPLETE); that is the real place where it can start to go wrong if
anyone breaks the assumption.
Thanks,
On Thu, Jun 29, 2023 at 9:33 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Jun 28, 2023 at 05:19:31PM -0700, Suren Baghdasaryan wrote:
> > On Wed, Jun 28, 2023 at 10:32 AM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Wed, Jun 28, 2023 at 10:25:29AM -0700, Suren Baghdasaryan wrote:
> > > > Enable handle_userfault to operate under VMA lock by releasing VMA lock
> > > > instead of mmap_lock and retrying. Note that FAULT_FLAG_RETRY_NOWAIT
> > > > should never be used when handling faults under per-VMA lock protection
> > > > because that would break the assumption that lock is dropped on retry.
> > > >
> > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > >
> > > Maybe the sanitize_fault_flags() changes suite more in patch 3, but not a
> > > big deal I guess.
> >
> > IIUC FAULT_FLAG_RETRY_NOWAIT comes into play in this patchset only in
> > the context of uffds, therefore that check seems to be needed when we
> > enable per-VMA lock uffd support, which is this patch. Does that make
> > sense?
>
> I don't see why uffd is special in this regard, as e.g. swap also checks
> NOWAIT when folio_lock_or_retry() so I assume it's also used there.
>
> IMHO the "NOWAIT should never apply with VMA_LOCK so far" assumption starts
> from patch 3 where it conditionally releases the vma lock when
> !(RETRY|COMPLETE); that is the real place where it can start to go wrong if
> anyone breaks the assumption.
Um, yes, you are right as usual. It was clear to me from the code that
NOWAIT is not used with swap under VMA_LOCK, that's why I didn't
consider this check earlier. Yeah, patch 3 seems like a more
appropriate place for it. I'll move it and post a new patchset later
today or tomorrow morning with your Acks.
Thanks,
Suren.
>
> Thanks,
>
> --
> Peter Xu
>
On Thu, Jun 29, 2023 at 9:39 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Thu, Jun 29, 2023 at 9:33 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Wed, Jun 28, 2023 at 05:19:31PM -0700, Suren Baghdasaryan wrote:
> > > On Wed, Jun 28, 2023 at 10:32 AM Peter Xu <peterx@redhat.com> wrote:
> > > >
> > > > On Wed, Jun 28, 2023 at 10:25:29AM -0700, Suren Baghdasaryan wrote:
> > > > > Enable handle_userfault to operate under VMA lock by releasing VMA lock
> > > > > instead of mmap_lock and retrying. Note that FAULT_FLAG_RETRY_NOWAIT
> > > > > should never be used when handling faults under per-VMA lock protection
> > > > > because that would break the assumption that lock is dropped on retry.
> > > > >
> > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > >
> > > > Maybe the sanitize_fault_flags() changes suite more in patch 3, but not a
> > > > big deal I guess.
> > >
> > > IIUC FAULT_FLAG_RETRY_NOWAIT comes into play in this patchset only in
> > > the context of uffds, therefore that check seems to be needed when we
> > > enable per-VMA lock uffd support, which is this patch. Does that make
> > > sense?
> >
> > I don't see why uffd is special in this regard, as e.g. swap also checks
> > NOWAIT when folio_lock_or_retry() so I assume it's also used there.
> >
> > IMHO the "NOWAIT should never apply with VMA_LOCK so far" assumption starts
> > from patch 3 where it conditionally releases the vma lock when
> > !(RETRY|COMPLETE); that is the real place where it can start to go wrong if
> > anyone breaks the assumption.
>
> Um, yes, you are right as usual. It was clear to me from the code that
> NOWAIT is not used with swap under VMA_LOCK, that's why I didn't
> consider this check earlier. Yeah, patch 3 seems like a more
> appropriate place for it. I'll move it and post a new patchset later
> today or tomorrow morning with your Acks.
Posted v6 at https://lore.kernel.org/all/20230630020436.1066016-1-surenb@google.com/
> Thanks,
> Suren.
>
> >
> > Thanks,
> >
> > --
> > Peter Xu
> >
@@ -277,17 +277,16 @@ 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);
+ assert_fault_locked(vmf);
- 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 +307,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 +322,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 +334,7 @@ static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx,
pte_t *pte;
bool ret = true;
- mmap_assert_locked(mm);
+ assert_fault_locked(vmf);
pgd = pgd_offset(mm, address);
if (!pgd_present(*pgd))
@@ -445,7 +442,7 @@ 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);
+ assert_fault_locked(vmf);
ctx = vma->vm_userfaultfd_ctx.ctx;
if (!ctx)
@@ -561,15 +558,12 @@ 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);
+ release_fault_lock(vmf);
if (likely(must_wait && !READ_ONCE(ctx->released))) {
wake_up_poll(&ctx->fd_wqh, EPOLLIN);
@@ -705,6 +705,17 @@ static inline bool vma_try_start_write(struct vm_area_struct *vma)
return true;
}
+static inline void vma_assert_locked(struct vm_area_struct *vma)
+{
+ int mm_lock_seq;
+
+ if (__is_vma_write_locked(vma, &mm_lock_seq))
+ return;
+
+ lockdep_assert_held(&vma->vm_lock->lock);
+ VM_BUG_ON_VMA(!rwsem_is_locked(&vma->vm_lock->lock), vma);
+}
+
static inline void vma_assert_write_locked(struct vm_area_struct *vma)
{
int mm_lock_seq;
@@ -731,6 +742,15 @@ static inline void release_fault_lock(struct vm_fault *vmf)
mmap_read_unlock(vmf->vma->vm_mm);
}
+static inline
+void assert_fault_locked(struct vm_fault *vmf)
+{
+ if (vmf->flags & FAULT_FLAG_VMA_LOCK)
+ vma_assert_locked(vmf->vma);
+ else
+ mmap_assert_locked(vmf->vma->vm_mm);
+}
+
#else /* CONFIG_PER_VMA_LOCK */
static inline void vma_init_lock(struct vm_area_struct *vma) {}
@@ -749,6 +769,12 @@ static inline void release_fault_lock(struct vm_fault *vmf)
mmap_read_unlock(vmf->vma->vm_mm);
}
+static inline
+void assert_fault_locked(struct vm_fault *vmf)
+{
+ mmap_assert_locked(vmf->vma->vm_mm);
+}
+
#endif /* CONFIG_PER_VMA_LOCK */
/*
@@ -5202,6 +5202,17 @@ static vm_fault_t sanitize_fault_flags(struct vm_area_struct *vma,
!is_cow_mapping(vma->vm_flags)))
return VM_FAULT_SIGSEGV;
}
+#ifdef CONFIG_PER_VMA_LOCK
+ /*
+ * Per-VMA locks can't be used with FAULT_FLAG_RETRY_NOWAIT because of
+ * the assumption that lock is dropped on VM_FAULT_RETRY.
+ */
+ if (WARN_ON_ONCE((*flags &
+ (FAULT_FLAG_VMA_LOCK | FAULT_FLAG_RETRY_NOWAIT)) ==
+ (FAULT_FLAG_VMA_LOCK | FAULT_FLAG_RETRY_NOWAIT)))
+ return VM_FAULT_SIGSEGV;
+#endif
+
return 0;
}
@@ -5294,15 +5305,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);