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

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

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.

Tamas