linux-kernel - Re: [linux-next PATCH] rapidio: Fix error handling path

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 linux-cve-announce 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:   Thu, 17 Sep 2020 23:33:19 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Souptick Joarder <jrdr.linux@...il.com>
CC:     Ira Weiny <ira.weiny@...el.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        <mporter@...nel.crashing.org>, <alex.bou9@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        <gustavoars@...nel.org>, <madhuparnabhowmik10@...il.com>,
        <linux-kernel@...r.kernel.org>,
        "Matthew Wilcox" <willy@...radead.org>
Subject: Re: [linux-next PATCH] rapidio: Fix error handling path

On 9/17/20 7:21 PM, Souptick Joarder wrote:
> On Thu, Sep 17, 2020 at 11:17 PM John Hubbard <jhubbard@...dia.com> wrote:
...
>>>> I sort of feel like returning partial successes is not working.  We
>>>> could easily make a wrapper which either pins everything or it returns
>>>> an error code.
>>
>> Yes we could. And I have the same feeling about this API. It's generated a
>> remarkable amount of bug fixes, several of which ended up being partial or
>> wrong in themselves. And mostly this is due to the complicated tristate
>> return code: instead of 0 or -ERRNO, it also can return "N pages that is
>> less than what you requested", and there are no standard helpers in the kernel
>> to make that easier to deal with
> 
> There was some discussion on removing return value 0 from one of the
> gup variants [1].
> I think it might be partially relevant to the current discussion.
> 
> [1] https://patchwork.kernel.org/patch/11529795/
> 

Yes, although as I mentioned above, I'm thinking of a 0 or -ERRNO return value,
and not even return nr_pages at all.

But in any case, as a practical matter, I'm not sure if it's a good idea to
actually change all the callsites, or not. If we just fix the remaining buggy
callers, maybe that's better than the churn associated with another API change.

On the other-other hand, there does seem to be more churn coming anyway, with
talk of actually doing a [get|pin]_user_bvec(), for example. So maybe it's better
to head off the coming mess.

This is something that should be discussed on linux-mm.

>>
>>>
>>> I guess the question is are there drivers which will keep working (or limp
>>> along?) on partial pins?  A quick search of a driver I thought did this does
>>> not apparently any more...  So it sounds good to me from 30,000 feet!  :-D
>>
>> It sounds good to me too--and from just a *few hundred feet* (having touched most
>> of the call sites at some point)! haha :)
>>
>> I think the wrapper should be short-term, though, just until all the callers
>> are converted to the simpler API. Then change the core gup/pup calls to the simpler
>> API. There are more than enough gup/pup API entry points as it is, that's for sure.
>>
>>
>> thanks,
>> --
>> John Hubbard
>> NVIDIA

thanks,
-- 
John Hubbard
NVIDIA