Re: [PATCH v3 2/3] userfaultfd: UFFDIO_MOVE uABI

[Peter Xu]

David,

On Mon, Oct 16, 2023 at 08:01:10PM +0200, David Hildenbrand wrote:
> [...]
> 
> > > > Actually, even though I have no solid clue, but I had a feeling that there
> > > > can be some interesting way to leverage this across-mm movement, while
> > > > keeping things all safe (by e.g. elaborately requiring other proc to create
> > > > uffd and deliver to this proc).
> > > 
> > > Okay, but no real use cases yet.
> > 
> > I can provide a "not solid" example.  I didn't mention it because it's
> > really something that just popped into my mind when thinking cross-mm, so I
> > never discussed with anyone yet nor shared it anywhere.
> > 
> > Consider VM live upgrade in a generic form (e.g., no VFIO), we can do that
> > very efficiently with shmem or hugetlbfs, but not yet anonymous.  We can do
> > extremely efficient postcopy live upgrade now with anonymous if with REMAP.
> > 
> > Basically I see it a potential way of moving memory efficiently especially
> > with thp.
> 
> It's an interesting use case indeed. The questions would be if this is (a) a
> use case we want to support; (b) why we need to make that decision now and
> add that feature.

I would like to support that if nothing stops it from happening, but that's
what we're discussing though..

For (b), I wanted to avoid UFFD_FEATURE_MOVE_CROSS_MM feature flag just for
this, if they're already so close, not to mention current code already
contains cross-mm support.

If to support that live upgrade use case, I'd probably need to rework tlb
flushing too to do the batching (actually tlb flush is not even needed for
upgrade scenario..).  I'm not sure whether Lokesh's use case would move
large chunks, it'll be perfect if Suren did it altogether.  But that one is
much easier if transparent to userapps.  Cross-mm is not transparent and
need another feature knob, which I want to avoid if possible.

> 
> One question is if this kind of "moving memory between processes" really
> should be done, because intuitively SHMEM smells like the right thing to use
> here (two processes wanting to access the same memory).

That's the whole point, IMHO, where shmem cannot be used.  As you said, on
when someone cannot use file memory for some reason like ksm.

> 
> The downsides of shmem are lack of the shared zeropage and KSM. The shared
> zeropage is usually less of a concern for VMs, but KSM is. However, KSM will
> also disallow moving pages here. But all non-deduplicated ones could be
> moved.
> 
> [I wondered whether moving KSM pages (rmap items) could be done; probably in
> some limited form with some more added complexity]

Yeah we can leave that complexity for later when really needed.  Here
cross-mm support, OTOH, isn't making it so complicated, IMHO.

Btw, we don't even necessarily need to be able to migrate KSM pages for a
VM live upgrade use case: we can unmerge the pages, upgrade, and wait for
KSM to scan & merge again on the new binary / mmap.  Userspace can have
that control easily, afaiu, via existing madvise().

> 
> > 
> > > 
> > > > 
> > > > Considering Andrea's original version already contains those bits and all
> > > > above, I'd vote that we go ahead with supporting two MMs.
> > > 
> > > You can do nasty things with that, as it stands, on the upstream codebase.
> > > 
> > > If you pin the page in src_mm and move it to dst_mm, you successfully broke
> > > an invariant that "exclusive" means "no other references from other
> > > processes". That page is marked exclusive but it is, in fact, not exclusive.
> > 
> > It is still exclusive to the dst mm?  I see your point, but I think you're
> > taking exclusiveness altogether with pinning, and IMHO that may not be
> > always necessary?
> 
> That's the definition of PAE. See do_wp_page() on when we reset PAE: when
> there are no other references, which implies no other references from other
> processes. Maybe you have "currently exclusively mapped" in mind, which is
> what the mapcount can be used for.

Okay.

