Re: [PATCH 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path

[Julien Grall <julien@xxxxxxx>]

Hi Michal,

On 22/03/2023 10:29, Michal Orzel wrote:
> When vgic_reserve_virq() fails and backend is in domain, we should also
> free the allocated event channel.
>
> When backend is in Xen and call to xzalloc() returns NULL, there is no
> need to call xfree(). This should be done instead on an error path
> from vgic_reserve_virq().
Most likely this was implemented this way to avoid a double "if ( 
vpl011->backend_in_domain)". TBH, I am not very thrilled with this 
approach. Could we instead consider to use domain_pl011_deinit()? (A 
couple of tweak would be necessary to use it)
> Also, take the opportunity to return -ENOMEM
> instead of -EINVAL when memory allocation fails.
>
> Fixes: 1ee1e4b0d1ff ("xen/arm: Allow vpl011 to be used by DomU")
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
>   xen/arch/arm/vpl011.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 541ec962f189..df29a65ad365 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -696,8 +696,8 @@ int domain_vpl011_init(struct domain *d, struct 
> vpl011_init_info *info)
>           vpl011->backend.xen = xzalloc(struct vpl011_xen_backend);
>           if ( vpl011->backend.xen == NULL )
>           {
> -            rc = -EINVAL;
> -            goto out1;
> +            rc = -ENOMEM;
> +            goto out;
>           }
>       }
>   
> @@ -720,12 +720,15 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>   out2:
>       vgic_free_virq(d, vpl011->virq);
>   
> +    if ( vpl011->backend_in_domain )
> +        free_xen_event_channel(d, vpl011->evtchn);
> +    else
> +        xfree(vpl011->backend.xen);
There is another bug here (unrelated to your change). You want to use 
XFREE() to avoid an extra free in domain_pl011_deinit(). Can you look at it?
> +
>   out1:
>       if ( vpl011->backend_in_domain )
>           destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
>                                   vpl011->backend.dom.ring_page);
> -    else
> -        xfree(vpl011->backend.xen);
>   
>   out:
>       return rc;
--
Julien Grall