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

[Lorenzo Stoakes]

On Mon, Oct 02, 2023 at 01:04:51PM +0200, David Hildenbrand wrote:
> 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@xxxxxxxxx>
> > ---
> >   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?

Sure it does somewhat, however there are 'ordinary' (maybe) scenarios where
this could possibly happen - FOLL_UNLOCKABLE and __get_user_pages() returns
0, or lock retained for non-FOLL_NOWAIT scenario and __get_user_pages() 0
also.

So I think the safest option might be to leave without-WARN, however you
could argue since we're making it an error now maybe we want to draw
attention to it by warning.

I just want to avoid a warning that _might_ be a product of a particular
faulting scenario.

Jason or John may have an opinion on this.

Actually having written all this, given we're changing this into an error
now anyway and this is just not a correct or expected scenario, yeah I
think WARN_ON_ONCE() would make sense, will update on v2.

>
> --
> Cheers,
>
> David / dhildenb
>