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,