From: John Hubbard <jhubbard@nvidia.com> To: Souptick Joarder <jrdr.linux@gmail.com>, <tony.luck@intel.com>, <fenghua.yu@intel.com>, <rspringer@google.com>, <toddpoynor@google.com>, <benchan@chromium.org>, <gregkh@linuxfoundation.org>, <jens.wiklander@linaro.org>, <akpm@linux-foundation.org>, <santosh.shilimkar@oracle.com>, <davem@davemloft.net>, <kuba@kernel.org>, <jack@suse.cz>, <ira.weiny@intel.com>, <jglisse@redhat.com> Cc: <inux-ia64@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <devel@driverdev.osuosl.org>, <tee-dev@lists.linaro.org>, <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 12:38:17 -0700 [thread overview] Message-ID: <0bfe4a8a-0d91-ef9b-066f-2ea7c68571b3@nvidia.com> (raw) In-Reply-To: <1588706059-4208-1-git-send-email-jrdr.linux@gmail.com> 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. 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. 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. Once that change (and anything I missed) is in place, then you could go ahead and stop handling ret==0 cases at the call sites. thanks, -- John Hubbard NVIDIA > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > --- > arch/ia64/kernel/err_inject.c | 2 +- > drivers/platform/goldfish/goldfish_pipe.c | 2 +- > drivers/staging/gasket/gasket_page_table.c | 4 ++-- > drivers/tee/tee_shm.c | 2 +- > mm/gup.c | 6 +++--- > net/rds/rdma.c | 2 +- > 6 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/arch/ia64/kernel/err_inject.c b/arch/ia64/kernel/err_inject.c > index 8b5b8e6b..fd72218 100644 > --- a/arch/ia64/kernel/err_inject.c > +++ b/arch/ia64/kernel/err_inject.c > @@ -143,7 +143,7 @@ static DEVICE_ATTR(name, 0644, show_##name, store_##name) > int ret; > > ret = get_user_pages_fast(virt_addr, 1, FOLL_WRITE, NULL); > - if (ret<=0) { > + if (ret < 0) { > #ifdef ERR_INJ_DEBUG > printk("Virtual address %lx is not existing.\n",virt_addr); > #endif > diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c > index 1ab207e..831449d 100644 > --- a/drivers/platform/goldfish/goldfish_pipe.c > +++ b/drivers/platform/goldfish/goldfish_pipe.c > @@ -277,7 +277,7 @@ static int goldfish_pin_pages(unsigned long first_page, > ret = pin_user_pages_fast(first_page, requested_pages, > !is_write ? FOLL_WRITE : 0, > pages); > - if (ret <= 0) > + if (ret < 0) > return -EFAULT; > if (ret < requested_pages) > *iter_last_page_size = PAGE_SIZE; > diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c > index f6d7157..1d08e1d 100644 > --- a/drivers/staging/gasket/gasket_page_table.c > +++ b/drivers/staging/gasket/gasket_page_table.c > @@ -489,11 +489,11 @@ static int gasket_perform_mapping(struct gasket_page_table *pg_tbl, > ret = get_user_pages_fast(page_addr - offset, 1, > FOLL_WRITE, &page); > > - if (ret <= 0) { > + if (ret < 0) { > dev_err(pg_tbl->device, > "get user pages failed for addr=0x%lx, offset=0x%lx [ret=%d]\n", > page_addr, offset, ret); > - return ret ? ret : -ENOMEM; > + return ret; > } > ++pg_tbl->num_active_pages; > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > index bd679b7..2706a1f 100644 > --- a/drivers/tee/tee_shm.c > +++ b/drivers/tee/tee_shm.c > @@ -230,7 +230,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, > if (rc > 0) > shm->num_pages = rc; > if (rc != num_pages) { > - if (rc >= 0) > + if (rc > 0) > rc = -ENOMEM; > ret = ERR_PTR(rc); > goto err; > diff --git a/mm/gup.c b/mm/gup.c > index 50681f0..8d293ed 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2760,7 +2760,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; > > @@ -2805,8 +2805,8 @@ static int internal_get_user_pages_fast(unsigned long start, int nr_pages, > * calling get_user_pages(). > * > * Returns number of pages pinned. This may be fewer than the number requested. > - * If nr_pages is 0 or negative, returns 0. If no pages were pinned, returns > - * -errno. > + * If nr_pages is 0 or negative, returns -errno. If no pages were pinned, > + * returns -errno. > */ > int get_user_pages_fast(unsigned long start, int nr_pages, > unsigned int gup_flags, struct page **pages) > diff --git a/net/rds/rdma.c b/net/rds/rdma.c > index a7ae118..44b96e6 100644 > --- a/net/rds/rdma.c > +++ b/net/rds/rdma.c > @@ -161,7 +161,7 @@ static int rds_pin_pages(unsigned long user_addr, unsigned int nr_pages, > gup_flags |= FOLL_WRITE; > > ret = pin_user_pages_fast(user_addr, nr_pages, gup_flags, pages); > - if (ret >= 0 && ret < nr_pages) { > + if (ret > 0 && ret < nr_pages) { > unpin_user_pages(pages, ret); > ret = -EFAULT; > } >
WARNING: multiple messages have this Message-ID (diff)
From: John Hubbard <jhubbard@nvidia.com> To: Souptick Joarder <jrdr.linux@gmail.com>, <tony.luck@intel.com>, <fenghua.yu@intel.com>, <rspringer@google.com>, <toddpoynor@google.com>, <benchan@chromium.org>, <gregkh@linuxfoundation.org>, <jens.wiklander@linaro.org>, <akpm@linux-foundation.org>, <santosh.shilimkar@oracle.com>, <davem@davemloft.net>, <kuba@kernel.org>, <jack@suse.cz>, <ira.weiny@intel.com>, <jglisse@redhat.com> Cc: devel@driverdev.osuosl.org, rds-devel@oss.oracle.com, linux-rdma@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, tee-dev@lists.linaro.org, linux-mm@kvack.org, inux-ia64@vger.kernel.org Subject: Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast() Date: Tue, 5 May 2020 12:38:17 -0700 [thread overview] Message-ID: <0bfe4a8a-0d91-ef9b-066f-2ea7c68571b3@nvidia.com> (raw) In-Reply-To: <1588706059-4208-1-git-send-email-jrdr.linux@gmail.com> 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. 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. 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. Once that change (and anything I missed) is in place, then you could go ahead and stop handling ret==0 cases at the call sites. thanks, -- John Hubbard NVIDIA > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > --- > arch/ia64/kernel/err_inject.c | 2 +- > drivers/platform/goldfish/goldfish_pipe.c | 2 +- > drivers/staging/gasket/gasket_page_table.c | 4 ++-- > drivers/tee/tee_shm.c | 2 +- > mm/gup.c | 6 +++--- > net/rds/rdma.c | 2 +- > 6 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/arch/ia64/kernel/err_inject.c b/arch/ia64/kernel/err_inject.c > index 8b5b8e6b..fd72218 100644 > --- a/arch/ia64/kernel/err_inject.c > +++ b/arch/ia64/kernel/err_inject.c > @@ -143,7 +143,7 @@ static DEVICE_ATTR(name, 0644, show_##name, store_##name) > int ret; > > ret = get_user_pages_fast(virt_addr, 1, FOLL_WRITE, NULL); > - if (ret<=0) { > + if (ret < 0) { > #ifdef ERR_INJ_DEBUG > printk("Virtual address %lx is not existing.\n",virt_addr); > #endif > diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c > index 1ab207e..831449d 100644 > --- a/drivers/platform/goldfish/goldfish_pipe.c > +++ b/drivers/platform/goldfish/goldfish_pipe.c > @@ -277,7 +277,7 @@ static int goldfish_pin_pages(unsigned long first_page, > ret = pin_user_pages_fast(first_page, requested_pages, > !is_write ? FOLL_WRITE : 0, > pages); > - if (ret <= 0) > + if (ret < 0) > return -EFAULT; > if (ret < requested_pages) > *iter_last_page_size = PAGE_SIZE; > diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c > index f6d7157..1d08e1d 100644 > --- a/drivers/staging/gasket/gasket_page_table.c > +++ b/drivers/staging/gasket/gasket_page_table.c > @@ -489,11 +489,11 @@ static int gasket_perform_mapping(struct gasket_page_table *pg_tbl, > ret = get_user_pages_fast(page_addr - offset, 1, > FOLL_WRITE, &page); > > - if (ret <= 0) { > + if (ret < 0) { > dev_err(pg_tbl->device, > "get user pages failed for addr=0x%lx, offset=0x%lx [ret=%d]\n", > page_addr, offset, ret); > - return ret ? ret : -ENOMEM; > + return ret; > } > ++pg_tbl->num_active_pages; > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > index bd679b7..2706a1f 100644 > --- a/drivers/tee/tee_shm.c > +++ b/drivers/tee/tee_shm.c > @@ -230,7 +230,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, > if (rc > 0) > shm->num_pages = rc; > if (rc != num_pages) { > - if (rc >= 0) > + if (rc > 0) > rc = -ENOMEM; > ret = ERR_PTR(rc); > goto err; > diff --git a/mm/gup.c b/mm/gup.c > index 50681f0..8d293ed 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2760,7 +2760,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; > > @@ -2805,8 +2805,8 @@ static int internal_get_user_pages_fast(unsigned long start, int nr_pages, > * calling get_user_pages(). > * > * Returns number of pages pinned. This may be fewer than the number requested. > - * If nr_pages is 0 or negative, returns 0. If no pages were pinned, returns > - * -errno. > + * If nr_pages is 0 or negative, returns -errno. If no pages were pinned, > + * returns -errno. > */ > int get_user_pages_fast(unsigned long start, int nr_pages, > unsigned int gup_flags, struct page **pages) > diff --git a/net/rds/rdma.c b/net/rds/rdma.c > index a7ae118..44b96e6 100644 > --- a/net/rds/rdma.c > +++ b/net/rds/rdma.c > @@ -161,7 +161,7 @@ static int rds_pin_pages(unsigned long user_addr, unsigned int nr_pages, > gup_flags |= FOLL_WRITE; > > ret = pin_user_pages_fast(user_addr, nr_pages, gup_flags, pages); > - if (ret >= 0 && ret < nr_pages) { > + if (ret > 0 && ret < nr_pages) { > unpin_user_pages(pages, ret); > ret = -EFAULT; > } > _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2020-05-05 19:38 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 [this message] 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 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=0bfe4a8a-0d91-ef9b-066f-2ea7c68571b3@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.