On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe <jgg@xxxxxxxx> 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.
