Re: [PATCH] xen/x86: Fix memory leak in vcpu_create() error path

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.12.2020 22:46, Andrew Cooper wrote:
> On 29/09/2020 07:18, Jan Beulich wrote:
>> On 28.09.2020 17:47, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/mm/hap/hap.c
>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>> @@ -563,30 +563,37 @@ void hap_final_teardown(struct domain *d)
>>>      paging_unlock(d);
>>>  }
>>>  
>>> +void hap_vcpu_teardown(struct vcpu *v)
>>> +{
>>> +    struct domain *d = v->domain;
>>> +    mfn_t mfn;
>>> +
>>> +    paging_lock(d);
>>> +
>>> +    if ( !paging_mode_hap(d) || !v->arch.paging.mode )
>>> +        goto out;
>> Any particular reason you don't use paging_get_hostmode() (as the
>> original code did) here? Any particular reason for the seemingly
>> redundant (and hence somewhat in conflict with the description's
>> "with the minimum number of safety checks possible")
>> paging_mode_hap()?
> 
> Yes to both.  As you spotted, I converted the shadow side first, and
> made the two consistent.
> 
> The paging_mode_{shadow,hap})() is necessary for idempotency.  These
> functions really might get called before paging is set up, for an early
> failure in domain_create().

In which case how would v->arch.paging.mode be non-NULL already?
They get set in {hap,shadow}_vcpu_init() only.

> The paging mode has nothing really to do with hostmode/guestmode/etc. 
> It is the only way of expressing the logic where it is clear that the
> lower pointer dereferences are trivially safe.

Well, yes and no - the other uses of course should then also use
paging_get_hostmode(), like various of the wrappers in paging.h
do. Or else I question why we have paging_get_hostmode() in the
first place. There are more examples in shadow code where this
gets open-coded when it probably shouldn't be. There haven't been
any such cases in HAP code so far ...

Additionally (noticing only now) in the shadow case you may now
loop over all vCPU-s in shadow_teardown() just for
shadow_vcpu_teardown() to bail right away. Wouldn't it make sense
to retain the "if ( shadow_mode_enabled(d) )" there around the
loop?

Jan