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);
