[94/95] mmap locking API: don't check locking if the mm isn't live yet

Commit Message

Andrew Morton Dec. 16, 2020, 4:47 a.m. UTC

From: Jann Horn <[e-mail]>
Subject: mmap locking API: don't check locking if the mm isn't live yet

In preparation for adding a mmap_assert_locked() check in
__get_user_pages(), teach the mmap_assert_*locked() helpers that it's fine
to operate on an mm without locking in the middle of execve() as long as
it hasn't been installed on a process yet.

Existing code paths that do this are (reverse callgraph):

  get_user_pages_remote
    get_arg_page
      copy_strings
      copy_string_kernel
      remove_arg_zero
    tomoyo_dump_page
      tomoyo_print_bprm
      tomoyo_scan_bprm
      tomoyo_environ

Link: https://lkml.kernel.org/r/[e-mail]
Signed-off-by: Jann Horn <[e-mail]>
Cc: "Eric W . Biederman" <[e-mail]>
Cc: Jason Gunthorpe <[e-mail]>
Cc: John Hubbard <[e-mail]>
Cc: Mauro Carvalho Chehab <[e-mail]>
Cc: Michel Lespinasse <[e-mail]>
Cc: Sakari Ailus <[e-mail]>
Signed-off-by: Andrew Morton <[e-mail]>
---

 fs/exec.c                 |    8 ++++++++
 include/linux/mm_types.h  |   10 ++++++++++
 include/linux/mmap_lock.h |   16 ++++++++++++----
 3 files changed, 30 insertions(+), 4 deletions(-)

Comments

Jann Horn Dec. 16, 2020, 5:07 a.m. UTC | #1

On Wed, Dec 16, 2020 at 5:47 AM Andrew Morton <[e-mail]> wrote:
> In preparation for adding a mmap_assert_locked() check in
> __get_user_pages(), teach the mmap_assert_*locked() helpers that it's fine
> to operate on an mm without locking in the middle of execve() as long as
> it hasn't been installed on a process yet.
>
> Existing code paths that do this are (reverse callgraph):
>
>   get_user_pages_remote
>     get_arg_page
>       copy_strings
>       copy_string_kernel
>       remove_arg_zero
>     tomoyo_dump_page
>       tomoyo_print_bprm
>       tomoyo_scan_bprm
>       tomoyo_environ

Sorry, can you please kill both this patch and the following one
("mm/gup: assert that the mmap lock is held in __get_user_pages()")
from the mm tree?

I'll send new stuff (a new iteration of
https://lore.kernel.org/linux-mm/[e-mail]/
"[PATCH resend v3 0/2] Broad write-locking of nascent mm in execve",
followed by a resend of "mm/gup: assert that the mmap lock is held in
__get_user_pages()") when it's ready.

As I noted in the cover letter of that thing, which was meant to
replace this patch (but isn't ready yet - yeah, I should get back to
that...), this approach doesn't really work because bprm->vma is used
after the switch to the new mm.

Sorry about the mess... :/

I guess the next time this happens, I should just immediately email
you requesting that the queued-up patches should be dropped once an
issue is identified...

Jason Gunthorpe Dec. 16, 2020, 6:08 p.m. UTC | #2

On Wed, Dec 16, 2020 at 06:07:48AM +0100, Jann Horn wrote:
> On Wed, Dec 16, 2020 at 5:47 AM Andrew Morton <[e-mail]> wrote:
> > In preparation for adding a mmap_assert_locked() check in
> > __get_user_pages(), teach the mmap_assert_*locked() helpers that it's fine
> > to operate on an mm without locking in the middle of execve() as long as
> > it hasn't been installed on a process yet.
> >
> > Existing code paths that do this are (reverse callgraph):
> >
> >   get_user_pages_remote
> >     get_arg_page
> >       copy_strings
> >       copy_string_kernel
> >       remove_arg_zero
> >     tomoyo_dump_page
> >       tomoyo_print_bprm
> >       tomoyo_scan_bprm
> >       tomoyo_environ
> 
> Sorry, can you please kill both this patch and the following one
> ("mm/gup: assert that the mmap lock is held in __get_user_pages()")
> from the mm tree?
> 
> I'll send new stuff (a new iteration of
> https://lore.kernel.org/linux-mm/[e-mail]/
> "[PATCH resend v3 0/2] Broad write-locking of nascent mm in execve",
> followed by a resend of "mm/gup: assert that the mmap lock is held in
> __get_user_pages()") when it's ready.

I'm glad you are still working on it, I think finally being able to
add lockdep to get_user_pages will be a help. I've fixed a number of
wrongly locked get_user_pages users :(

Thanks,
Jason

Patch

--- a/fs/exec.c~mmap-locking-api-dont-check-locking-if-the-mm-isnt-live-yet
+++ a/fs/exec.c
@@ -1001,6 +1001,14 @@  static int exec_mmap(struct mm_struct *m
 		}
 	}
 
+#if defined(CONFIG_LOCKDEP) || defined(CONFIG_DEBUG_VM)
+	/*
+	 * From here on, the mm may be accessed concurrently, and proper locking
+	 * is required for things like get_user_pages_remote().
+	 */
+	mm->mmap_lock_required = 1;
+#endif
+
 	task_lock(tsk);
 	membarrier_exec_mmap(mm);
 
--- a/include/linux/mmap_lock.h~mmap-locking-api-dont-check-locking-if-the-mm-isnt-live-yet
+++ a/include/linux/mmap_lock.h
@@ -161,14 +161,22 @@  static inline void mmap_read_unlock_non_
 
 static inline void mmap_assert_locked(struct mm_struct *mm)
 {
-	lockdep_assert_held(&mm->mmap_lock);
-	VM_BUG_ON_MM(!rwsem_is_locked(&mm->mmap_lock), mm);
+#if defined(CONFIG_LOCKDEP) || defined(CONFIG_DEBUG_VM)
+	if (mm->mmap_lock_required) {
+		lockdep_assert_held(&mm->mmap_lock);
+		VM_BUG_ON_MM(!rwsem_is_locked(&mm->mmap_lock), mm);
+	}
+#endif
 }
 
 static inline void mmap_assert_write_locked(struct mm_struct *mm)
 {
-	lockdep_assert_held_write(&mm->mmap_lock);
-	VM_BUG_ON_MM(!rwsem_is_locked(&mm->mmap_lock), mm);
+#if defined(CONFIG_LOCKDEP) || defined(CONFIG_DEBUG_VM)
+	if (mm->mmap_lock_required) {
+		lockdep_assert_held_write(&mm->mmap_lock);
+		VM_BUG_ON_MM(!rwsem_is_locked(&mm->mmap_lock), mm);
+	}
+#endif
 }
 
 static inline int mmap_lock_is_contended(struct mm_struct *mm)
--- a/include/linux/mm_types.h~mmap-locking-api-dont-check-locking-if-the-mm-isnt-live-yet
+++ a/include/linux/mm_types.h
@@ -561,6 +561,16 @@  struct mm_struct {
 #ifdef CONFIG_IOMMU_SUPPORT
 		u32 pasid;
 #endif
+
+#if defined(CONFIG_LOCKDEP) || defined(CONFIG_DEBUG_VM)
+		/*
+		 * Notes whether this mm has been installed on a process yet.
+		 * If not, only the task going through execve() can access this
+		 * mm, and no locking is needed around get_user_pages_remote().
+		 * This flag is only used for debug checks.
+		 */
+		bool mmap_lock_required;
+#endif
 	} __randomize_layout;
 
 	/*