Re: [PATCH rc] mm/gup: use unpin_user_pages() in check_and_migrate_cma_pages()

[John Hubbard <jhubbard@nvidia.com>]

On 10/30/20 1:50 PM, Ira Weiny wrote:
> On Fri, Oct 30, 2020 at 01:32:28PM -0300, Jason Gunthorpe wrote:
>> When FOLL_PIN is passed to __get_user_pages() the page list must be put
>> back using unpin_user_pages() otherwise the page pin reference persists in
>> a corrupted state.
>>
>> Cc: <stable@kernel.org>
>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>> ---
>>   mm/gup.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> I don't have any way to test CMA stuff, this was noticed by inspection.
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 1bb349e5ed212a..2e26757f3c9276 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -1630,8 +1630,11 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm,
>>   		/*
>>   		 * drop the above get_user_pages reference.
>>   		 */
> 
> I wonder if that comment should be deleted/modified?  It does not seem to apply
> any longer.

Modify, maybe. But it's still relevant: the pages came from either gup or pup.

> 
> Also, looks like there is another place this occurs right before the call to
> check_and_migrate_cma_pages() in __gup_longterm_locked()
> 
> 1730                 if (check_dax_vmas(vmas_tmp, rc)) {
> 1731                         for (i = 0; i < rc; i++)
> 1732                                 put_page(pages[i]);
> 1733                         rc = -EOPNOTSUPP;
> 1734                         goto out;
> 1735                 }
> 
> We should change that site as well.
> 
> And since we now have 2 places should this be a helper?
> 

+1


thanks,
-- 
John Hubbard
NVIDIA

> Ira
> 
>> -		for (i = 0; i < nr_pages; i++)
>> -			put_page(pages[i]);
>> +		if (gup_flags & FOLL_PIN)
>> +			unpin_user_pages(pages, nr_pages);
>> +		else
>> +			for (i = 0; i < nr_pages; i++)
>> +				put_page(pages[i]);
>>   
>>   		if (migrate_pages(&cma_page_list, alloc_migration_target, NULL,
>>   			(unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) {
>> -- 
>> 2.28.0
>>