[RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Souptick Joarder]

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.

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;
 	}
-- 
1.9.1

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[John Hubbard]

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;
>   	}
> 

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Souptick Joarder]

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 ?

>
> 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 ?

>
>
> 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;
> >       }
> >
>

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[John Hubbard]

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

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Jan Kara]

On Wed 06-05-20 02:06:56, 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 ?

Well, first I'd ask a different question: Why do you want to change the
current behavior? It's not like the current behavior is confusing.  Callers
that pass >0 pages can happily rely on the simple behavior of < 0 return on
error or > 0 return if we mapped some pages. Callers that can possibly ask
to map 0 pages can get 0 pages back - kind of expected - and I don't see
any benefit in trying to rewrite these callers to handle -EINVAL instead...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Souptick Joarder]

On Wed, May 6, 2020 at 3:36 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 06-05-20 02:06:56, 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 ?
>
> Well, first I'd ask a different question: Why do you want to change the
> current behavior? It's not like the current behavior is confusing.  Callers
> that pass >0 pages can happily rely on the simple behavior of < 0 return on
> error or > 0 return if we mapped some pages. Callers that can possibly ask
> to map 0 pages can get 0 pages back - kind of expected - and I don't see
> any benefit in trying to rewrite these callers to handle -EINVAL instead...

Callers with a request to map 0 pages doesn't have a valid use case. But if any
caller end up doing it mistakenly, -errno should be returned to caller
rather than 0
which will indicate more precisely that map 0 pages is not a valid
request from caller.
With these, 3rd return value 0, is no more needed.

That was the thought behind this proposal.

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Jan Kara]

On Wed 06-05-20 17:51:39, Souptick Joarder wrote:
> On Wed, May 6, 2020 at 3:36 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 06-05-20 02:06:56, 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 ?
> >
> > Well, first I'd ask a different question: Why do you want to change the
> > current behavior? It's not like the current behavior is confusing.  Callers
> > that pass >0 pages can happily rely on the simple behavior of < 0 return on
> > error or > 0 return if we mapped some pages. Callers that can possibly ask
> > to map 0 pages can get 0 pages back - kind of expected - and I don't see
> > any benefit in trying to rewrite these callers to handle -EINVAL instead...
> 
> Callers with a request to map 0 pages doesn't have a valid use case. But if any
> caller end up doing it mistakenly, -errno should be returned to caller
> rather than 0
> which will indicate more precisely that map 0 pages is not a valid
> request from caller.

Well, I believe this depends on the point of view. Similarly as reading 0
bytes is successful, we could consider mapping 0 pages successful as well.
And there can be valid cases where number of pages to map is computed from
some input and when 0 pages should be mapped, it is not a problem and your
change would force such callers to special case this with explicitely
checking for 0 pages to map and not calling GUP in that case at all.

I'm not saying what you propose is necessarily bad, I just say I don't find
it any better than the current behavior and so IMO it's not worth the
churn. Now if you can come up with some examples of current in-kernel users
who indeed do get the handling of the return value wrong, I could be
convinced otherwise.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Souptick Joarder]

On Wed, May 6, 2020 at 6:29 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 06-05-20 17:51:39, Souptick Joarder wrote:
> > On Wed, May 6, 2020 at 3:36 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Wed 06-05-20 02:06:56, 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 ?
> > >
> > > Well, first I'd ask a different question: Why do you want to change the
> > > current behavior? It's not like the current behavior is confusing.  Callers
> > > that pass >0 pages can happily rely on the simple behavior of < 0 return on
> > > error or > 0 return if we mapped some pages. Callers that can possibly ask
> > > to map 0 pages can get 0 pages back - kind of expected - and I don't see
> > > any benefit in trying to rewrite these callers to handle -EINVAL instead...
> >
> > Callers with a request to map 0 pages doesn't have a valid use case. But if any
> > caller end up doing it mistakenly, -errno should be returned to caller
> > rather than 0
> > which will indicate more precisely that map 0 pages is not a valid
> > request from caller.
>
> Well, I believe this depends on the point of view. Similarly as reading 0
> bytes is successful, we could consider mapping 0 pages successful as well.
> And there can be valid cases where number of pages to map is computed from
> some input and when 0 pages should be mapped, it is not a problem and your
> change would force such callers to special case this with explicitely
> checking for 0 pages to map and not calling GUP in that case at all.
>
> I'm not saying what you propose is necessarily bad, I just say I don't find
> it any better than the current behavior and so IMO it's not worth the
> churn. Now if you can come up with some examples of current in-kernel users
> who indeed do get the handling of the return value wrong, I could be
> convinced otherwise.

