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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.12.2020 14:58, Andrew Cooper wrote:
> On 18/12/2020 08:27, Jan Beulich wrote:
>> 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.
> 
> Right, but we also might end up here with an error early in
> vcpu_create(), where d->arch.paging is set up, but v->arch.paging isn't.
> 
> This logic needs to be safe to use at any point of partial initialisation.
> 
> (And to be clear, I found I needed both of these based on some
> artificial error injection testing.)
> 
>>> 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.
> 
> I'm not convinced it is an appropriate abstraction to have, and I don't
> expect it to survive the nested virt work.
> 
>> 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 ...
> 
> Doesn't matter.  Its use here would obfuscate the code (this is one part
> of why I think it is a bad abstraction to begin with), and if the
> implementation ever changed, the function would lose its safety.
> 
>> 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?
> 
> I'm not entirely convinced that was necessarily safe.  Irrespective, see
> the TODO.  The foreach_vcpu() is only a stopgap until some cleanup
> structure changes come along (which I had queued behind this patch anyway).

Well, fair enough (for all of the points). You have my R-b already,
and all you need to do (if you haven't already) is re-base the
change, as the conflicting one of mine (which was triggered by
reviewing yours) has gone in already.

Jan