linux-kernel - Re: [PATCH v3] mm/gup: Allow real explicit breaking of COW

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:   Fri, 21 Aug 2020 12:05:19 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Peter Xu <peterx@...hat.com>
Cc:     Jan Kara <jack@...e.cz>, Andrea Arcangeli <aarcange@...hat.com>,
        Linux-MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Marty Mcfadden <mcfadden8@...l.gov>,
        "Maya B . Gokhale" <gokhale2@...l.gov>,
        Jann Horn <jannh@...gle.com>, Christoph Hellwig <hch@....de>,
        Oleg Nesterov <oleg@...hat.com>,
        Kirill Shutemov <kirill@...temov.name>
Subject: Re: [PATCH v3] mm/gup: Allow real explicit breaking of COW

On Fri, Aug 21, 2020 at 11:23 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> But the PageKsm() page_count() issue I didn't even realize. That worries me.

Well, the fix is simple, although I don't love the magic PageKsm
semantics that hide it from the page count.

But since (a) a Ksm page is presumably normally shared (ie things like
all zeroes) and (b) copying should always be safe, just do that.

The case we *used* to have with trying to reuse the KSM page seems
like it's not just adding complexity, it's optimizing for entirely the
wrong case.

Check both before and after getting the page lock, for the same reason
we do it for the page count.

The logic there matches the "reuse swap page", but while that old
logic may have made sense 20 years ago, the swap cache case should be
*so* rare these days that it feels completely pointless to try to
reuse it.

Aggressively doing a new allocation, copy, and freeing the old swap
cache page is quite possibly cheaper than taking the page lock anyway,
but more importantly, it's not a case that should normally trigger in
the first place.

That said, looking at this code again, I get the feeling that the
mapcount check is pointless.

Afaik, page_count() should always be larger than page_mapcount(), so
if mapcount is > 1, then we'd have caught it with the page_count()
check.

Hmm? Am I popssibly missing some other subtle special case?

Are there any THP issues? Again, doing the copy should always be the
safe thing to do, and since we get the page lock for the reuse case I
think we're ok on that front.

What else possible special cases could we hit?

                Linus