On Sun, Jul 01, 2018 at 09:10:57AM -0700, Dan Williams wrote:
> On Sat, Jun 30, 2018 at 1:17 PM, Alex Williamson
> <alex.williamson@xxxxxxxxxx> wrote:
> > On Fri, 29 Jun 2018 11:31:50 -0600
> > Jason Gunthorpe <jgg@xxxxxxxxxxxx> wrote:
> >
> >> The patch noted in the fixes below converted get_user_pages_fast() to
> >> get_user_pages_longterm(), however the two calls differ in a few ways.
> >>
> >> First _fast() is documented to not require the mmap_sem, while _longterm()
> >> is documented to need it. Hold the mmap sem as required.
> >>
> >> Second, _fast accepts an 'int write' while _longterm uses 'unsigned int
> >> gup_flags', so the expression '!!(prot & IOMMU_WRITE)' is only working by
> >> luck as FOLL_WRITE is currently == 0x1. Use the expected FOLL_WRITE
> >> constant instead.
> >>
> >> Fixes: 94db151dc892 ("vfio: disable filesystem-dax page pinning")
> >> Cc: <stable@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> >> ---
> >> drivers/vfio/vfio_iommu_type1.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > Minor change as shown below, we don't need both branches coming up with
> > the FOLL_WRITE flag in slightly different ways.
> >
> >> I noticed this while trying to review some RDMA code that was touching
> >> our get_user_pages_longterm() call site and wanted to see what others
> >> are doing..
> >>
> >> If someone can explain that get_user_pages_longterm() is safe to call
> >> without the mmap_sem held I'd love to here it!
> >
> > Me too :-\
> >
> >> The comments in gup.c do seem to pretty clearly state the
> >> __get_user_pages_locked() called internally by
> >> get_user_pages_longterm() needs mmap_sem held..
> >>
> >> This is confusing me because this is the only
> >> get_user_pages_longterm() callsite that doesn't hold the mmap_sem, and
> >> if it really isn't required I'd like to remove it from the RDMA code
> >> as well :)
> >
> > commit 0e81a8fc0411c9baec88f3f65154285fede473f6
> > Author: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > Date: Fri Jun 29 11:31:50 2018 -0600
> >
> > vfio: Use get_user_pages_longterm correctly
> >
> > The patch noted in the fixes below converted get_user_pages_fast() to
> > get_user_pages_longterm(), however the two calls differ in a few ways.
> >
> > First _fast() is documented to not require the mmap_sem, while _longterm()
> > is documented to need it. Hold the mmap sem as required.
> >
> > Second, _fast accepts an 'int write' while _longterm uses 'unsigned int
> > gup_flags', so the expression '!!(prot & IOMMU_WRITE)' is only working by
> > luck as FOLL_WRITE is currently == 0x1. Use the expected FOLL_WRITE
> > constant instead.
> >
> > Fixes: 94db151dc892 ("vfio: disable filesystem-dax page pinning")
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
>
> Ugh, yes.
>
> Acked-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> I think we also need the following clue bat for people like me in the future:
>
> diff --git a/mm/gup.c b/mm/gup.c
> index b70d7ba7cc13..388a5c799fa5 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -873,6 +873,8 @@ static __always_inline long
> __get_user_pages_locked(struct task_struct *ts
> long ret, pages_done;
> bool lock_dropped;
>
> + lockdep_assert_held_read(&mm->mmap_sem);
> +
It should be "lockdep_assert_held(&mm->mmap_sem);" and not as you wrote.
There is nothing wrong in holding exclusive lock while accessing gup.
> if (locked) {
> /* if VM_FAULT_RETRY can be returned, vmas become invalid */
> BUG_ON(vmas);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
