| Message ID | 4-v1-dd94f8f0d5ad+716-gup_tidy_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Simplify the external interface for GUP | expand |
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. > + */ > + mmap_assert_locked(mm); > } > > if (flags & FOLL_PIN) thanks,
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
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. + */ + mmap_assert_locked(mm); } if (flags & FOLL_PIN)
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(+)