From: John Hubbard <jhubbard@nvidia.com> To: Souptick Joarder <jrdr.linux@gmail.com> Cc: "Tony Luck" <tony.luck@intel.com>, fenghua.yu@intel.com, "Rob Springer" <rspringer@google.com>, "Todd Poynor" <toddpoynor@google.com>, benchan@chromium.org, "Greg KH" <gregkh@linuxfoundation.org>, "Jens Wiklander" <jens.wiklander@linaro.org>, "Andrew Morton" <akpm@linux-foundation.org>, santosh.shilimkar@oracle.com, "David S. Miller" <davem@davemloft.net>, kuba@kernel.org, "Jan Kara" <jack@suse.cz>, "Ira Weiny" <ira.weiny@intel.com>, "Jérôme Glisse" <jglisse@redhat.com>, inux-ia64@vger.kernel.org, linux-kernel@vger.kernel.org, "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>, tee-dev@lists.linaro.org, Linux-MM <linux-mm@kvack.org>, netdev@vger.kernel.org, linux-rdma@vger.kernel.org, rds-devel@oss.oracle.com Subject: Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast() Date: Tue, 5 May 2020 13:52:59 -0700 [thread overview] Message-ID: <3a974031-39dc-0359-a574-3ab6770de97f@nvidia.com> (raw) In-Reply-To: <CAFqt6zZMsQkOdjAb2k1EjwX=DtZ8gKfbRzwvreHOX-0vJLngNg@mail.gmail.com> On 2020-05-05 13:36, Souptick Joarder wrote: > On Wed, May 6, 2020 at 1:08 AM John Hubbard <jhubbard@nvidia.com> wrote: >> >> On 2020-05-05 12:14, Souptick Joarder wrote: >>> Currently {get|pin}_user_pages_fast() have 3 return value 0, -errno >>> and no of pinned pages. The only case where these two functions will >>> return 0, is for nr_pages <= 0, which doesn't find a valid use case. >>> But if at all any, then a -ERRNO will be returned instead of 0, which >>> means {get|pin}_user_pages_fast() will have 2 return values -errno & >>> no of pinned pages. >>> >>> Update all the callers which deals with return value 0 accordingly. >> >> Hmmm, seems a little shaky. In order to do this safely, I'd recommend >> first changing gup_fast/pup_fast so so that they return -EINVAL if >> the caller specified nr_pages==0, and of course auditing all callers, >> to ensure that this won't cause problems. > > While auditing it was figured out, there are 5 callers which cares for > return value > 0 of gup_fast/pup_fast. What problem it might cause if we change > gup_fast/pup_fast > to return -EINVAL and update all the callers in a single commit ? If you change the semantics of a core API, it's critical to do it in steps that are safe even against other code changes that may be merged in. There are other people potentially editing the callers. And those might very well be in different git trees, and on different mailing lists. Even within a tree, it's possible to either overlook a call site during an audit, or for someone else (who overlooked your change's review discussions) to commit a change that doesn't follow the same assumptions. So API assumptions often need to be backed up by things like -errno return values, or sometimes even WARN*() statements. For a recent example: gup() assumes that no one passes in a "bare" FOLL_PIN flag to it. Therfore, it returns -errno and also WARN's in that case--for precisely the same reasons: other people are editing the code base. It's not static. > >> >> The gup.c documentation would also need updating in a couple of comment >> blocks, above get_user_pages_remote(), and __get_user_pages(), because >> those talk about a zero return value. > > OK. > >> >> This might be practical without slowing down the existing code, because >> there is already a check in place, so just tweaking it like this (untested) >> won't change performance at all: >> >> diff --git a/mm/gup.c b/mm/gup.c >> index 11fda538c9d9..708eed79ae29 100644 >> --- a/mm/gup.c >> +++ b/mm/gup.c >> @@ -2787,7 +2787,7 @@ static int internal_get_user_pages_fast(unsigned long start, >> int nr_pages, >> end = start + len; >> >> if (end <= start) >> - return 0; >> + return -EINVAL; >> if (unlikely(!access_ok((void __user *)start, len))) >> return -EFAULT; >> >> ...although I might be missing some other things that need a similar change, >> so you should look carefully for yourself. > > Do you refer to other gup APIs similar to gup_fast/pup_fast ? Yes, like all the gup/pup variants. thanks, -- John Hubbard NVIDIA
WARNING: multiple messages have this Message-ID (diff)
From: John Hubbard <jhubbard@nvidia.com> To: Souptick Joarder <jrdr.linux@gmail.com> Cc: "Jan Kara" <jack@suse.cz>, Linux-MM <linux-mm@kvack.org>, inux-ia64@vger.kernel.org, "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>, rds-devel@oss.oracle.com, linux-rdma@vger.kernel.org, kuba@kernel.org, fenghua.yu@intel.com, santosh.shilimkar@oracle.com, "Jérôme Glisse" <jglisse@redhat.com>, "Rob Springer" <rspringer@google.com>, "Todd Poynor" <toddpoynor@google.com>, "Tony Luck" <tony.luck@intel.com>, "Greg KH" <gregkh@linuxfoundation.org>, linux-kernel@vger.kernel.org, "Jens Wiklander" <jens.wiklander@linaro.org>, tee-dev@lists.linaro.org, netdev@vger.kernel.org, "Andrew Morton" <akpm@linux-foundation.org>, "David S. Miller" <davem@davemloft.net> Subject: Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast() Date: Tue, 5 May 2020 13:52:59 -0700 [thread overview] Message-ID: <3a974031-39dc-0359-a574-3ab6770de97f@nvidia.com> (raw) In-Reply-To: <CAFqt6zZMsQkOdjAb2k1EjwX=DtZ8gKfbRzwvreHOX-0vJLngNg@mail.gmail.com> On 2020-05-05 13:36, Souptick Joarder wrote: > On Wed, May 6, 2020 at 1:08 AM John Hubbard <jhubbard@nvidia.com> wrote: >> >> On 2020-05-05 12:14, Souptick Joarder wrote: >>> Currently {get|pin}_user_pages_fast() have 3 return value 0, -errno >>> and no of pinned pages. The only case where these two functions will >>> return 0, is for nr_pages <= 0, which doesn't find a valid use case. >>> But if at all any, then a -ERRNO will be returned instead of 0, which >>> means {get|pin}_user_pages_fast() will have 2 return values -errno & >>> no of pinned pages. >>> >>> Update all the callers which deals with return value 0 accordingly. >> >> Hmmm, seems a little shaky. In order to do this safely, I'd recommend >> first changing gup_fast/pup_fast so so that they return -EINVAL if >> the caller specified nr_pages==0, and of course auditing all callers, >> to ensure that this won't cause problems. > > While auditing it was figured out, there are 5 callers which cares for > return value > 0 of gup_fast/pup_fast. What problem it might cause if we change > gup_fast/pup_fast > to return -EINVAL and update all the callers in a single commit ? If you change the semantics of a core API, it's critical to do it in steps that are safe even against other code changes that may be merged in. There are other people potentially editing the callers. And those might very well be in different git trees, and on different mailing lists. Even within a tree, it's possible to either overlook a call site during an audit, or for someone else (who overlooked your change's review discussions) to commit a change that doesn't follow the same assumptions. So API assumptions often need to be backed up by things like -errno return values, or sometimes even WARN*() statements. For a recent example: gup() assumes that no one passes in a "bare" FOLL_PIN flag to it. Therfore, it returns -errno and also WARN's in that case--for precisely the same reasons: other people are editing the code base. It's not static. > >> >> The gup.c documentation would also need updating in a couple of comment >> blocks, above get_user_pages_remote(), and __get_user_pages(), because >> those talk about a zero return value. > > OK. > >> >> This might be practical without slowing down the existing code, because >> there is already a check in place, so just tweaking it like this (untested) >> won't change performance at all: >> >> diff --git a/mm/gup.c b/mm/gup.c >> index 11fda538c9d9..708eed79ae29 100644 >> --- a/mm/gup.c >> +++ b/mm/gup.c >> @@ -2787,7 +2787,7 @@ static int internal_get_user_pages_fast(unsigned long start, >> int nr_pages, >> end = start + len; >> >> if (end <= start) >> - return 0; >> + return -EINVAL; >> if (unlikely(!access_ok((void __user *)start, len))) >> return -EFAULT; >> >> ...although I might be missing some other things that need a similar change, >> so you should look carefully for yourself. > > Do you refer to other gup APIs similar to gup_fast/pup_fast ? Yes, like all the gup/pup variants. thanks, -- John Hubbard NVIDIA _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2020-05-05 20:53 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-05 19:14 [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast() Souptick Joarder 2020-05-05 19:14 ` Souptick Joarder 2020-05-05 19:38 ` John Hubbard 2020-05-05 19:38 ` John Hubbard 2020-05-05 20:36 ` Souptick Joarder 2020-05-05 20:36 ` Souptick Joarder 2020-05-05 20:52 ` John Hubbard [this message] 2020-05-05 20:52 ` John Hubbard 2020-05-06 10:06 ` Jan Kara 2020-05-06 10:06 ` Jan Kara 2020-05-06 12:21 ` Souptick Joarder 2020-05-06 12:21 ` Souptick Joarder 2020-05-06 12:59 ` Jan Kara 2020-05-06 12:59 ` Jan Kara 2020-05-06 16:08 ` Souptick Joarder 2020-05-06 16:08 ` Souptick Joarder 2020-05-07 10:13 ` Jan Kara 2020-05-07 10:13 ` Jan Kara 2020-05-07 10:32 ` Souptick Joarder 2020-05-07 10:32 ` Souptick Joarder 2020-05-07 18:44 ` John Hubbard 2020-05-07 18:44 ` John Hubbard
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3a974031-39dc-0359-a574-3ab6770de97f@nvidia.com \ --to=jhubbard@nvidia.com \ --cc=akpm@linux-foundation.org \ --cc=benchan@chromium.org \ --cc=davem@davemloft.net \ --cc=devel@driverdev.osuosl.org \ --cc=fenghua.yu@intel.com \ --cc=gregkh@linuxfoundation.org \ --cc=inux-ia64@vger.kernel.org \ --cc=ira.weiny@intel.com \ --cc=jack@suse.cz \ --cc=jens.wiklander@linaro.org \ --cc=jglisse@redhat.com \ --cc=jrdr.linux@gmail.com \ --cc=kuba@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linux-rdma@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=rds-devel@oss.oracle.com \ --cc=rspringer@google.com \ --cc=santosh.shilimkar@oracle.com \ --cc=tee-dev@lists.linaro.org \ --cc=toddpoynor@google.com \ --cc=tony.luck@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.