Re: [PATCH 4/8] mm/gup: add an assertion that the mmap lock is locked

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Jan 19, 2023 at 07:08:08PM -0800, John Hubbard wrote:
> On 1/17/23 07:58, Jason Gunthorpe wrote:
> > This is always required, but we can't have a proper unguarded assertion
> > because of a shortcut in fork. So, cover as much as we can for now.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >   mm/gup.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 9e332e3f6ea8e2..d203e268793b9c 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -1350,6 +1350,14 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
> >   			return -EAGAIN;
> >   		lock_dropped = true;
> >   		*locked = 1;
> > +	} else if (flags & FOLL_PIN) {
> > +		/*
> > +		 * The mmap lock must be held when calling this function. This
> > +		 * is true even for non-pin modes, but due to a shortcut in fork
> > +		 * not taking the lock for the new mm we cannot check this
> > +		 * comprehensively.
> 
> I get that fork doesn't lock the newly created mm. But I'm having
> trouble finding the calling path from fork to __get_user_pages_locked()
> that leads to this comment, can you provide a hint? Both the comment
> and the commit log are rather coy about where this happens.

Hurm, so I see this did get fixed but nobody added the assertion to
gup.c :(

Jann Horn was working on this here:

https://lore.kernel.org/linux-mm/CAG48ez1-PBCdv3y8pn-Ty-b+FmBSLwDuVKFSt8h7wARLy0dF-Q@mail.gmail.com/

However, there was never any note on the mailing list what happened to
the series..

But Andrew did take:
  b2767d97f5ff ("binfmt_elf: take the mmap lock around find_extend_vma()")
  f3964599c22f ("mm/gup_benchmark: take the mmap lock around GUP")

Then we had this other work:
  5b78ed24e8ec ("mm/pagemap: add mmap_assert_locked() annotations to find_vma*()")

Which actually added the assertion to find_vma(), which is called
unconditionally by gup.c:

 __get_user_pages_locked()
  __get_user_pages()
  find_extend_vma()
   find_vma()

So we've already had the assertion for a while now.

I'll update this commit to make it unconditional and note that it
already exists.

Thanks,
Jason