On 26/07/2019 10:53, Juergen Gross wrote:
> On 26.07.19 11:46, Andrew Cooper wrote:
>> On 24/07/2019 12:26, Juergen Gross wrote:
>>> diff --git a/xen/common/wait.c b/xen/common/wait.c
>>> index 4f830a14e8..3fc5f68611 100644
>>> --- a/xen/common/wait.c
>>> +++ b/xen/common/wait.c
>>> @@ -34,8 +34,6 @@ struct waitqueue_vcpu {
>>> */
>>> void *esp;
>>> char *stack;
>>> - cpumask_t saved_affinity;
>>> - unsigned int wakeup_cpu;
>>> #endif
>>> };
>>> @@ -131,12 +129,10 @@ static void __prepare_to_wait(struct
>>> waitqueue_vcpu *wqv)
>>> ASSERT(wqv->esp == 0);
>>> /* Save current VCPU affinity; force wakeup on *this* CPU
>>> only. */
>>> - wqv->wakeup_cpu = smp_processor_id();
>>> - cpumask_copy(&wqv->saved_affinity, curr->cpu_hard_affinity);
>>> - if ( vcpu_set_hard_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
>>> + if ( vcpu_temporary_affinity(curr, smp_processor_id(),
>>> VCPU_AFFINITY_WAIT) )
>>> {
>>> gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
>>> - domain_crash(current->domain);
>>> + domain_crash(curr->domain);
>>> for ( ; ; )
>>> do_softirq();
>>> @@ -170,7 +166,7 @@ static void __prepare_to_wait(struct
>>> waitqueue_vcpu *wqv)
>>> if ( unlikely(wqv->esp == 0) )
>>> {
>>> gdprintk(XENLOG_ERR, "Stack too large in %s\n", __func__);
>>> - domain_crash(current->domain);
>>> + domain_crash(curr->domain);
>>> for ( ; ; )
>>> do_softirq();
>>> @@ -182,30 +178,24 @@ static void __prepare_to_wait(struct
>>> waitqueue_vcpu *wqv)
>>> static void __finish_wait(struct waitqueue_vcpu *wqv)
>>> {
>>> wqv->esp = NULL;
>>> - (void)vcpu_set_hard_affinity(current, &wqv->saved_affinity);
>>> + vcpu_temporary_affinity(current, NR_CPUS, VCPU_AFFINITY_WAIT);
>>> }
>>> void check_wakeup_from_wait(void)
>>> {
>>> - struct waitqueue_vcpu *wqv = current->waitqueue_vcpu;
>>> + struct vcpu *curr = current;
>>> + struct waitqueue_vcpu *wqv = curr->waitqueue_vcpu;
>>> ASSERT(list_empty(&wqv->list));
>>> if ( likely(wqv->esp == NULL) )
>>> return;
>>> - /* Check if we woke up on the wrong CPU. */
>>> - if ( unlikely(smp_processor_id() != wqv->wakeup_cpu) )
>>> + /* Check if we are still pinned. */
>>> + if ( unlikely(!(curr->affinity_broken & VCPU_AFFINITY_WAIT)) )
>>> {
>>> - /* Re-set VCPU affinity and re-enter the scheduler. */
>>> - struct vcpu *curr = current;
>>> - cpumask_copy(&wqv->saved_affinity, curr->cpu_hard_affinity);
>>> - if ( vcpu_set_hard_affinity(curr,
>>> cpumask_of(wqv->wakeup_cpu)) )
>>> - {
>>> - gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
>>> - domain_crash(current->domain);
>>> - }
>>> - wait(); /* takes us back into the scheduler */
>>> + gdprintk(XENLOG_ERR, "vcpu affinity lost\n");
>>> + domain_crash(curr->domain);
>>> }
>>
>> I'm sorry to retract my R-by after the fact, but I've only just noticed
>> (while rebasing some of my pending work over this) that it is buggy.
>>
>> The reason wait() was called is because it is not safe to leave that
>> if() clause.
>>
>> With this change in place, we'll arrange for the VM to be crashed, then
>> longjump back into the stack from from the waiting vCPU, on the wrong
>> CPU. Any caller with smp_processor_id() or thread-local variables cache
>> by pointer on the stack will then cause memory corruption.
>>
>> Its not immediately obvious how to fix this, but bear in mind that as
>> soon as the vm-event interface is done, I plan to delete this whole
>> waitqueue infrastructure anyway.
>
> Shouldn't just calling wait() after domain_crash() be fine then?
>
> That's what would have happened in the original error case, too.
No - I don't think so. That was to try and get back into a position
where the scheduler rescheduled this vcpu on the correct cpu, so it
could safely longjmp back into context.
With the domain crash here, nothing will happen[1] until we do
successfully longjmp() back into context, because we've got a stack
frame which needs unwinding before it is safe to start cleaning the
domain up.
~Andrew
[1] If something other than nothing happens, then we've got a
refcounting issue...
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
