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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 5 Jan 2021 13:55:54 -0500
From:   Andrea Arcangeli <aarcange@...hat.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     Nadav Amit <nadav.amit@...il.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Nadav Amit <namit@...are.com>,
        Yu Zhao <yuzhao@...gle.com>, Andy Lutomirski <luto@...nel.org>,
        Pavel Emelyanov <xemul@...nvz.org>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        Minchan Kim <minchan@...nel.org>,
        Will Deacon <will@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [RFC PATCH v2 1/2] mm/userfaultfd: fix memory corruption due to
 writeprotect

On Tue, Jan 05, 2021 at 01:41:34PM -0500, Peter Xu wrote:
> Agreed. I didn't mention uffd_wp check (which I actually mentioned in the reply
> to v1 patchset) here only because the uffd_wp check is pure optimization; while

Agreed it's a pure optimization.

Only if we used the group lock to fix this (which we didn't since it
wouldn't help clear_refs to avoid the performance regression), the
optimization would have become not an optimization anymore.

> the uffd_wp_resolve check is more critical because it is potentially a fix of
> similar tlb flushing issue where we could have demoted the pte without being
> noticed, so I think it's indeed more important as Nadav wanted to fix in the
> same patch.

I didn't get why that was touched in the same patch, I already
suggested to remove that optimization...

> It would be even nicer if we have both covered (all of them can be in
> unlikely() as Andrea suggested in the other email), then maybe nicer as a
> standalone patch, then mention about the difference of the two in the commit
> log (majorly, the resolving change will be more than optimization).

Yes, if you want to go ahead optimizing both cases of the
UFFDIO_WRITEPROTECT, I don't think there's any dependency on this. The
huge_memory.c also needs covering but I didn't look at it, hopefully
the code will result as clean as in the pte case.

I'll try to cleanup the tlb flush in the meantime to see if it look
maintainable after the cleanups.

Then we can change it to wait_pending_flush(); return VM_FAULT_RETRY
model if we want to or if the IPI is slower, at least clear_refs will
still not block on random pagein or swapin from disk, but only anon
memory write access will block while clear_refs run.

Thanks,
Andrea

Powered by blists - more mailing lists