| Message ID | 20230628172529.744839-7-surenb@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Per-VMA lock support for swap and userfaults | expand |
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. Acked-by: Peter Xu <peterx@redhat.com> Thanks!
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 > >
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 4e800bb7d2ab..9d61e3e7da7b 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -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); diff --git a/include/linux/mm.h b/include/linux/mm.h index bbaec479bf98..cd5389338def 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -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 */ /* diff --git a/mm/memory.c b/mm/memory.c index 4fb8ecfc6d13..672f7383a622 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -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);
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(-)