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

[Tim Deegan <tim@xxxxxxx>]

At 12:06 +0200 on 25 May (1432555565), Vitaly Kuznetsov wrote:
> Tim Deegan <tim@xxxxxxx> writes:
> 
> > At 17:26 +0100 on 22 May (1432315574), Jan Beulich wrote:
> >> >>> On 22.05.15 at 17:36, <vkuznets@xxxxxxxxxx> wrote:
> >> >>>>> On 13.05.15 at 11:49, <vkuznets@xxxxxxxxxx> 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.

> >  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)?

> > 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.

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel