Re: [PATCH v13 0/5] UFFD write-tracking migration/snapshots

[Peter Xu <peterx@redhat.com>]

On Thu, Feb 11, 2021 at 08:01:29PM +0100, David Hildenbrand wrote:
> On 11.02.21 19:28, Andrey Gruzdev wrote:
> > On 11.02.2021 20:32, Peter Xu wrote:
> > > On Thu, Feb 11, 2021 at 07:19:47PM +0300, Andrey Gruzdev wrote:
> > > > On 09.02.2021 22:06, David Hildenbrand wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > just stumbled over this, quick question:
> > > > > > > 
> > > > > > > I recently played with UFFD_WP and notices that write protection is
> > > > > > > only effective on pages/ranges that have already pages populated (IOW:
> > > > > > > !pte_none() in the kernel).
> > > > > > > 
> > > > > > > In case memory was never populated (or was discarded using e.g.,
> > > > > > > madvice(DONTNEED)), write-protection will be skipped silently and you
> > > > > > > won't get WP events for applicable pages.
> > > > > > > 
> > > > > > > So if someone writes to a yet unpoupulated page ("zero"), you won't
> > > > > > > get WP events.
> > > > > > > 
> > > > > > > I can spot that you do a single uffd_change_protection() on the whole
> > > > > > > RAMBlock.
> > > > > > > 
> > > > > > > How are you handling that scenario, or why don't you have to handle
> > > > > > > that scenario?
> > > > > > > 
> > > > > > Hi David,
> > > > > > 
> > > > > > I really wonder if such a problem exists.. If we are talking about a
> > > > > I immediately ran into this issue with my simplest test cases. :)
> > > > > 
> > > > > > write to an unpopulated page, we should get first page fault on
> > > > > > non-present page and populate it with protection bits from
> > > > > > respective vma.
> > > > > > For UFFD_WP vma's  page will be populated non-writable. So we'll get
> > > > > > another page fault on present but read-only page and go to
> > > > > > handle_userfault.
> > > > > See the attached test program. Triggers for me on 5.11.0-rc6+ and
> > > > > 5.9.13-200.fc33
> > > > > 
> > > > > gcc -lpthread uffdio_wp.c -o uffdio_wp
> > > > > ./uffdio_wp
> > > > > WP did not fire
> > > > > 
> > > > > Uncomment the placement of the zeropage just before registering to make
> > > > > the WP actually trigger. If there is no PTE, there is nothing to
> > > > > protect.
> > > > > 
> > > > > 
> > > > > And it makes sense: How should the fault handler know which ranges you
> > > > > wp-ed, if there is no place to store that information (the PTEs!). The
> > > > > VMA cannot tell that story, it only knows that someone registered
> > > > > UFFD_WP to selectively wp some parts.
> > > > > 
> > > > > You might have to register also for MISSING faults and place zero pages.
> > > > > 
> > > > Looked at the kernel code, agree that we miss WP events for unpopulated
> > > > pages, UFFD_WP softbit won't be set in this case. But it doesn't make saved
> > > > snapshot inconsistent or introduce security issues. The only side effect is
> > > > that we may save updated page instead of zeroed, just increasing snapshot
> > > > size. However this guest-physical page has never been touched from the point
> > > > of view of saved vCPU/device state and is not a concern.
> > > Oh I just remembered one thing, that Linux should be zeroing pages when
> > > allocating, so even if the page has legacy content it'll be cleared with
> > > __GFP_ZERO allocations.  So yeah it would be harder to have issue at least with
> > > a sensible OS.  I'm not sure about Windows or others, but it could be a common
> > > case.  Then the only overhead is the extra pages we kept in the live snapshot,
> > > which takes some more disk space.
> > > 
> > > Or there could be firmware running without OS at all, but it should really not
> > > read unallocated pages assuming there must be zero.  It's not a sane behavior
> > > even for a firmware.
> > > 
> > > > Often (at least on desktop Windows guests) only a small part of RAM has ever
> > > > been allocated by guest. Migration code needs to read each guest-physical
> > > > page, so we'll have a lot of additional UFFD events, much more MISSING
> > > > events then WP-faults.
> > > > 
> > > > And the main problem is that adding MISSING handler is impossible in current
> > > > single-threaded snapshot code. We'll get an immediate deadlock on iterative
> > > > page read.
> > > Right.  We'll need to rework the design but just for saving a bunch of snapshot
> > > image disk size.  So now I agree with you, let's keep this in mind, but maybe
> > > it isn't worth a fix for now, at least until we figure something really broken.
> > > 
> > > Andrey, do you think we should still mention this issue into the todo list of
> > > the wiki page of live snapshot?
> > > 
> > > Thanks,
> > > 
> > Yes, even if the page happens to be overwritten, it's overwritten by the same VM so
> > no security boundaries are crossed. And no machine code can assume that RAM content
> > is zeroed on power-on or reset so our snapshot state stays quite consistent.
> > 
> > Agree we should keep it in mind, but IMHO adding MISSING handler and running separate
> > thread would make performance worse.. So I doubt it's worth adding this to TODO list..
> > 
> 
> So, here is what happens: your snapshot will contain garbage at places where
> it should contain zero.
> 
> This happens when your guest starts using an unpopulated page after
> snapshotting started and the page has not been copied to the snapshot yet.
> You won't get a WP event, therefore you cannot copy "zero" to the snapshot
> before content gets overridden.
> 
> If you load your snapshot, it contains garbage at places that are supposed
> to contain zero.
> 
> 
> There is a feature in virtio-balloon that *depends* on previously discarded
> pages from staying unmodified in some cases: free page reporting.
> 
> The guest will report pages (that might have been initialized to zero) to
> the hypervisor). The hypervisor will discard page content if the content was
> initialized to zero. After that, the guest is free to reuse the pages
> anytime with the guarantee that content has not been modified (-> still is
> zero).
> 
> 
> See QEMU hw/virtio/virtio-balloon.c: virtio_balloon_handle_report()
> 
> "When we discard the page it has the effect of removing the page from the
> hypervisor itself and causing it to be zeroed when it is returned to us. So
> we must not discard the page [...] if the guest is expecting it to retain a
> non-zero value."
> 
> And Linux drivers/virtio/virtio_balloon.c: virtballoon_validate()
> 
> "Inform the hypervisor that our pages are poisoned or initialized. If we
> cannot do that then we should disable page reporting as it could potentially
> change the contents of our free pages."
> 
> 
> It's as simple as having a Linux guest with init_on_free and
> free-page-reporting active via virtio-balloon.
> 
> Long story short, your feature might break guests (when continuing the
> snapshot) when free page reporting is active. I agree that MISSING events
> might be too heavy, so you should disable snapshots if free page reporting
> is active.

When the page is reported with poison_val set, it is written already by the
guest, right (either all zeros, or some special marks)?  If so, why it won't be
wr-protected by uffd?  Thanks,

-- 
Peter Xu