There are 5 callers of {get|pin}_user_pages_fast().

arch/ia64/kernel/err_inject.c#L145
staging/gasket/gasket_page_table.c#L489

Checking return value 0 doesn't make sense for above 2.

drivers/platform/goldfish/goldfish_pipe.c#L277
net/rds/rdma.c#L165
drivers/tee/tee_shm.c#L262

These 3 callers have calculated the no of pages value before passing it to
{get|pin}_user_pages_fast(). But if they end up passing nr_pages <= 0, a return
value of either 0 or -EINVAL doesn't going to harm any existing
behavior of callers.

IMO, it is safe to return -errno for nr_pages <= 0, for
{get|pin}_user_pages_fast().

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Jan Kara]

On Wed 06-05-20 21:38:40, Souptick Joarder wrote:
> On Wed, May 6, 2020 at 6:29 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 06-05-20 17:51:39, Souptick Joarder wrote:
> > > On Wed, May 6, 2020 at 3:36 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Wed 06-05-20 02:06:56, 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 ?
> > > >
> > > > Well, first I'd ask a different question: Why do you want to change the
> > > > current behavior? It's not like the current behavior is confusing.  Callers
> > > > that pass >0 pages can happily rely on the simple behavior of < 0 return on
> > > > error or > 0 return if we mapped some pages. Callers that can possibly ask
> > > > to map 0 pages can get 0 pages back - kind of expected - and I don't see
> > > > any benefit in trying to rewrite these callers to handle -EINVAL instead...
> > >
> > > Callers with a request to map 0 pages doesn't have a valid use case. But if any
> > > caller end up doing it mistakenly, -errno should be returned to caller
> > > rather than 0
> > > which will indicate more precisely that map 0 pages is not a valid
> > > request from caller.
> >
> > Well, I believe this depends on the point of view. Similarly as reading 0
> > bytes is successful, we could consider mapping 0 pages successful as well.
> > And there can be valid cases where number of pages to map is computed from
> > some input and when 0 pages should be mapped, it is not a problem and your
> > change would force such callers to special case this with explicitely
> > checking for 0 pages to map and not calling GUP in that case at all.
> >
> > I'm not saying what you propose is necessarily bad, I just say I don't find
> > it any better than the current behavior and so IMO it's not worth the
> > churn. Now if you can come up with some examples of current in-kernel users
> > who indeed do get the handling of the return value wrong, I could be
> > convinced otherwise.
> 
> There are 5 callers of {get|pin}_user_pages_fast().

Oh, there are *much* more callers that 5. It's more like 70. Just grep the
source... And then you have all other {get|pin}_user_pages() variants that
need to be kept consistent. So overall we have over 200 calls to some
variant of GUP.

> arch/ia64/kernel/err_inject.c#L145
> staging/gasket/gasket_page_table.c#L489
> 
> Checking return value 0 doesn't make sense for above 2.
> 
> drivers/platform/goldfish/goldfish_pipe.c#L277
> net/rds/rdma.c#L165
> drivers/tee/tee_shm.c#L262
> 
> These 3 callers have calculated the no of pages value before passing it to
> {get|pin}_user_pages_fast(). But if they end up passing nr_pages <= 0, a return
> value of either 0 or -EINVAL doesn't going to harm any existing
> behavior of callers.
> 
> IMO, it is safe to return -errno for nr_pages <= 0, for
> {get|pin}_user_pages_fast().

OK, so no real problem with any of these callers. I still don't see a
justification for the churn you suggest... Auditting all those code sites
is going to be pretty tedious.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Souptick Joarder]

