| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 5 May 2020 12:38:17 -0700
From: John Hubbard <jhubbard@...dia.com>
To: Souptick Joarder <jrdr.linux@...il.com>, <tony.luck@...el.com>,
<fenghua.yu@...el.com>, <rspringer@...gle.com>,
<toddpoynor@...gle.com>, <benchan@...omium.org>,
<gregkh@...uxfoundation.org>, <jens.wiklander@...aro.org>,
<akpm@...ux-foundation.org>, <santosh.shilimkar@...cle.com>,
<davem@...emloft.net>, <kuba@...nel.org>, <jack@...e.cz>,
<ira.weiny@...el.com>, <jglisse@...hat.com>
CC: <inux-ia64@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<devel@...verdev.osuosl.org>, <tee-dev@...ts.linaro.org>,
<linux-mm@...ck.org>, <netdev@...r.kernel.org>,
<linux-rdma@...r.kernel.org>, <rds-devel@....oracle.com>
Subject: Re: [RFC] mm/gup.c: Updated return value of
{get|pin}_user_pages_fast()
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@...il.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;
> }
>
Powered by blists - more mailing lists