Re: [PATCH 3/4] mm/gup: make failure to pin an error if FOLL_NOWAIT not specified

[David Hildenbrand <david@redhat.com>]

On 01.10.23 18:00, Lorenzo Stoakes wrote:
> There really should be no circumstances under which a non-FOLL_NOWAIT GUP
> operation fails to return any pages, so make this an error.
> 
> To catch the trivial case, simply exit early if nr_pages == 0.
> 
> This brings __get_user_pages_locked() in line with the behaviour of its
> nommu variant.
> 
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> ---
>   mm/gup.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index b21b33d1787e..fb2218d74ca5 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1471,6 +1471,9 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
>   	long ret, pages_done;
>   	bool must_unlock = false;
>   
> +	if (!nr_pages)
> +		return 0;
> +

Probably unlikely() is reasonable. I even wonder if WARN_ON_ONCE() would 
be appropriate, but likely there are weird callers that end up calling 
this with nr_pages==0 ... probably they should be identified and 
changed. Future work.

>   	/*
>   	 * The internal caller expects GUP to manage the lock internally and the
>   	 * lock must be released when this returns.
> @@ -1595,6 +1598,14 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
>   		mmap_read_unlock(mm);
>   		*locked = 0;
>   	}
> +
> +	/*
> +	 * Failing to pin anything implies something has gone wrong except when
> +	 * FOLL_NOWAIT is specified, so explicitly make this an error.
> +	 */
> +	if (pages_done == 0 && !(flags & FOLL_NOWAIT))
> +		return -EFAULT;
> +

But who would be affected by that and why do we care about adding this 
check?

This smells like a "if (WARN_ON_ONCE())", correct?

-- 
Cheers,

David / dhildenb