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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.09.2020 17:47, Andrew Cooper wrote:
> Various paths in vcpu_create() end up calling paging_update_paging_modes(),
> which eventually allocate a monitor pagetable if one doesn't exist.
> 
> However, an error in vcpu_create() results in the vcpu being cleaned up
> locally, and not put onto the domain's vcpu list.  Therefore, the monitor
> table is not freed by {hap,shadow}_teardown()'s loop.  This is caught by
> assertions later that we've successfully freed the entire hap/shadow memory
> pool.
> 
> The per-vcpu loops in domain teardown logic is conceptually wrong, but exist
> due to insufficient existing structure in the existing logic.
> 
> Break paging_vcpu_teardown() out of paging_teardown(), with mirrored breakouts
> in the hap/shadow code, and use it from arch_vcpu_create()'s error path.  This
> fixes the memory leak.
> 
> The new {hap,shadow}_vcpu_teardown() must be idempotent, and are written to be
> as tolerable as possible, with the minimum number of safety checks possible.
> In particular, drop the mfn_valid() check - if junk is in these fields, then
> Xen is going to explode anyway.
> 
> Reported-by: Michał Leszczyński <michal.leszczynski@xxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
yet I've got a couple of simple questions:

> --- 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()?

> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -2775,6 +2775,32 @@ int shadow_enable(struct domain *d, u32 mode)
>      return rv;
>  }
>  
> +void shadow_vcpu_teardown(struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +
> +    paging_lock(d);
> +
> +    if ( !paging_mode_shadow(d) || !v->arch.paging.mode )

Same question regarding paging_get_hostmode() here, albeit I see
the original code open-coded it in this case.

Jan