diff mbox series

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

Message ID c7bfaf30cb682b92766e35ec85d93a84798b37f4.1696174961.git.lstoakes@gmail.com (mailing list archive)
State New
Headers show
Series various improvements to the GUP interface | expand

Commit Message

Lorenzo Stoakes Oct. 1, 2023, 4 p.m. UTC
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(+)

Comments

David Hildenbrand Oct. 2, 2023, 11:04 a.m. UTC | #1
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?
Lorenzo Stoakes Oct. 2, 2023, 10:51 p.m. UTC | #2
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@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?

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
>
Jason Gunthorpe Oct. 9, 2023, 10:22 p.m. UTC | #3
On Mon, Oct 02, 2023 at 11:51:04PM +0100, Lorenzo Stoakes wrote:

> > > 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.

Ideally the subfunctions would never return 0 when they are not
supposed to return zero and this would be a warn on to try to enforce
that.

There should be a clear limited set of flags where the caller is
expected to handle a 0 return - and those flags should have guidance
what the caller should do to handle it..

Jason
diff mbox series

Patch

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;
+
 	/*
 	 * 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;
+
 	return pages_done;
 }