| Message ID | 1588706059-4208-1-git-send-email-jrdr.linux@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast() | expand |
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,
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; > > } > > >
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,
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
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.
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
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().
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
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().
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,
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; }
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(-)