| Message ID | CAG48ez03YJG9JU_6tGiMcaVjuTyRE_o4LEQ7901b5ZoCnNAjcg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] mm/gup_benchmark: Take the mmap lock around GUP | expand |
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 <jgg@ziepe.ca> 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 <jannh@google.com> wrote: > On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <jgg@ziepe.ca> 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 <jannh@google.com> wrote: > > On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <jgg@ziepe.ca> 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 Wed, Sep 30, 2020 at 1:15 PM Jann Horn <jannh@google.com> wrote: > On Wed, Sep 30, 2020 at 2:50 PM Jann Horn <jannh@google.com> wrote: > > On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <jgg@ziepe.ca> 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.
On Thu, Oct 1, 2020 at 1:26 AM Jason Gunthorpe <jgg@ziepe.ca> 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 <jannh@google.com> wrote: > > > On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <jgg@ziepe.ca> 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 Thu, Oct 01, 2020 at 01:51:33AM +0200, Jann Horn wrote: > On Thu, Oct 1, 2020 at 1:26 AM Jason Gunthorpe <jgg@ziepe.ca> 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 <jannh@google.com> wrote: > > > > On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <jgg@ziepe.ca> 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 <jgg@ziepe.ca> 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 <jgg@ziepe.ca> 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 <jannh@google.com> wrote: > > > > > On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <jgg@ziepe.ca> 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 <jgg@ziepe.ca> 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. :)
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 }
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 <jannh@google.com> --- fs/exec.c | 8 ++++++++ include/linux/mm_types.h | 9 +++++++++ include/linux/mmap_lock.h | 16 ++++++++++++---- 3 files changed, 29 insertions(+), 4 deletions(-) #endif /* _LINUX_MMAP_LOCK_H */