[Fix] Qemu/Xen: Fix early freeing MSIX MMIO memory region

On 05/10/2015 18:53, Stefano Stabellini wrote:
>> This patch is to fix the issue via moving MSIX MMIO memory region into
>> struct XenPCIPassthroughState and free it together with pt device's obj.
> 
> Given that all the MSI-X related info are in XenPTMSIX, I would prefer
> to keep the mmio memory region there, if possible.
> 
> Couldn't you just unhook msix->mmio from XenPCIPassthroughState's object
> in xen_pt_msix_delete?  Calling object_property_del_child or
> object_unparent?

This is the right thing to do, but there are two separate things to fix.

One is the use-after-free of msix->mmio, the other is that freeing 
s->msix and in general xen_pt_config_delete should be done from the 
.instance_finalize callback.  This is documented in docs/memory.txt.

This is an attempt at a patch (not even compiled):


where xen_pt_config_unmap would be called from xen_pt_destroy, and the call
to xen_pt_config_delete would be moved to xen_pci_passthrough_info's
instance_finalize member.

Paolo

On Tue, 6 Oct 2015, Paolo Bonzini wrote:
> On 05/10/2015 18:53, Stefano Stabellini wrote:
> >> This patch is to fix the issue via moving MSIX MMIO memory region into
> >> struct XenPCIPassthroughState and free it together with pt device's obj.
> > 
> > Given that all the MSI-X related info are in XenPTMSIX, I would prefer
> > to keep the mmio memory region there, if possible.
> > 
> > Couldn't you just unhook msix->mmio from XenPCIPassthroughState's object
> > in xen_pt_msix_delete?  Calling object_property_del_child or
> > object_unparent?
> 
> This is the right thing to do, but there are two separate things to fix.
> 
> One is the use-after-free of msix->mmio, the other is that freeing 
> s->msix and in general xen_pt_config_delete should be done from the 
> .instance_finalize callback.  This is documented in docs/memory.txt.
> 
> This is an attempt at a patch (not even compiled):
> 
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index e3d7194..e476bac 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -610,7 +610,7 @@ error_out:
>      return rc;
>  }
>  
> -void xen_pt_msix_delete(XenPCIPassthroughState *s)
> +void xen_pt_msix_unmap(XenPCIPassthroughState *s)
>  {
>      XenPTMSIX *msix = s->msix;
>  
> @@ -627,6 +627,17 @@ void xen_pt_msix_delete(XenPCIPassthroughState *s)
>      }
>  
>      memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio);
> +}
> +
> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
> +{
> +    XenPTMSIX *msix = s->msix;
> +
> +    if (!msix) {
> +        return;
> +    }
> +
> +    object_unparent(&msix->mmio);
>  
>      g_free(s->msix);
>      s->msix = NULL;
> 
> where xen_pt_config_unmap would be called from xen_pt_destroy, and the call
> to xen_pt_config_delete would be moved to xen_pci_passthrough_info's
> instance_finalize member.

Thanks for the explanation and the code. It makes sense to me.

Lan, could you please write up a patch based on this approach and test
it?

On 10/6/2015 9:49 PM, Stefano Stabellini wrote:
> On Tue, 6 Oct 2015, Paolo Bonzini wrote:
>> On 05/10/2015 18:53, Stefano Stabellini wrote:
>>>> This patch is to fix the issue via moving MSIX MMIO memory region into
>>>> struct XenPCIPassthroughState and free it together with pt device's obj.
>>>
>>> Given that all the MSI-X related info are in XenPTMSIX, I would prefer
>>> to keep the mmio memory region there, if possible.
>>>
>>> Couldn't you just unhook msix->mmio from XenPCIPassthroughState's object
>>> in xen_pt_msix_delete?  Calling object_property_del_child or
>>> object_unparent?
>>
>> This is the right thing to do, but there are two separate things to fix.
>>
>> One is the use-after-free of msix->mmio, the other is that freeing
>> s->msix and in general xen_pt_config_delete should be done from the
>> .instance_finalize callback.  This is documented in docs/memory.txt.
>>
>> This is an attempt at a patch (not even compiled):
>>
>> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
>> index e3d7194..e476bac 100644
>> --- a/hw/xen/xen_pt_msi.c
>> +++ b/hw/xen/xen_pt_msi.c
>> @@ -610,7 +610,7 @@ error_out:
>>       return rc;
>>   }
>>
>> -void xen_pt_msix_delete(XenPCIPassthroughState *s)
>> +void xen_pt_msix_unmap(XenPCIPassthroughState *s)
>>   {
>>       XenPTMSIX *msix = s->msix;
>>
>> @@ -627,6 +627,17 @@ void xen_pt_msix_delete(XenPCIPassthroughState *s)
>>       }
>>
>>       memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio);
>> +}
>> +
>> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
>> +{
>> +    XenPTMSIX *msix = s->msix;
>> +
>> +    if (!msix) {
>> +        return;
>> +    }
>> +
>> +    object_unparent(&msix->mmio);
>>
>>       g_free(s->msix);
>>       s->msix = NULL;
>>
>> where xen_pt_config_unmap would be called from xen_pt_destroy, and the call
>> to xen_pt_config_delete would be moved to xen_pci_passthrough_info's
>> instance_finalize member.
>
> Thanks for the explanation and the code. It makes sense to me.
>
> Lan, could you please write up a patch based on this approach and test
> it?
>

Sure. I will update patch following the guide. Thanks Paolo & Stefano.