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