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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 22/03/2023 17:19, Julien Grall wrote:
> 
> 
> 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)
I think we could. More about it later.

> 
>> 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?
Strictly speaking this is not a bug. Memory allocation can only happen if 
backend is in Xen.
This means, that if vpl011 init fails, we will call free only once 
(domain_vpl011_deinit
will not be called on this path i.e. we will invoke panic after construct_domU).
Of course, we could switch to XFREE just for sanity.

> 
>> +
>>   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;
> 
Solution to reuse domain_pl011_deinit would be as follows:

     vgic_free_virq(d, vpl011->virq);
 
 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);
+    domain_vpl011_deinit(d);
 
 out:
     return rc;
@@ -737,12 +733,15 @@ void domain_vpl011_deinit(struct domain *d)
 
     if ( vpl011->backend_in_domain )
     {
-        if ( !vpl011->backend.dom.ring_buf )
-            return;
+        if ( vpl011->backend.dom.ring_buf )
+            destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
+                                    vpl011->backend.dom.ring_page);
 
-        free_xen_event_channel(d, vpl011->evtchn);
-        destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
-                                vpl011->backend.dom.ring_page);
+        if ( vpl011->evtchn )
+        {
+            free_xen_event_channel(d, vpl011->evtchn);
+            vpl011->evtchn = 0;
+        }
     }
     else
         xfree(vpl011->backend.xen);

However there is one problem with guarding free_xen_event_channel to be called 
only once.
Even without allocating event channel, vpl011->evtchn is 0 by default. So doing:
if ( is_port_valid(vpl011->evtchn) )
    free_xen_event_channel(d, vpl011->evtchn);
would not help us as evtchn 0 is always there. So in my proposal I'm assuming 
(I think correctly)
that vpl011->evtchn cannot be 0 after successful evtchn allocation because 0 is 
"reserved".
But I might be wrong in which case I'm clueless how to do the proper check.

~Michal