Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230801220733.1987762-1-surenb@google.com>
In-Reply-To: <20230801220733.1987762-1-surenb@google.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Thu, 3 Aug 2023 10:28:32 -0700
Message-ID: <CAJuCfpEow+uz5tGgV2y=bAAh2tdwjcqv+hUz0Om68usLaw0jsA@mail.gmail.com>
Subject: Re: [PATCH v2 0/6] make vma locking more obvious
To:     akpm@linux-foundation.org
Cc:     torvalds@linux-foundation.org, jannh@google.com,
        willy@infradead.org, liam.howlett@oracle.com, david@redhat.com,
        peterx@redhat.com, ldufour@linux.ibm.com, vbabka@suse.cz,
        michel@lespinasse.org, jglisse@google.com, mhocko@suse.com,
        hannes@cmpxchg.org, dave@stgolabs.net, hughd@google.com,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Aug 1, 2023 at 3:07=E2=80=AFPM Suren Baghdasaryan <surenb@google.co=
m> wrote:
>
> During recent vma locking patch reviews Linus and Jann Horn noted a numbe=
r
> 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=3Dn would benefit fr=
om
> being mmap_assert_write_locked() instead of no-op and then any place whic=
h
> 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 th=
e
> 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 po=
licy")
>
> Changes since v1:
> - replace walk_page_range() parameter with mm_walk_ops.walk_lock,
> per Linus
> - introduced page_walk_lock enum to allow different locking modes
> during a walk, per Linus
> - added Liam's Reviewed-by

v3 is posted at
https://lore.kernel.org/all/20230803172652.2849981-1-surenb@google.com/

>
> 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
>
>  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 ++++
>  drivers/infiniband/hw/hfi1/file_ops.c   |  1 +
>  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                               | 29 +++++++++++++-------
>  mm/mprotect.c                           |  2 ++
>  mm/pagewalk.c                           | 36 ++++++++++++++++++++++---
>  mm/vmscan.c                             |  1 +
>  26 files changed, 146 insertions(+), 42 deletions(-)
>
> --
> 2.41.0.585.gd2178a4bd4-goog
>