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

[John Hubbard <jhubbard@nvidia.com>]

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