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] [day] [month] [year] [list]
Date:   Tue, 29 Dec 2020 12:52:58 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Kirill A. Shutemov" <kirill@...temov.name>
Cc:     Hugh Dickins <hughd@...gle.com>,
        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 Tue, Dec 29, 2020 at 5:28 AM Kirill A. Shutemov <kirill@...temov.name> wrote:
>
> I reworked do_fault_around() so it doesn't touch vmf->address and pass
> original address down to ->map_pages(). No need in the new argument in
> ->map_pages. filemap_map_pages() calculates address based on pgoff of the
> page handled.

So I really like how the end result looks, but it worries me how many
problems this patch has had, and I'd love to try to make it more
incremental.

In particular, I really liked your re-write of the loop in
filemap_map_pages(), and how you separated out the pmd case to be a
separate early case. I think that was some of the nicest part of the
patch.

And I really like how you changed the vmf->address meaning in this
latest version, and pass the real address down, and pass the range
just in the start/end_pgoff. That just looks like the RightThing(tm)
to do.

But I think these nice cleanups could and should be independent of
some of the other changes.

The unlocking change, for example, that changed the return type from
void to vm_fault_t, also looks like a nice cleanup, but that's also
the one that caused the last series of odd problems for Hugh.

The fix for that then also caused that ugly "goto out" mess, which
undid some of the "this looks really nice" effects.

I don't even see the need for that "goto out". Yes, it resets the
'vmf->address' back to what it should be (so that __do_fault()
fallback can work right), but the two first "goto out" cases haven't
actually changed it (or ther mmap_miss), so why did they need to do
that?

Anyway, I really like how it all looks (apart from that odd "goto out"
detail - nitpicking here) but it does worry me that it changes so much
and had so many small problems..

         Linus

Powered by blists - more mailing lists