Re: [PATCH] mm/gup: honour FOLL_PIN in NOMMU __get_user_pages_locked()

["David Hildenbrand (Arm)" <david@kernel.org>]

>> It just follows what we do everywhere else (e.g., follow_page_pte()).
>>
>>
>>> +				if (try_grab_folio(page_folio(pages[i]), 1,
>>> +						   foll_flags)) {
>>> +					pages[i] = NULL;
>>> +					break;
>>> +				}
>>> +			}
>>
>> If it fails on the first iteration, we return -EFAULT instead of -ENOMEM.
>>
>> I know, I know, nobody cares. But if we touch it, we might just want to return
>> the error we get from try_grab_folio().
> 
> So just abort here and return it?  No, that will not work, there's a
> lock we would jump around.  How about something like this horrid thing,
> adding back in the relevant unlikely() to match the other calls like
> this:
> 
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index ad9ded39609c..8fa5b37be8b7 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1983,6 +1983,7 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
>  	struct vm_area_struct *vma;
>  	bool must_unlock = false;
>  	vm_flags_t vm_flags;
> +	int ret;
>  	long i;
>  
>  	if (!nr_pages)
> @@ -2019,8 +2020,15 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
>  
>  		if (pages) {
>  			pages[i] = virt_to_page((void *)start);
> -			if (pages[i])
> -				get_page(pages[i]);
> +			if (pages[i]) {
> +				ret = try_grab_folio(page_folio(pages[i]), 1,
> +						     foll_flags);
> +				if (unlikely(ret)) {
> +					pages[i] = NULL;
> +					i = ret;
> +					break;

So, we have to report the old i in case of an error.

Assuming we want to also return -EFAULT on !pages[i] -- which i think we want,
maybe the following (uncompiled and untested)?

diff --git a/mm/gup.c b/mm/gup.c
index ad9ded39609c..d00ff8d988fa 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1983,6 +1983,7 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
        struct vm_area_struct *vma;
        bool must_unlock = false;
        vm_flags_t vm_flags;
+       int ret, err = -EFAULT;
        long i;
 
        if (!nr_pages)
@@ -2019,8 +2020,14 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
 
                if (pages) {
                        pages[i] = virt_to_page((void *)start);
-                       if (pages[i])
-                               get_page(pages[i]);
+                       if (!pages[i])
+                               break;
+                       ret = try_grab_folio(page_folio(pages[i]), 1, foll_flags);
+                       if (ret) {
+                               pages[i] = NULL;
+                               err = ret;
+                               break
+                       }
                }
 
                start = (start + PAGE_SIZE) & PAGE_MASK;
@@ -2031,7 +2038,7 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
                *locked = 0;
        }
 
-       return i ? : -EFAULT;
+       return i ? : err;
 }
 #endif /* !CONFIG_MMU */


-- 
Cheers,

David