Re: [PATCH vfio] vfio: Use get_user_pages_longterm correctly

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

 



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);
+
        if (locked) {
                /* if VM_FAULT_RETRY can be returned, vmas become invalid */
                BUG_ON(vmas);



[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