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]

Message-ID: <20200821193100.GA38249@xz-x1>
Date:   Fri, 21 Aug 2020 15:31:00 -0400
From:   Peter Xu <peterx@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
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:31AM -0700, Linus Torvalds wrote:
> On Fri, Aug 21, 2020 at 11:08 AM Peter Xu <peterx@...hat.com> wrote:
> >
> > IIUC, Jan wanted to point out the fact that KSM didn't increase page count for
> > stable pages (reasons are above get_ksm_page()).
> 
> Ouch.
> 
> > Do we still at least need to check the swap count if PageSwapCache(page)?
> 
> No. Because a PageSwapCache() page should be a perfectly normal page
> cache thing. It will increment the page count if it's active.
> 
> That PageKsm() thing that *doesn't* increment the page could does look
> worrisome, but
> 
> > So we check page_count() (which covers KSM or normal pages) after we've got the
> > page lock, while we keep all the rest.
> 
> Why would we keep the rest? I
> 
> The actual thing I would really want to get rid of is the page lock,
> in fact. We shouldn't need it in this path, and it's the most
> expensive part of it all. But that's also why I did the page count
> test optimistically unlocked - because if page_count is 1, then we
> really shouldn't contend with anything else, so hopefully the
> (currently quite expensive) page locking is actually a non-issue once
> you get there.
> 
> But the PageKsm() page_count() issue I didn't even realize. That worries me.

That's definitely tricky.. Though IMHO that's another KSM topic that we might
want to look into later besides the current effort to fix up the COW breaking
issue for gup.  For now, it shouldn't be hard for us as long as we do cow
always for KSM pages.  However, do we really want to revert the whole logic of
52d1e606ee73?  Asking because it still seems to be an improvement to me (after
all we'll need to look after KSM pages here).  So it seems still good to keep.

Thanks,

-- 
Peter Xu