Re: [PATCH v6 04/10] xen: Introduce XENMEM_soft_reset operation

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Tim Deegan <tim@xen.org> writes:

> At 12:06 +0200 on 25 May (1432555565), Vitaly Kuznetsov wrote:
>> Tim Deegan <tim@xen.org> writes:
>> 
>> > At 17:26 +0100 on 22 May (1432315574), Jan Beulich wrote:
>> >> >>> On 22.05.15 at 17:36, <vkuznets@redhat.com> wrote:
>> >> >>>>> On 13.05.15 at 11:49, <vkuznets@redhat.com> wrote:
>> >> >>> +    if ( !source_d->is_dying )
>> >> >>> +    {
>> >> >>> +        /*
>> >> >>> +         * Make sure no allocation/remapping for the source domain is ongoing
>> >> >>> +         * and set is_dying flag to prevent such actions in future.
>> >> >>> +         */
>> >> >>> +        spin_lock(&source_d->page_alloc_lock);
>> >> >>> +        source_d->is_dying = DOMDYING_locked;
>> >> >>
>> >> >> Furthermore I don't see how this prevents there being further
>> >> >> changes to the GFN <-> MFN mappings for the guest (yet you rely
>> >> >> on them to be stable below afaict).
>> >> >>
>> >> > 
>> >> > As you suggested below we can complement that by pausing both source and
>> >> > destination domains here. In that case these domains won't be changing
>> >> > their mappings by themselves but it would still be possible for the
>> >> > hypervisor to change something. We do have !d->is_dying check in many
>> >> > places though ... In theory we could have taken the p2m_lock() for both
>> >> > domains but I'm not sure all stuff I need here will cope with p2m_lock()
>> >> > already being held, this lock is x86-specific and I'm not sure we want
>> >> > it in the first place. I'd be very grateful for some additional ideas on
>> >> > how to make it safer.
>> >> 
>> >> For whether p2m_lock() might be needed here I'd like to defer to
>> >> Tim.
>> >
>> > I don't think that will work.  Given that you have to make this
>> > preemptable you can't hope to hold the p2m lock for the entire
>> > operation.
>> >
>> > If you want to make sure that the p2m mapping doesn't change
>> > underfoot, you can use get_gfn()/put_gfn() around each individual
>> > operation.
>> 
>> Thanks, I see... An additional concern from Jan was (I suppose) about
>> the safeness (or correctness) of this operation as a whole: even when
>> both source and destination domains are paused their mappings can be
>> changed by the control domain (especially having possible preemption in
>> mind).
>
> True.  I don't think there's anything you can do about that.  As I
> said, you can't hold the p2m lock while your operation is
> preempted.  AFAICT this operation is pretty much best-effort already;
> it doesn't really handle grant-table/shared/paged/readonly/access-controlled
> mappings.

This is intentional mostly intentional and desired: when a Linux guest
does e.g. kexec there is no easy way to transfer the knowledge from one
kernel to another. This operation is supposed to emulate a normal reboot
for a domain where all these special mappings are being lost.

>
>> >  If you use get_gfn_type_access() it will also report
>> > superpage mappings, so you can drop the loop that attempts
>> > to detect them in the PoD case.
>> 
>> Thanks, I'll have a look. This one is x86-specific but the whole PoD
>> case is already x86-specific because of p2m_is_pod().
>
> It might be worth adding this interface to arm, depending on
> what you want to do about p2m-access types.  E.g. can a guest OS use
> this kexec operation to turn off all its p2m-access controls (and so
> escape from the control of an external security tool)?

Good question. We could definitely do
p2m_get_mem_access/p2m_set_mem_access to preserve this information but
I'm not sure we really need this. What happens when a domain reboots? It
gets new p2m and all p2m-access controls are being lost so all external
security tools are supposed to re-establish all the required p2m-access
controls. The only difference with soft-reset is the fact that the
memory content is being preserved but the same effect could in theory be 
achieved by the guest domain itself by e.g. dumping all memory content
to a persistent storage, rebooting and restoring it.

>
>> > Incidentally, counting from 0-->max_mapped_gfn is unfortunate (though I
>> > know this is not the first code to do that).  It would be better
>> > to introduce an iterator over the p2m itself, either some sort of
>> > for_each_gfn(dom, callback_fn, callback_arg) or a get_next_gfn(dom,
>> > base_gfn, &found_gfn) that skips unmapped areas.
>> 
>> I agree. Do you see this as a compulsory part of this series?
>
> I don't think so; not in something like this form anyway.  If you
> wanted to handle the source p2m changing underfoot by re-scanning for
> new entries, then you'd definitely want it.

No, not really. Protecting against a misbehaving control domain
(guaranteeing the correctness of the operation) is going
to be incomplete anyway. I think we just need to make this operation
safe so even the control domain won't be able to compromise the
hypervisor by doing something nasty.

>
> Tim.

-- 
  Vitaly