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
Signed-off-by: Jann Horn <[email protected]>
---
fs/exec.c | 8 ++++++++
include/linux/mm_types.h | 9 +++++++++
include/linux/mmap_lock.h | 16 ++++++++++++----
3 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index a91003e28eaa..c02b0e8e1c0b 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1129,6 +1129,14 @@ static int exec_mmap(struct mm_struct *mm)
}
}
+#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);
active_mm = tsk->active_mm;
membarrier_exec_mmap(mm);
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index ed028af3cb19..89fee0d0d652 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -552,6 +552,15 @@ struct mm_struct {
atomic_long_t hugetlb_usage;
#endif
struct work_struct async_put_work;
+#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;
/*
diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
index 0707671851a8..c4fd874954d7 100644
--- a/include/linux/mmap_lock.h
+++ b/include/linux/mmap_lock.h
@@ -77,14 +77,22 @@ static inline void
mmap_read_unlock_non_owner(struct mm_struct *mm)
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
}
#endif /* _LINUX_MMAP_LOCK_H */
--
2.28.0.709.gb0816b6eb0-goog
On Tue, Sep 29, 2020 at 06:20:00PM -0700, Jann Horn 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.
I'm happy to see lockdep being added here, but can you elaborate on
why add this mmap_locked_required instead of obtaining the lock in the
execv path?
Jason
On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <[email protected]> wrote:
> On Tue, Sep 29, 2020 at 06:20:00PM -0700, Jann Horn 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.
>
> I'm happy to see lockdep being added here, but can you elaborate on
> why add this mmap_locked_required instead of obtaining the lock in the
> execv path?
My thinking was: At that point, we're logically still in the
single-owner initialization phase of the mm_struct. Almost any object
has initialization and teardown steps that occur in a context where
the object only has a single owner, and therefore no locking is
required. It seems to me that adding locking in places like
get_arg_page() would be confusing because it would suggest the
existence of concurrency where there is no actual concurrency, and it
might be annoying in terms of lockdep if someone tries to use
something like get_arg_page() while holding the mmap_sem of the
calling process. It would also mean that we'd be doing extra locking
in normal kernel builds that isn't actually logically required.
Hmm, on the other hand, dup_mmap() already locks the child mm (with
mmap_write_lock_nested()), so I guess it wouldn't be too bad to also
do it in get_arg_page() and tomoyo_dump_page(), with comments that
note that we're doing this for lockdep consistency... I guess I can go
change this in v2.
On Wed, Sep 30, 2020 at 2:50 PM Jann Horn <[email protected]> wrote:
> On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <[email protected]> wrote:
> > On Tue, Sep 29, 2020 at 06:20:00PM -0700, Jann Horn 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.
> >
> > I'm happy to see lockdep being added here, but can you elaborate on
> > why add this mmap_locked_required instead of obtaining the lock in the
> > execv path?
>
> My thinking was: At that point, we're logically still in the
> single-owner initialization phase of the mm_struct. Almost any object
> has initialization and teardown steps that occur in a context where
> the object only has a single owner, and therefore no locking is
> required. It seems to me that adding locking in places like
> get_arg_page() would be confusing because it would suggest the
> existence of concurrency where there is no actual concurrency, and it
> might be annoying in terms of lockdep if someone tries to use
> something like get_arg_page() while holding the mmap_sem of the
> calling process. It would also mean that we'd be doing extra locking
> in normal kernel builds that isn't actually logically required.
>
> Hmm, on the other hand, dup_mmap() already locks the child mm (with
> mmap_write_lock_nested()), so I guess it wouldn't be too bad to also
> do it in get_arg_page() and tomoyo_dump_page(), with comments that
> note that we're doing this for lockdep consistency... I guess I can go
> change this in v2.
Actually, I'm taking that back. There's an extra problem:
get_arg_page() accesses bprm->vma, which is set all the way back in
__bprm_mm_init(). We really shouldn't be pretending that we're
properly taking the mmap_sem when actually, we keep reusing a
vm_area_struct pointer.
So for that reason I prefer the approach in the existing patch, where
we make it clear that mm_struct has two different lifetime phases in
which GUP works, and that those lifetime phases have very different
locking requirements.
Does that sound reasonable?
On Wed, Sep 30, 2020 at 10:14:57PM +0200, Jann Horn wrote:
> On Wed, Sep 30, 2020 at 2:50 PM Jann Horn <[email protected]> wrote:
> > On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <[email protected]> wrote:
> > > On Tue, Sep 29, 2020 at 06:20:00PM -0700, Jann Horn 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.
> > >
> > > I'm happy to see lockdep being added here, but can you elaborate on
> > > why add this mmap_locked_required instead of obtaining the lock in the
> > > execv path?
> >
> > My thinking was: At that point, we're logically still in the
> > single-owner initialization phase of the mm_struct. Almost any object
> > has initialization and teardown steps that occur in a context where
> > the object only has a single owner, and therefore no locking is
> > required. It seems to me that adding locking in places like
> > get_arg_page() would be confusing because it would suggest the
> > existence of concurrency where there is no actual concurrency, and it
> > might be annoying in terms of lockdep if someone tries to use
> > something like get_arg_page() while holding the mmap_sem of the
> > calling process. It would also mean that we'd be doing extra locking
> > in normal kernel builds that isn't actually logically required.
> >
> > Hmm, on the other hand, dup_mmap() already locks the child mm (with
> > mmap_write_lock_nested()), so I guess it wouldn't be too bad to also
> > do it in get_arg_page() and tomoyo_dump_page(), with comments that
> > note that we're doing this for lockdep consistency... I guess I can go
> > change this in v2.
>
> Actually, I'm taking that back. There's an extra problem:
> get_arg_page() accesses bprm->vma, which is set all the way back in
> __bprm_mm_init(). We really shouldn't be pretending that we're
> properly taking the mmap_sem when actually, we keep reusing a
> vm_area_struct pointer.
Any chance the mmap lock can just be held from mm_struct allocation
till exec inserts it into the process?
> Does that sound reasonable?
My only concern is how weird it is to do this with a variable, I've
never seen something like this before
Jason
On Thu, Oct 1, 2020 at 1:26 AM Jason Gunthorpe <[email protected]> wrote:
> On Wed, Sep 30, 2020 at 10:14:57PM +0200, Jann Horn wrote:
> > On Wed, Sep 30, 2020 at 2:50 PM Jann Horn <[email protected]> wrote:
> > > On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <[email protected]> wrote:
> > > > On Tue, Sep 29, 2020 at 06:20:00PM -0700, Jann Horn 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.
> > > >
> > > > I'm happy to see lockdep being added here, but can you elaborate on
> > > > why add this mmap_locked_required instead of obtaining the lock in the
> > > > execv path?
> > >
> > > My thinking was: At that point, we're logically still in the
> > > single-owner initialization phase of the mm_struct. Almost any object
> > > has initialization and teardown steps that occur in a context where
> > > the object only has a single owner, and therefore no locking is
> > > required. It seems to me that adding locking in places like
> > > get_arg_page() would be confusing because it would suggest the
> > > existence of concurrency where there is no actual concurrency, and it
> > > might be annoying in terms of lockdep if someone tries to use
> > > something like get_arg_page() while holding the mmap_sem of the
> > > calling process. It would also mean that we'd be doing extra locking
> > > in normal kernel builds that isn't actually logically required.
> > >
> > > Hmm, on the other hand, dup_mmap() already locks the child mm (with
> > > mmap_write_lock_nested()), so I guess it wouldn't be too bad to also
> > > do it in get_arg_page() and tomoyo_dump_page(), with comments that
> > > note that we're doing this for lockdep consistency... I guess I can go
> > > change this in v2.
> >
> > Actually, I'm taking that back. There's an extra problem:
> > get_arg_page() accesses bprm->vma, which is set all the way back in
> > __bprm_mm_init(). We really shouldn't be pretending that we're
> > properly taking the mmap_sem when actually, we keep reusing a
> > vm_area_struct pointer.
>
> Any chance the mmap lock can just be held from mm_struct allocation
> till exec inserts it into the process?
Hm... it should work if we define a lockdep subclass for this so that
lockdep is happy when we call get_user() on the old mm_struct while
holding that mmap lock.
> > Does that sound reasonable?
>
> My only concern is how weird it is to do this with a variable, I've
> never seen something like this before
It seems clearer to me this way than taking locks when there is no
concurrency that we actually need to guard against. But since both you
and Michel seem to hate it, I'll go and code up the version with a
lockdep subclass. Under protest. :P
On Wed, Sep 30, 2020 at 1:15 PM Jann Horn <[email protected]> wrote:
> On Wed, Sep 30, 2020 at 2:50 PM Jann Horn <[email protected]> wrote:
> > On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <[email protected]> wrote:
> > > On Tue, Sep 29, 2020 at 06:20:00PM -0700, Jann Horn 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.
> > >
> > > I'm happy to see lockdep being added here, but can you elaborate on
> > > why add this mmap_locked_required instead of obtaining the lock in the
> > > execv path?
> >
> > My thinking was: At that point, we're logically still in the
> > single-owner initialization phase of the mm_struct. Almost any object
> > has initialization and teardown steps that occur in a context where
> > the object only has a single owner, and therefore no locking is
> > required. It seems to me that adding locking in places like
> > get_arg_page() would be confusing because it would suggest the
> > existence of concurrency where there is no actual concurrency, and it
> > might be annoying in terms of lockdep if someone tries to use
> > something like get_arg_page() while holding the mmap_sem of the
> > calling process. It would also mean that we'd be doing extra locking
> > in normal kernel builds that isn't actually logically required.
> >
> > Hmm, on the other hand, dup_mmap() already locks the child mm (with
> > mmap_write_lock_nested()), so I guess it wouldn't be too bad to also
> > do it in get_arg_page() and tomoyo_dump_page(), with comments that
> > note that we're doing this for lockdep consistency... I guess I can go
> > change this in v2.
>
> Actually, I'm taking that back. There's an extra problem:
> get_arg_page() accesses bprm->vma, which is set all the way back in
> __bprm_mm_init(). We really shouldn't be pretending that we're
> properly taking the mmap_sem when actually, we keep reusing a
> vm_area_struct pointer.
>
> So for that reason I prefer the approach in the existing patch, where
> we make it clear that mm_struct has two different lifetime phases in
> which GUP works, and that those lifetime phases have very different
> locking requirements.
>
> Does that sound reasonable?
I'm really not a fan of adding such exceptions; I think it's both
unusual and adds complexity that is not strictly contained into the
init paths.
I don't really understand the concern with the bprm vma in
get_arg_page(); I'm not super familiar with this code but isn't it a
normal vma within the process that __do_execve_file() is creating ? I
received Jason's last email while I was composing this one, but I
think I have the same concern/approach as him, i.e. I think it would
be simplest to keep the new MM locked through the __do_execve_file()
call and avoid adding the mmap_lock_required exception to the
mmap_assert_locked rule.
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
On Thu, Oct 01, 2020 at 01:51:33AM +0200, Jann Horn wrote:
> On Thu, Oct 1, 2020 at 1:26 AM Jason Gunthorpe <[email protected]> wrote:
> > On Wed, Sep 30, 2020 at 10:14:57PM +0200, Jann Horn wrote:
> > > On Wed, Sep 30, 2020 at 2:50 PM Jann Horn <[email protected]> wrote:
> > > > On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <[email protected]> wrote:
> > > > > On Tue, Sep 29, 2020 at 06:20:00PM -0700, Jann Horn 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.
> > > > >
> > > > > I'm happy to see lockdep being added here, but can you elaborate on
> > > > > why add this mmap_locked_required instead of obtaining the lock in the
> > > > > execv path?
> > > >
> > > > My thinking was: At that point, we're logically still in the
> > > > single-owner initialization phase of the mm_struct. Almost any object
> > > > has initialization and teardown steps that occur in a context where
> > > > the object only has a single owner, and therefore no locking is
> > > > required. It seems to me that adding locking in places like
> > > > get_arg_page() would be confusing because it would suggest the
> > > > existence of concurrency where there is no actual concurrency, and it
> > > > might be annoying in terms of lockdep if someone tries to use
> > > > something like get_arg_page() while holding the mmap_sem of the
> > > > calling process. It would also mean that we'd be doing extra locking
> > > > in normal kernel builds that isn't actually logically required.
> > > >
> > > > Hmm, on the other hand, dup_mmap() already locks the child mm (with
> > > > mmap_write_lock_nested()), so I guess it wouldn't be too bad to also
> > > > do it in get_arg_page() and tomoyo_dump_page(), with comments that
> > > > note that we're doing this for lockdep consistency... I guess I can go
> > > > change this in v2.
> > >
> > > Actually, I'm taking that back. There's an extra problem:
> > > get_arg_page() accesses bprm->vma, which is set all the way back in
> > > __bprm_mm_init(). We really shouldn't be pretending that we're
> > > properly taking the mmap_sem when actually, we keep reusing a
> > > vm_area_struct pointer.
> >
> > Any chance the mmap lock can just be held from mm_struct allocation
> > till exec inserts it into the process?
>
> Hm... it should work if we define a lockdep subclass for this so that
> lockdep is happy when we call get_user() on the old mm_struct while
> holding that mmap lock.
A subclass isn't right, it has to be a _nested annotation.
nested locking is a pretty good reason to not be able to do this, this
is something lockdep does struggle to model.
Jason
On Thu, Oct 1, 2020 at 9:15 PM Jason Gunthorpe <[email protected]> wrote:
> On Thu, Oct 01, 2020 at 01:51:33AM +0200, Jann Horn wrote:
> > On Thu, Oct 1, 2020 at 1:26 AM Jason Gunthorpe <[email protected]> wrote:
> > > On Wed, Sep 30, 2020 at 10:14:57PM +0200, Jann Horn wrote:
> > > > On Wed, Sep 30, 2020 at 2:50 PM Jann Horn <[email protected]> wrote:
> > > > > On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <[email protected]> wrote:
> > > > > > On Tue, Sep 29, 2020 at 06:20:00PM -0700, Jann Horn 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.
> > > > > >
> > > > > > I'm happy to see lockdep being added here, but can you elaborate on
> > > > > > why add this mmap_locked_required instead of obtaining the lock in the
> > > > > > execv path?
> > > > >
> > > > > My thinking was: At that point, we're logically still in the
> > > > > single-owner initialization phase of the mm_struct. Almost any object
> > > > > has initialization and teardown steps that occur in a context where
> > > > > the object only has a single owner, and therefore no locking is
> > > > > required. It seems to me that adding locking in places like
> > > > > get_arg_page() would be confusing because it would suggest the
> > > > > existence of concurrency where there is no actual concurrency, and it
> > > > > might be annoying in terms of lockdep if someone tries to use
> > > > > something like get_arg_page() while holding the mmap_sem of the
> > > > > calling process. It would also mean that we'd be doing extra locking
> > > > > in normal kernel builds that isn't actually logically required.
> > > > >
> > > > > Hmm, on the other hand, dup_mmap() already locks the child mm (with
> > > > > mmap_write_lock_nested()), so I guess it wouldn't be too bad to also
> > > > > do it in get_arg_page() and tomoyo_dump_page(), with comments that
> > > > > note that we're doing this for lockdep consistency... I guess I can go
> > > > > change this in v2.
> > > >
> > > > Actually, I'm taking that back. There's an extra problem:
> > > > get_arg_page() accesses bprm->vma, which is set all the way back in
> > > > __bprm_mm_init(). We really shouldn't be pretending that we're
> > > > properly taking the mmap_sem when actually, we keep reusing a
> > > > vm_area_struct pointer.
> > >
> > > Any chance the mmap lock can just be held from mm_struct allocation
> > > till exec inserts it into the process?
> >
> > Hm... it should work if we define a lockdep subclass for this so that
> > lockdep is happy when we call get_user() on the old mm_struct while
> > holding that mmap lock.
>
> A subclass isn't right, it has to be a _nested annotation.
>
> nested locking is a pretty good reason to not be able to do this, this
> is something lockdep does struggle to model.
Did I get the terminology wrong? I thought they were the same. The
down_*_nested() APIs take an argument "subclass", with the default
subclass for the functions without "_nested" being 0.
Anyway, I wrote a patch for this yesterday, I'll send it out later
today after testing that it still boots without lockdep warnings. Then
you can decide whether you prefer it to the current patch.
On Thu, Oct 01, 2020 at 10:16:35PM +0200, Jann Horn wrote:
> > A subclass isn't right, it has to be a _nested annotation.
> >
> > nested locking is a pretty good reason to not be able to do this, this
> > is something lockdep does struggle to model.
>
> Did I get the terminology wrong? I thought they were the same. The
> down_*_nested() APIs take an argument "subclass", with the default
> subclass for the functions without "_nested" being 0.
AFAIK a subclass at init time sticks with the lock forever, the
_nested ones are temporary overrides.
I think what you kind of want is to start out with
lockdep_set_novalidate_class() then switch to a real class once things
are finished. Not sure exactly how :)
Jason
On Fri, Oct 2, 2020 at 1:41 AM Jason Gunthorpe <[email protected]> wrote:
> On Thu, Oct 01, 2020 at 10:16:35PM +0200, Jann Horn wrote:
> > > A subclass isn't right, it has to be a _nested annotation.
> > >
> > > nested locking is a pretty good reason to not be able to do this, this
> > > is something lockdep does struggle to model.
> >
> > Did I get the terminology wrong? I thought they were the same. The
> > down_*_nested() APIs take an argument "subclass", with the default
> > subclass for the functions without "_nested" being 0.
>
> AFAIK a subclass at init time sticks with the lock forever, the
> _nested ones are temporary overrides.
>
> I think what you kind of want is to start out with
> lockdep_set_novalidate_class() then switch to a real class once things
> are finished. Not sure exactly how :)
Huh, is there an API that sets a *subclass* (not a class) at init
time? I don't think there is.
Anyway, I'm pretty sure I just need to use the normal _nested()
locking API. I'm still cleaning up and testing a little bit, but I'll
send it out in a short while, unless I run into unexpected trouble.
Let's continue this if necessary once there's a concrete patch to talk
about. :)