linux-kernel - Re: [PATCH] mm: reuse the unshared swapcache page in do_wp

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: <CAHk-=wi21DZ4H5uLnn2QgAeAUqg0wNPboijC0OgDDk1e7TdkPw@mail.gmail.com>
Date:   Thu, 13 Jan 2022 09:14:48 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     David Hildenbrand <david@...hat.com>
Cc:     Matthew Wilcox <willy@...radead.org>,
        Liang Zhang <zhangliang5@...wei.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux-MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        wangzhigang17@...wei.com
Subject: Re: [PATCH] mm: reuse the unshared swapcache page in do_wp_page

On Thu, Jan 13, 2022 at 8:48 AM David Hildenbrand <david@...hat.com> wrote:
>
> I'm wondering if we can get rid of the mapcount checks in
> reuse_swap_page() and instead check for page_count() and swapcount only.

Honestly, I think even checking page_count() is pointless.

If the page has users, then that's fine. That's irrelevant for whether
it's a swap page or not, no?

So if you want to remove it from the swap cache, all that matters is that

 (a) you have it locked so that there can be no new users trying to mix it up

 (b) there are no swapcount() users of this page (which don't have a
ref to the page itself, they only have a swap entry), so that you
can't have somebody trying to look it up (whether some racy
"concurrent" lookup _or_ any later one, since we're about to remove
the swap cache association).

Why would "map_count()" matter - it's now many times the *page* is
mapped, it's irrelevant to swap cache status? And for the same reason,
what difference does "page_count()" have?

One big reason I did the COW rewrite was literally that the rules were
pure voodoo and made no sense at all. There was no underlying logic,
it was just a random collection of random tests that didn't have any
logical architecture to them.

Our VM is really really complicated already, so I really want our code
to make sense.

So even if I'm entirely wrong in my swap/map-count arguments above,
I'd like whatever patches in this area to be really well commented and
have some fundamental rules and logic to them so that people can read
the code and go "Ok, makes sense".

Please?

         Linus