Re: [Xen-devel] [PATCH v3 2/2] xen: merge temporary vcpu pinning scenarios

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/07/2019 12:42, Juergen Gross wrote:
> On 26.07.19 13:35, Jan Beulich wrote:
>> On 26.07.2019 11:46, Andrew Cooper wrote:
>>> On 24/07/2019 12:26, Juergen Gross wrote:
>>>> @@ -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.
>>
>> In which case - should we revert the commit until this is resolved?
>
> In my opinion it is not that urgent. I don't think any of our OSStests
> will ever be able to trigger this issue, as AFAIK no test is using the
> wait_event() interface nor do they test suspend/resume. And both need
> to be true (at the same time!) plus a cpu needs to fail coming up when
> resuming again.

Yeah - I don't think reverting it is necessary, but I will flag
"resolving this somehow" as a 4.12 blocker.

The HVI scale tests trigger this path.  Guess how I discovered that
Introspection + Livepatching = boom.

I am leaning on the side of panic().  I agree that if the APIs are used
correctly, it can't occur.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel