linux-kernel - Re: [PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user

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:   Sat, 7 Nov 2020 18:17:22 -0800
From:   John Hubbard <jhubbard@...dia.com>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Souptick Joarder <jrdr.linux@...il.com>
CC:     <linux-security-module@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Jan Kara <jack@...e.cz>,
        Matthew Wilcox <willy@...radead.org>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>
Subject: Re: [PATCH 1/2] tomoyo: Convert get_user_pages*() to
 pin_user_pages*()

On 11/7/20 5:13 PM, Tetsuo Handa wrote:
> On 2020/11/08 4:17, John Hubbard wrote:
>> On 11/7/20 1:04 AM, John Hubbard wrote:
>>> On 11/7/20 12:24 AM, Souptick Joarder wrote:
>>>> In 2019, we introduced pin_user_pages*() and now we are converting
>>>> get_user_pages*() to the new API as appropriate. [1] & [2] could
>>>> be referred for more information. This is case 5 as per document [1].
>>>
>>> It turns out that Case 5 can be implemented via a better pattern, as long
>>> as we're just dealing with a page at a time, briefly:
>>>
>>> lock_page()
>>> write to page's data
>>> unlock_page()
>>>
>>> ...which neatly synchronizes with writeback and other fs activities.
>>
>> Ahem, I left out a key step: set_page_dirty()!
>>
>> lock_page()
>> write to page's data
>> set_page_dirty()
>> unlock_page()
>>
> 
> Excuse me, but Documentation/core-api/pin_user_pages.rst says
> "CASE 5: Pinning in order to _write_ to the data within the page"
> while tomoyo_dump_page() is for "_read_ the data within the page".
> Do we want to convert to pin_user_pages_remote() or lock_page() ?
> 

Sorry, I missed the direction here, was too focused on the Case 5
aspect. Yes. Case 5 (which, again, I think we're about to re-document)
is only about *writing* to data within the page.

So in this case, where it is just reading from the page, I think it's
already from a gup vs pup point of view.

btw, it's not clear to me whether the current code is susceptible to any
sort of problem involving something writing to the page while it
is being dumped (I am curious). But changing from gup to pup wouldn't
fix that, if it were a problem. It a separate question from this patch.

(Souptick, if you're interested, the Case 5 documentation change and
callsite retrofit is all yours if you want it. Otherwise it's on
my list.)

thanks,
-- 
John Hubbard
NVIDIA