Re: [PATCH v2] x86/vpmu: Fix race-condition in vpmu_load

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.09.2022 16:28, Tamas K Lengyel wrote:
> On Thu, Sep 29, 2022 at 9:07 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
>> On 26.09.2022 16:22, Tamas K Lengyel wrote:
>>> On Mon, Sep 26, 2022 at 10:12 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 22.09.2022 22:48, Tamas K Lengyel wrote:
>>>>> --- a/xen/arch/x86/cpu/vpmu.c
>>>>> +++ b/xen/arch/x86/cpu/vpmu.c
>>>>> @@ -376,57 +376,24 @@ void vpmu_save(struct vcpu *v)
>>>>>      vpmu->last_pcpu = pcpu;
>>>>>      per_cpu(last_vcpu, pcpu) = v;
>>>>>
>>>>> +    vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
>>>>> +
>>>>>      if ( alternative_call(vpmu_ops.arch_vpmu_save, v, 0) )
>>>>>          vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>>>>>
>>>>> +    vpmu_reset(vpmu, VPMU_CONTEXT_SAVE);
>>>>> +
>>>>>      apic_write(APIC_LVTPC, PMU_APIC_VECTOR | APIC_LVT_MASKED);
>>>>>  }
>>>>>
>>>>>  int vpmu_load(struct vcpu *v, bool_t from_guest)
>>>>>  {
>>>>>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>>>>> -    int pcpu = smp_processor_id(), ret;
>>>>> -    struct vcpu *prev = NULL;
>>>>> +    int ret;
>>>>>
>>>>>      if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
>>>>>          return 0;
>>>>>
>>>>> -    /* First time this VCPU is running here */
>>>>> -    if ( vpmu->last_pcpu != pcpu )
>>>>> -    {
>>>>> -        /*
>>>>> -         * Get the context from last pcpu that we ran on. Note that if
>>>> another
>>>>> -         * VCPU is running there it must have saved this VPCU's
>> context
>>>> before
>>>>> -         * startig to run (see below).
>>>>> -         * There should be no race since remote pcpu will disable
>>>> interrupts
>>>>> -         * before saving the context.
>>>>> -         */
>>>>> -        if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
>>>>> -        {
>>>>> -            on_selected_cpus(cpumask_of(vpmu->last_pcpu),
>>>>> -                             vpmu_save_force, (void *)v, 1);
>>>>> -            vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>>>>> -        }
>>>>> -    }
>>>>> -
>>>>> -    /* Prevent forced context save from remote CPU */
>>>>> -    local_irq_disable();
>>>>> -
>>>>> -    prev = per_cpu(last_vcpu, pcpu);
>>>>> -
>>>>> -    if ( prev != v && prev )
>>>>> -    {
>>>>> -        vpmu = vcpu_vpmu(prev);
>>>>> -
>>>>> -        /* Someone ran here before us */
>>>>> -        vpmu_save_force(prev);
>>>>> -        vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>>>>> -
>>>>> -        vpmu = vcpu_vpmu(v);
>>>>> -    }
>>>>> -
>>>>> -    local_irq_enable();
>>>>> -
>>>>>      /* Only when PMU is counting, we load PMU context immediately. */
>>>>>      if ( !vpmu_is_set(vpmu, VPMU_RUNNING) ||
>>>>>           (!has_vlapic(vpmu_vcpu(vpmu)->domain) &&
>>>>
>>>> What about the other two uses of vpmu_save_force() in this file? I looks
>>>> to me as if only the use in mem_sharing.c needs to be retained.
>>>
>>> I don't know, maybe. I rather focus this patch only on the issue and its
>>> fix as I don't want to introduce unintended side effects by doing a
>>> cleanup/consolidation at other code-paths when not strictly necessary.
>>
>> While I see your point, I'm afraid I don't think I can ack this
>> change without knowing whether the other uses don't expose a similar
>> issue. It would feel wrong to fix only one half of a problem. I may,
>> somewhat hesitantly, give an ack if e.g. Boris offered his R-b.
>> Else the only other option I see is that some other maintainer give
>> their ack.
>>
> 
> I may have misunderstood what you are asking. I thought you were asking if
> the other two remaining users of vpmu_save_force could be switched over to
> vpmu_save as a generic cleanup, to which my answer is still maybe. From the
> perspective of this particular bug those use-cases are safe. On is acting
> on the current vcpu and doesn't try to run vpmu_save_force on a remote
> vcpu, the other one is being called when the domain is being shut down so
> the vcpu cannot be in a runnable state.

Hmm, yes - I can accept that. Thanks for the clarification.

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan