Re: [PATCH vfio] vfio: Use get_user_pages_longterm correctly

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux