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
