| Message ID | c7bfaf30cb682b92766e35ec85d93a84798b37f4.1696174961.git.lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | various improvements to the GUP interface | expand |
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?
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 >
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 --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; }
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(+)