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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 5 May 2020 13:52:59 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Souptick Joarder <jrdr.linux@...il.com>
CC:     Tony Luck <tony.luck@...el.com>, <fenghua.yu@...el.com>,
        Rob Springer <rspringer@...gle.com>,
        Todd Poynor <toddpoynor@...gle.com>, <benchan@...omium.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Jens Wiklander <jens.wiklander@...aro.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        <santosh.shilimkar@...cle.com>,
        "David S. Miller" <davem@...emloft.net>, <kuba@...nel.org>,
        Jan Kara <jack@...e.cz>, Ira Weiny <ira.weiny@...el.com>,
        Jérôme Glisse <jglisse@...hat.com>,
        <inux-ia64@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        "open list:ANDROID DRIVERS" <devel@...verdev.osuosl.org>,
        <tee-dev@...ts.linaro.org>, Linux-MM <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 13:36, Souptick Joarder wrote:
> On Wed, May 6, 2020 at 1:08 AM John Hubbard <jhubbard@...dia.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

Powered by blists - more mailing lists