linux-kernel - Re: [PATCH 1/5] mm: Introduce mm_struct.has

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:   Tue, 22 Sep 2020 11:02:03 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Peter Xu <peterx@...hat.com>
CC:     <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jan Kara <jack@...e.cz>, Michal Hocko <mhocko@...e.com>,
        Kirill Tkhai <ktkhai@...tuozzo.com>,
        Kirill Shutemov <kirill@...temov.name>,
        Hugh Dickins <hughd@...gle.com>,
        Christoph Hellwig <hch@....de>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Leon Romanovsky <leonro@...dia.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        "Jann Horn" <jannh@...gle.com>
Subject: Re: [PATCH 1/5] mm: Introduce mm_struct.has_pinned

On 9/22/20 8:17 AM, Peter Xu wrote:
> On Mon, Sep 21, 2020 at 04:53:38PM -0700, John Hubbard wrote:
>> On 9/21/20 2:17 PM, Peter Xu wrote:
>>> (Commit message collected from Jason Gunthorpe)
>>>
>>> Reduce the chance of false positive from page_maybe_dma_pinned() by keeping
>>
>> Not yet, it doesn't. :)  More:
>>
>>> track if the mm_struct has ever been used with pin_user_pages(). mm_structs
>>> that have never been passed to pin_user_pages() cannot have a positive
>>> page_maybe_dma_pinned() by definition. This allows cases that might drive up
>>> the page ref_count to avoid any penalty from handling dma_pinned pages.
>>>
>>> Due to complexities with unpining this trivial version is a permanent sticky
>>> bit, future work will be needed to make this a counter.
>>
>> How about this instead:
>>
>> Subsequent patches intend to reduce the chance of false positives from
>> page_maybe_dma_pinned(), by also considering whether or not a page has
>> even been part of an mm struct that has ever had pin_user_pages*()
>> applied to any of its pages.
>>
>> In order to allow that, provide a boolean value (even though it's not
>> implemented exactly as a boolean type) within the mm struct, that is
>> simply set once and never cleared. This will suffice for an early, rough
>> implementation that fixes a few problems.
>>
>> Future work is planned, to provide a more sophisticated solution, likely
>> involving a counter, and *not* involving something that is set and never
>> cleared.
> 
> This looks good, thanks.  Though I think Jason's version is good too (as long
> as we remove the confusing sentence, that's the one starting with "mm_structs
> that have never been passed... ").  Before I drop Jason's version, I think I'd
> better figure out what's the major thing we missed so that maybe we can add
> another paragraph.  E.g., "future work will be needed to make this a counter"
> already means "involving a counter, and *not* involving something that is set
> and never cleared" to me... Because otherwise it won't be called a counter..
> 

That's just a bit of harmless redundancy, intended to help clarify where this
is going. But if the redundancy isn't actually helping, you could simply
truncate it to the first half of the sentence, like this:

"Future work is planned, to provide a more sophisticated solution, likely
involving a counter."


thanks,
-- 
John Hubbard
NVIDIA