* (no subject) @ 2022-04-21 16:41 Yury Norov 2022-04-21 23:04 ` John Hubbard 0 siblings, 1 reply; 5+ messages in thread From: Yury Norov @ 2022-04-21 16:41 UTC (permalink / raw) To: Andrew Morton, Minchan Kim, John Hubbard, linux-mm, linux-kernel Cc: Yury Norov Subject: [PATCH] mm/gup: fix comments to pin_user_pages_*() pin_user_pages API forces FOLL_PIN in gup_flags, which means that the API requires struct page **pages to be provided (not NULL). However, the comment to pin_user_pages() says: * @pages: array that receives pointers to the pages pinned. * Should be at least nr_pages long. Or NULL, if caller * only intends to ensure the pages are faulted in. This patch fixes comments along the pin_user_pages code, and also adds WARN_ON(!pages), so that API users will have better understanding on how to use it. It has been independently spotted by Minchan Kim and confirmed with John Hubbard: https://lore.kernel.org/all/YgWA0ghrrzHONehH@google.com/ Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com> --- mm/gup.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/mm/gup.c b/mm/gup.c index f598a037eb04..559626457585 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -2871,6 +2871,10 @@ int pin_user_pages_fast(unsigned long start, int nr_pages, if (WARN_ON_ONCE(gup_flags & FOLL_GET)) return -EINVAL; + /* FOLL_PIN requires pages != NULL */ + if (WARN_ON_ONCE(!pages)) + return -EINVAL; + gup_flags |= FOLL_PIN; return internal_get_user_pages_fast(start, nr_pages, gup_flags, pages); } @@ -2893,6 +2897,10 @@ int pin_user_pages_fast_only(unsigned long start, int nr_pages, */ if (WARN_ON_ONCE(gup_flags & FOLL_GET)) return 0; + + /* FOLL_PIN requires pages != NULL */ + if (WARN_ON_ONCE(!pages)) + return 0; /* * FOLL_FAST_ONLY is required in order to match the API description of * this routine: no fall back to regular ("slow") GUP. @@ -2920,8 +2928,7 @@ EXPORT_SYMBOL_GPL(pin_user_pages_fast_only); * @nr_pages: number of pages from start to pin * @gup_flags: flags modifying lookup behaviour * @pages: array that receives pointers to the pages pinned. - * Should be at least nr_pages long. Or NULL, if caller - * only intends to ensure the pages are faulted in. + * Should be at least nr_pages long. * @vmas: array of pointers to vmas corresponding to each page. * Or NULL if the caller does not require them. * @locked: pointer to lock flag indicating whether lock is held and @@ -2944,6 +2951,10 @@ long pin_user_pages_remote(struct mm_struct *mm, if (WARN_ON_ONCE(gup_flags & FOLL_GET)) return -EINVAL; + /* FOLL_PIN requires pages != NULL */ + if (WARN_ON_ONCE(!pages)) + return -EINVAL; + gup_flags |= FOLL_PIN; return __get_user_pages_remote(mm, start, nr_pages, gup_flags, pages, vmas, locked); @@ -2957,8 +2968,7 @@ EXPORT_SYMBOL(pin_user_pages_remote); * @nr_pages: number of pages from start to pin * @gup_flags: flags modifying lookup behaviour * @pages: array that receives pointers to the pages pinned. - * Should be at least nr_pages long. Or NULL, if caller - * only intends to ensure the pages are faulted in. + * Should be at least nr_pages long. * @vmas: array of pointers to vmas corresponding to each page. * Or NULL if the caller does not require them. * @@ -2976,6 +2986,10 @@ long pin_user_pages(unsigned long start, unsigned long nr_pages, if (WARN_ON_ONCE(gup_flags & FOLL_GET)) return -EINVAL; + /* FOLL_PIN requires pages != NULL */ + if (WARN_ON_ONCE(!pages)) + return -EINVAL; + gup_flags |= FOLL_PIN; return __gup_longterm_locked(current->mm, start, nr_pages, pages, vmas, gup_flags); @@ -2994,6 +3008,10 @@ long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, if (WARN_ON_ONCE(gup_flags & FOLL_GET)) return -EINVAL; + /* FOLL_PIN requires pages != NULL */ + if (WARN_ON_ONCE(!pages)) + return -EINVAL; + gup_flags |= FOLL_PIN; return get_user_pages_unlocked(start, nr_pages, pages, gup_flags); } -- 2.32.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: 2022-04-21 16:41 Yury Norov @ 2022-04-21 23:04 ` John Hubbard 2022-04-21 23:09 ` Re: John Hubbard 2022-04-21 23:17 ` Re: Yury Norov 0 siblings, 2 replies; 5+ messages in thread From: John Hubbard @ 2022-04-21 23:04 UTC (permalink / raw) To: Yury Norov, Andrew Morton, Minchan Kim, linux-mm, linux-kernel On 4/21/22 09:41, Yury Norov wrote: > Subject: [PATCH] mm/gup: fix comments to pin_user_pages_*() > Hi Yuri, Thanks for picking this up. I have been distracted and didn't trust myself to focus on this properly, so it's good to have help! IT/admin point: somehow the first line of the commit description didn't make it into an actual email subject. The subject line was blank when it arrived in my inbox, and the subject is in the body here instead. Not sure how that happened. Maybe check your git-sendemail setup? > pin_user_pages API forces FOLL_PIN in gup_flags, which means that the > API requires struct page **pages to be provided (not NULL). However, > the comment to pin_user_pages() says: > > * @pages: array that receives pointers to the pages pinned. > * Should be at least nr_pages long. Or NULL, if caller > * only intends to ensure the pages are faulted in. > > This patch fixes comments along the pin_user_pages code, and also adds > WARN_ON(!pages), so that API users will have better understanding > on how to use it. No need to quote the code in the commit log. Instead, just summarize. For example: pin_user_pages API forces FOLL_PIN in gup_flags, which means that the API requires struct page **pages to be provided (not NULL). However, the comment to pin_user_pages() clearly allows for passing in a NULL @pages argument. Remove the incorrect comments, and add WARN_ON_ONCE(!pages) calls to enforce the API. > > It has been independently spotted by Minchan Kim and confirmed with > John Hubbard: > > https://lore.kernel.org/all/YgWA0ghrrzHONehH@google.com/ Let's add a Cc: line for Michan as well: Cc: Minchan Kim <minchan@kernel.org> > > Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com> > --- > mm/gup.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index f598a037eb04..559626457585 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2871,6 +2871,10 @@ int pin_user_pages_fast(unsigned long start, int nr_pages, > if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > return -EINVAL; > > + /* FOLL_PIN requires pages != NULL */ Please delete each and every one of these one-line comments, because they merely echo what the code says. > + if (WARN_ON_ONCE(!pages)) > + return -EINVAL; > + > gup_flags |= FOLL_PIN; > return internal_get_user_pages_fast(start, nr_pages, gup_flags, pages); > } > @@ -2893,6 +2897,10 @@ int pin_user_pages_fast_only(unsigned long start, int nr_pages, > */ > if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > return 0; > + > + /* FOLL_PIN requires pages != NULL */ > + if (WARN_ON_ONCE(!pages)) > + return 0; > /* > * FOLL_FAST_ONLY is required in order to match the API description of > * this routine: no fall back to regular ("slow") GUP. > @@ -2920,8 +2928,7 @@ EXPORT_SYMBOL_GPL(pin_user_pages_fast_only); > * @nr_pages: number of pages from start to pin > * @gup_flags: flags modifying lookup behaviour > * @pages: array that receives pointers to the pages pinned. > - * Should be at least nr_pages long. Or NULL, if caller > - * only intends to ensure the pages are faulted in. > + * Should be at least nr_pages long. > * @vmas: array of pointers to vmas corresponding to each page. > * Or NULL if the caller does not require them. > * @locked: pointer to lock flag indicating whether lock is held and > @@ -2944,6 +2951,10 @@ long pin_user_pages_remote(struct mm_struct *mm, > if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > return -EINVAL; > > + /* FOLL_PIN requires pages != NULL */ > + if (WARN_ON_ONCE(!pages)) > + return -EINVAL; > + > gup_flags |= FOLL_PIN; > return __get_user_pages_remote(mm, start, nr_pages, gup_flags, > pages, vmas, locked); > @@ -2957,8 +2968,7 @@ EXPORT_SYMBOL(pin_user_pages_remote); > * @nr_pages: number of pages from start to pin > * @gup_flags: flags modifying lookup behaviour > * @pages: array that receives pointers to the pages pinned. > - * Should be at least nr_pages long. Or NULL, if caller > - * only intends to ensure the pages are faulted in. > + * Should be at least nr_pages long. > * @vmas: array of pointers to vmas corresponding to each page. > * Or NULL if the caller does not require them. > * > @@ -2976,6 +2986,10 @@ long pin_user_pages(unsigned long start, unsigned long nr_pages, > if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > return -EINVAL; > > + /* FOLL_PIN requires pages != NULL */ > + if (WARN_ON_ONCE(!pages)) > + return -EINVAL; > + > gup_flags |= FOLL_PIN; > return __gup_longterm_locked(current->mm, start, nr_pages, > pages, vmas, gup_flags); > @@ -2994,6 +3008,10 @@ long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > return -EINVAL; > > + /* FOLL_PIN requires pages != NULL */ > + if (WARN_ON_ONCE(!pages)) > + return -EINVAL; > + > gup_flags |= FOLL_PIN; > return get_user_pages_unlocked(start, nr_pages, pages, gup_flags); > } I hope we don't break any callers with the newly enforced !pages, but it's the right thing to do, in order to avoid misunderstandings. thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: 2022-04-21 23:04 ` John Hubbard @ 2022-04-21 23:09 ` John Hubbard 2022-04-21 23:17 ` Re: Yury Norov 1 sibling, 0 replies; 5+ messages in thread From: John Hubbard @ 2022-04-21 23:09 UTC (permalink / raw) To: Yury Norov, Andrew Morton, Minchan Kim, linux-mm, linux-kernel On 4/21/22 16:04, John Hubbard wrote: > On 4/21/22 09:41, Yury Norov wrote: >> Subject: [PATCH] mm/gup: fix comments to pin_user_pages_*() >> > > Hi Yuri, ...and I see that I have typo'd both Yury's and Minchan's name (further down), in the same email! Really apologize for screwing that up. It's Yury-with-a-"y", I know. :) thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: 2022-04-21 23:04 ` John Hubbard 2022-04-21 23:09 ` Re: John Hubbard @ 2022-04-21 23:17 ` Yury Norov 2022-04-21 23:21 ` Re: John Hubbard 1 sibling, 1 reply; 5+ messages in thread From: Yury Norov @ 2022-04-21 23:17 UTC (permalink / raw) To: John Hubbard; +Cc: Andrew Morton, Minchan Kim, linux-mm, linux-kernel On Thu, Apr 21, 2022 at 04:04:44PM -0700, John Hubbard wrote: > On 4/21/22 09:41, Yury Norov wrote: > > Subject: [PATCH] mm/gup: fix comments to pin_user_pages_*() > > > > Hi Yuri, > > Thanks for picking this up. I have been distracted and didn't trust > myself to focus on this properly, so it's good to have help! > > IT/admin point: somehow the first line of the commit description didn't > make it into an actual email subject. The subject line was blank when it > arrived in my inbox, and the subject is in the body here instead. Not > sure how that happened. > > Maybe check your git-sendemail setup? git-sendmail is OK. I just accidentally added empty line above Subject, which broke format. My bad, sorry for this. > > pin_user_pages API forces FOLL_PIN in gup_flags, which means that the > > API requires struct page **pages to be provided (not NULL). However, > > the comment to pin_user_pages() says: > > > > * @pages: array that receives pointers to the pages pinned. > > * Should be at least nr_pages long. Or NULL, if caller > > * only intends to ensure the pages are faulted in. > > > > This patch fixes comments along the pin_user_pages code, and also adds > > WARN_ON(!pages), so that API users will have better understanding > > on how to use it. > > No need to quote the code in the commit log. Instead, just summarize. > For example: > > pin_user_pages API forces FOLL_PIN in gup_flags, which means that the > API requires struct page **pages to be provided (not NULL). However, the > comment to pin_user_pages() clearly allows for passing in a NULL @pages > argument. > > Remove the incorrect comments, and add WARN_ON_ONCE(!pages) calls to > enforce the API. > > > > > It has been independently spotted by Minchan Kim and confirmed with > > John Hubbard: > > > > https://lore.kernel.org/all/YgWA0ghrrzHONehH@google.com/ > > Let's add a Cc: line for Michan as well: > > Cc: Minchan Kim <minchan@kernel.org> He's in CC already, I think... > > Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com> > > --- > > mm/gup.c | 26 ++++++++++++++++++++++---- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/mm/gup.c b/mm/gup.c > > index f598a037eb04..559626457585 100644 > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -2871,6 +2871,10 @@ int pin_user_pages_fast(unsigned long start, int nr_pages, > > if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > > return -EINVAL; > > + /* FOLL_PIN requires pages != NULL */ > > Please delete each and every one of these one-line comments, because > they merely echo what the code says. Sure. > > + if (WARN_ON_ONCE(!pages)) > > + return -EINVAL; > > + > > gup_flags |= FOLL_PIN; > > return internal_get_user_pages_fast(start, nr_pages, gup_flags, pages); > > } > > @@ -2893,6 +2897,10 @@ int pin_user_pages_fast_only(unsigned long start, int nr_pages, > > */ > > if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > > return 0; > > + > > + /* FOLL_PIN requires pages != NULL */ > > + if (WARN_ON_ONCE(!pages)) > > + return 0; > > /* > > * FOLL_FAST_ONLY is required in order to match the API description of > > * this routine: no fall back to regular ("slow") GUP. > > @@ -2920,8 +2928,7 @@ EXPORT_SYMBOL_GPL(pin_user_pages_fast_only); > > * @nr_pages: number of pages from start to pin > > * @gup_flags: flags modifying lookup behaviour > > * @pages: array that receives pointers to the pages pinned. > > - * Should be at least nr_pages long. Or NULL, if caller > > - * only intends to ensure the pages are faulted in. > > + * Should be at least nr_pages long. > > * @vmas: array of pointers to vmas corresponding to each page. > > * Or NULL if the caller does not require them. > > * @locked: pointer to lock flag indicating whether lock is held and > > @@ -2944,6 +2951,10 @@ long pin_user_pages_remote(struct mm_struct *mm, > > if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > > return -EINVAL; > > + /* FOLL_PIN requires pages != NULL */ > > + if (WARN_ON_ONCE(!pages)) > > + return -EINVAL; > > + > > gup_flags |= FOLL_PIN; > > return __get_user_pages_remote(mm, start, nr_pages, gup_flags, > > pages, vmas, locked); > > @@ -2957,8 +2968,7 @@ EXPORT_SYMBOL(pin_user_pages_remote); > > * @nr_pages: number of pages from start to pin > > * @gup_flags: flags modifying lookup behaviour > > * @pages: array that receives pointers to the pages pinned. > > - * Should be at least nr_pages long. Or NULL, if caller > > - * only intends to ensure the pages are faulted in. > > + * Should be at least nr_pages long. > > * @vmas: array of pointers to vmas corresponding to each page. > > * Or NULL if the caller does not require them. > > * > > @@ -2976,6 +2986,10 @@ long pin_user_pages(unsigned long start, unsigned long nr_pages, > > if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > > return -EINVAL; > > + /* FOLL_PIN requires pages != NULL */ > > + if (WARN_ON_ONCE(!pages)) > > + return -EINVAL; > > + > > gup_flags |= FOLL_PIN; > > return __gup_longterm_locked(current->mm, start, nr_pages, > > pages, vmas, gup_flags); > > @@ -2994,6 +3008,10 @@ long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > > if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > > return -EINVAL; > > + /* FOLL_PIN requires pages != NULL */ > > + if (WARN_ON_ONCE(!pages)) > > + return -EINVAL; > > + > > gup_flags |= FOLL_PIN; > > return get_user_pages_unlocked(start, nr_pages, pages, gup_flags); > > } > > I hope we don't break any callers with the newly enforced !pages, but it's > the right thing to do, in order to avoid misunderstandings. > > thanks, > -- > John Hubbard > NVIDIA Let me test v2 and resend shortly. Thanks, Yury ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: 2022-04-21 23:17 ` Re: Yury Norov @ 2022-04-21 23:21 ` John Hubbard 0 siblings, 0 replies; 5+ messages in thread From: John Hubbard @ 2022-04-21 23:21 UTC (permalink / raw) To: Yury Norov; +Cc: Andrew Morton, Minchan Kim, linux-mm, linux-kernel On 4/21/22 16:17, Yury Norov wrote: >> Let's add a Cc: line for Michan as well: >> >> Cc: Minchan Kim <minchan@kernel.org> > > He's in CC already, I think... > Here, I am talking about attribution in the commit log, as opposed to the email Cc. In other words, I'm suggesting that you literally add this line to the commit description: Cc: Minchan Kim <minchan@kernel.org> thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-04-21 23:21 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-21 16:41 Yury Norov 2022-04-21 23:04 ` John Hubbard 2022-04-21 23:09 ` Re: John Hubbard 2022-04-21 23:17 ` Re: Yury Norov 2022-04-21 23:21 ` Re: John Hubbard
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.