> 
> > 
> > > 
> > > Once you achieved that, you can easily have src_mm not have MMF_HAS_PINNED,
> > 
> > (I suppose you meant dst_mm here)
> 
> Yes.
> 
> > 
> > > so you can just COW-share that page. Now you successfully broke the
> > > invariant that COW-shared pages must not be pinned. And you can even trigger
> > > VM_BUG_ONs, like in sanity_check_pinned_pages().
> > 
> > Yeah, that's really unfortunate.  But frankly, I don't think it's the fault
> > of this new feature, but the rest.
> > 
> > Let's imagine if the MMF_HAS_PINNED wasn't proposed as a per-mm flag, but
> > per-vma, which I don't see why we can't because it's simply a hint so far.
> > Then if we apply the same rule here, UFFDIO_REMAP won't even work for
> > single-mm as long as cross-vma. Then UFFDIO_REMAP as a whole feature will
> > be NACKed simply because of this..
> 
> Because of gup-fast we likely won't see that happening. And if we would, it
> could be handled (src_mm has the flag set, set it on the destination if the
> page maybe pinned after hiding it from gup-fast; or simply always copy the
> flag if set on the src).
> 
> > 
> > And I don't think anyone can guarantee a per-vma MMF_HAS_PINNED can never
> > happen, or any further change to pinning solution that may affect this.  So
> > far it just looks unsafe to remap a pin page to me.
> 
> It may be questionable to allow remapping pinned pages.
> 
> > 
> > I don't have a good suggestion here if this is a risk.. I'd think it risky
> > then to do REMAP over pinned pages no matter cross-mm or single-mm.  It
> > means probably we just rule them out: folio_maybe_dma_pinned() may not even
> > be enough to be safe with fast-gup.  We may need page_needs_cow_for_dma()
> > with proper write_protect_seq no matter cross-mm or single-mm?
> 
> If you unmap and sync against GUP-fast, you can check after unmapping and
> remap and fail if it maybe pinned afterwards. Plus an early check upfront.
> 
> > 
> > > 
> > > Can it all be fixed? Sure, with more complexity. For something without clear
> > > motivation, I'll have to pass.
> > 
> > I think what you raised is a valid concern, but IMHO it's better fixed no
> > matter cross-mm or single-mm.  What do you think?
> 
> single-mm should at least not cause harm, but the semantics are
> questionable. cross-mm could, especially with malicious user space that
> wants to find ways of harming the kernel.

For kernel, I think we're discussing to see whether it's safe to do so from
kernel pov, e.g., whether to exclude pinned pages is part of that.

For the user app, the dest process has provided the uffd descriptor on its
own willingness, or be a child of the UFFDIO_MOVE issuer when used with
EVENT_FORK, I assume that's already some form of safety check because it
cannot be any process, but ones that are proactively in close cooperative
with the issuer process.

> 
> I'll note that mremap with pinned pages works.

But that's not "by design", am I right?  IOW, do we have any real pin user
that rely on mremap() allowing pages to be moved?

I don't see any word guarantee at least from man page that mremap() will
make sure the PFN won't change after the movement.. even though it seems to
be what happening now.

Neither do I think when designing MMF_HAS_PINNED we kept that in mind that
it won't be affected by someone mremap() pinned pages and we want to keep
it working..

All of it just seems to be an accident..

One step back, we're free to define UFFDIO_MOVE anyway, and we don't
necessarily need to always follow mremap().  E.g., mremap() also supports
ksm pages, but IIUC we already decide to not support that for now on
UFFDIO_MOVE.  UFFDIO_MOVE seems all fine to make it clear on failing at
pinned pages from the 1st day if that satisfies our goals, too.

> 
> > 
> > In general, pinning lose its whole point here to me for an userspace either
> > if it DONTNEEDs it or REMAP it.  What would be great to do here is we unpin
> > it upon DONTNEED/REMAP/whatever drops the page, because it loses its
> > coherency anyway, IMHO.
> 
> Further, moving a part of a THP would fail either way, because the pinned
> THP cannot get split.
> 
> -- 
> Cheers,
> 
> David / dhildenb
> 

Thanks,

-- 
Peter Xu