Re: [PATCH v1 4/7] mm/ksm: fix KSM COW breaking with userfaultfd-wp via FAULT_FLAG_UNSHARE

[David Hildenbrand]

Hi Peter,

sorry for replying so late, I only managed now to get back to this patch 
set.

> > Yes, I can give it a try. What I dislike about ksm_test is that it's a
> > mixture of benchmarks and test cases that have to explicitly triggered by
> > parameters. It's not a simple "run all available test cases" tests as we
> > know it. So maybe something separate (or having it as part of the uffd
> > tests) makes more sense.
>
> We can add an entry into run_vmtests.sh.  That's also what current ksm_test
> does.

Right, but I kind-of don't like that way at all and would much rather do 
it cleaner...

> Yes adding into uffd test would work too, but I do have a plan that we
> should move functional tests out of userfaultfd.c, leaving that with the
> stress test only.  Not really a big deal, though.

... similar to what you want to do with userfaultfd.c

So maybe I'll just add a new test for ksm functional tests.

> > > > Consequently, we will no longer trigger a fake write fault and break COW
> > > > without any such side-effects.
> > > >
> > > > This is primarily a fix for KSM+userfaultfd-wp, however, the fake write
> > > > fault was always questionable. As this fix is not easy to backport and it's
> > > > not very critical, let's not cc stable.
> > >
> > > A patch to cc most of the stable would probably need to still go with the
> > > old write approach but attaching ALLOW_RETRY.  But I agree maybe that may
> > > not need to bother, or a report should have arrived earlier..  The unshare
> > > approach looks much cleaner indeed.
> >
> > A fix without FAULT_FLAG_UNSHARE is not straight forward. We really don't
> > want to notify user space about write events here (because there is none).
> > And there is no way around the uffd handling in WP code without that.
> >
> > FAULT_FLAG_ALLOW_RETRY would rely on userfaultfd triggering and having to
> > resolve the WP event.
>
> Right it'll be very much a false positive, but the userspace should be fine
> with it e.g. for live snapshot we need to copy page earlier; it still won't
> stop the process from running along the way.  But I agree that's not ideal.

At least the test case at hand will wait until infinitely, because there 
is no handler that would allow break_cow to make progress (well, we 
don't expect write events in the test :) ).

Anyhow, I don't think messing with that for stable kernels is worth the 
pain / complexity / possible issues.

--
Thanks,

David / dhildenb