On Sun, Jul 1, 2018 at 9:43 AM, Leon Romanovsky <leonro@xxxxxxxxxxxx> wrote:
> 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.
You're right, I thought for the purposes of lockdep that an exclusive
hold also means protection against new read locks, but the assertion
goes the other way and lockdep_assert_held_read() is incorrect.
