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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxx/
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