On Thu, May 7, 2020 at 3:43 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 06-05-20 21:38:40, Souptick Joarder wrote:
> > On Wed, May 6, 2020 at 6:29 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Wed 06-05-20 17:51:39, Souptick Joarder wrote:
> > > > On Wed, May 6, 2020 at 3:36 PM Jan Kara <jack@suse.cz> wrote:
> > > > >
> > > > > On Wed 06-05-20 02:06:56, 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 ?
> > > > >
> > > > > Well, first I'd ask a different question: Why do you want to change the
> > > > > current behavior? It's not like the current behavior is confusing.  Callers
> > > > > that pass >0 pages can happily rely on the simple behavior of < 0 return on
> > > > > error or > 0 return if we mapped some pages. Callers that can possibly ask
> > > > > to map 0 pages can get 0 pages back - kind of expected - and I don't see
> > > > > any benefit in trying to rewrite these callers to handle -EINVAL instead...
> > > >
> > > > Callers with a request to map 0 pages doesn't have a valid use case. But if any
> > > > caller end up doing it mistakenly, -errno should be returned to caller
> > > > rather than 0
> > > > which will indicate more precisely that map 0 pages is not a valid
> > > > request from caller.
> > >
> > > Well, I believe this depends on the point of view. Similarly as reading 0
> > > bytes is successful, we could consider mapping 0 pages successful as well.
> > > And there can be valid cases where number of pages to map is computed from
> > > some input and when 0 pages should be mapped, it is not a problem and your
> > > change would force such callers to special case this with explicitely
> > > checking for 0 pages to map and not calling GUP in that case at all.
> > >
> > > I'm not saying what you propose is necessarily bad, I just say I don't find
> > > it any better than the current behavior and so IMO it's not worth the
> > > churn. Now if you can come up with some examples of current in-kernel users
> > > who indeed do get the handling of the return value wrong, I could be
> > > convinced otherwise.
> >
> > There are 5 callers of {get|pin}_user_pages_fast().
>
> Oh, there are *much* more callers that 5. It's more like 70. Just grep the
> source... And then you have all other {get|pin}_user_pages() variants that
> need to be kept consistent. So overall we have over 200 calls to some
> variant of GUP.

Sorry, I mean, there are 5 callers of {get|pin}_user_pages_fast() who
have interest in
return value 0, out of total 42.

>
> > arch/ia64/kernel/err_inject.c#L145
> > staging/gasket/gasket_page_table.c#L489
> >
> > Checking return value 0 doesn't make sense for above 2.
> >
> > drivers/platform/goldfish/goldfish_pipe.c#L277
> > net/rds/rdma.c#L165
> > drivers/tee/tee_shm.c#L262
> >
> > These 3 callers have calculated the no of pages value before passing it to
> > {get|pin}_user_pages_fast(). But if they end up passing nr_pages <= 0, a return
> > value of either 0 or -EINVAL doesn't going to harm any existing
> > behavior of callers.
> >
> > IMO, it is safe to return -errno for nr_pages <= 0, for
> > {get|pin}_user_pages_fast().
>
> OK, so no real problem with any of these callers. I still don't see a
> justification for the churn you suggest... Auditting all those code sites
> is going to be pretty tedious.

I try to audit all 42 callers of {get|pin}_user_pages_fast() and
figure out these 5 callers
which need to be updated and I think, other callers of
{get|pin}_user_pages_fast() will not be
effected.

But I didn't go through other variants of gup/pup except
{get|pin}_user_pages_fast().

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[John Hubbard]

On 2020-05-07 03:32, Souptick Joarder wrote:
...
>> OK, so no real problem with any of these callers. I still don't see a
>> justification for the churn you suggest... Auditting all those code sites
>> is going to be pretty tedious.
> 
> I try to audit all 42 callers of {get|pin}_user_pages_fast() and
> figure out these 5 callers
> which need to be updated and I think, other callers of
> {get|pin}_user_pages_fast() will not be
> effected.
> 
> But I didn't go through other variants of gup/pup except
> {get|pin}_user_pages_fast().


I feel the need to apologize for suggesting that a change to -EINVAL
would help. :)

If you change what the return value means, but only apply it the
gup/pup _fast() variants of this API set, that would make
the API significantly *worse*.

Also, no one has been able to come up with a scenario in which the call
sites actually have a problem handling return values of zero. In fact,
on the contrary: there are call site where returning 0 after being
requested to pin zero pages, helps simplify the code. For example, if
they're just doing math such as "if(nr_expected != nr_pages_pinned) ...".


This looks like a complete dead end, sorry.

thanks,
-- 
John Hubbard
NVIDIA