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  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:   Wed, 16 Dec 2020 10:41:36 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Kirill A. Shutemov" <kirill@...temov.name>
Cc:     Matthew Wilcox <willy@...radead.org>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Will Deacon <will@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Jan Kara <jack@...e.cz>, Minchan Kim <minchan@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Vinayak Menon <vinmenon@...eaurora.org>,
        Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [PATCH 1/2] mm: Allow architectures to request 'old' entries when prefaulting

On Wed, Dec 16, 2020 at 9:07 AM Kirill A. Shutemov <kirill@...temov.name> wrote:
>
> If this looks fine, I'll submit a proper patch.

That patch looks good to me.

It would be good if somebody else looked it through - maybe I like it
just because I got to pee in the snow and make my mark. But i think
that filemap_map_pages() now looks a lot more understandable, and
having that pte_offset_map_lock() outside the loop should be good.

In particular, it will make it much easier to then make arguments
about things like

 - increase the page mapping count

 - check that the page is not locked

 - check that the page still has a mapping

we can say "we know we hold the page table lock, and we increased the
page mapping count, and we saw that the page still had a mapping and
was not locked after that".

And that means that with some trivial memory ordering rules, we can
know that any concurrent "truncate()" that got the page lock after we
checked it, will get held up at

        if (page_mapped(page)) {
                unsigned int nr = thp_nr_pages(page);
                unmap_mapping_pages(mapping, page->index, nr, false);
        }

in truncate_cleanup_page(), and that means that we can safely map the
page _without_ ever actually even doing a "trylock_page()" at all.

Before this patch of yours, that logic would have been broken, because
the page table lock wasn't actually held for the whole first iteration
of that loop in filemap_map_pages().

And getting rid of the trylock_page() in that path gets rid of _the_
major page lock  case for at least one class of loads.

So I like the patch. But I would _really_ like for somebody else to
look at it, and look at my thinking above.

Because maybe I'm missing something.

                Linus

Powered by blists - more mailing lists