by Suren Baghdasaryan

[permalink] [raw]

Subject: Re: [PATCH v3 0/6] make vma locking more obvious

On Thu, Aug 3, 2023 at 10:26 AM Suren Baghdasaryan <[email protected]> wrote:
>
> During recent vma locking patch reviews Linus and Jann Horn noted a number
> of issues with vma locking and suggested improvements:
>
> 1. walk_page_range() does not have ability to write-lock a vma during the
> walk when it's done under mmap_write_lock. For example s390_reset_cmma().
>
> 2. Vma locking is hidden inside vm_flags modifiers and is hard to follow.
> Suggestion is to change vm_flags_reset{_once} to assert that vma is
> write-locked and require an explicit locking.
>
> 3. Same issue with vma_prepare() hiding vma locking.
>
> 4. In userfaultfd vm_flags are modified after vma->vm_userfaultfd_ctx and
> page faults can operate on a context while it's changed.
>
> 5. do_brk_flags() and __install_special_mapping() not locking a newly
> created vma before adding it into the mm. While not strictly a problem,
> this is fragile if vma is modified after insertion, as in the
> mmap_region() case which was recently fixed. Suggestion is to always lock
> a new vma before inserting it and making it visible to page faults.
>
> 6. vma_assert_write_locked() for CONFIG_PER_VMA_LOCK=n would benefit from
> being mmap_assert_write_locked() instead of no-op and then any place which
> operates on a vma and calls mmap_assert_write_locked() can be converted
> into vma_assert_write_locked().
>
> I CC'ed stable only on the first patch because others are cleanups and the
> bug in userfaultfd does not affect stable (lock_vma_under_rcu prevents
> uffds from being handled under vma lock protection). However I would be
> happy if the whole series is merged into stable 6.4 since it makes vma
> locking more maintainable.
>
> The patches apply cleanly over Linus' ToT and will conflict when applied
> over mm-unstable due to missing [1]. The conflict can be easily resolved
> by ignoring conflicting deletions but probably simpler to take [1] into
> mm-unstable and avoid later conflict.
>
> [1] commit 6c21e066f925 ("mm/mempolicy: Take VMA lock before replacing policy")
>
> Changes since v2:
> - removed vma locking from hfi1_file_mmap(), per Linus
> - moved vma locking out of dup_anon_vma(), per Liam
> - added Liam's Reviewed-by

v4 is posted at
https://lore.kernel.org/all/[email protected]/

>
> Suren Baghdasaryan (6):
> mm: enable page walking API to lock vmas during the walk
> mm: for !CONFIG_PER_VMA_LOCK equate write lock assertion for vma and
> mmap
> mm: replace mmap with vma write lock assertions when operating on a
> vma
> mm: lock vma explicitly before doing vm_flags_reset and
> vm_flags_reset_once
> mm: always lock new vma before inserting into vma tree
> mm: move vma locking out of vma_prepare and dup_anon_vma
>
> arch/powerpc/kvm/book3s_hv_uvmem.c | 1 +
> arch/powerpc/mm/book3s64/subpage_prot.c | 1 +
> arch/riscv/mm/pageattr.c | 1 +
> arch/s390/mm/gmap.c | 5 ++++
> fs/proc/task_mmu.c | 5 ++++
> fs/userfaultfd.c | 6 +++++
> include/linux/mm.h | 13 ++++++---
> include/linux/pagewalk.h | 11 ++++++++
> mm/damon/vaddr.c | 2 ++
> mm/hmm.c | 1 +
> mm/hugetlb.c | 2 +-
> mm/khugepaged.c | 5 ++--
> mm/ksm.c | 25 ++++++++++-------
> mm/madvise.c | 8 +++---
> mm/memcontrol.c | 2 ++
> mm/memory-failure.c | 1 +
> mm/memory.c | 2 +-
> mm/mempolicy.c | 22 +++++++++------
> mm/migrate_device.c | 1 +
> mm/mincore.c | 1 +
> mm/mlock.c | 4 ++-
> mm/mmap.c | 32 ++++++++++++++--------
> mm/mprotect.c | 2 ++
> mm/pagewalk.c | 36 ++++++++++++++++++++++---
> mm/vmscan.c | 1 +
> 25 files changed, 147 insertions(+), 43 deletions(-)
>
> --
> 2.41.0.585.gd2178a4bd4-goog
>