Re: [PATCH v4 3/3] intel_iommu: Take the VTD lock when looking for and creating address spaces

[CLEMENT MATHIEU--DRIF]

Hi Zhenzhong,

On 28/04/2025 10:55 am, Duan, Zhenzhong wrote:
> Caution: External email. Do not open attachments or click links, unless this 
> email comes from a known sender and you know the content is safe.
> 
> 
> Hi Clement,
> 
>> -----Original Message-----
>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--d...@eviden.com>
>> Subject: [PATCH v4 3/3] intel_iommu: Take the VTD lock when looking for and
>> creating address spaces
>>
>> vtd_find_add_as can be called by multiple threads which leads to a race
>> condition on address space creation. The IOMMU lock must be taken to
>> avoid such a race.
>>
>> Signed-off-by: Clement Mathieu--Drif <clement.mathieu--d...@eviden.com>
>> ---
>> hw/i386/intel_iommu.c | 28 ++++++++++++++++++++++++++--
>> 1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index b7855f4b87..931ac01ef0 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -4203,11 +4203,15 @@ VTDAddressSpace
>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>          .pasid = pasid,
>>      };
>>      VTDAddressSpace *vtd_dev_as;
>> +    struct vtd_as_key *new_key = NULL;
>>      char name[128];
>>
>> +    vtd_iommu_lock(s);
>>      vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
>> +    vtd_iommu_unlock(s);
>> +
>>      if (!vtd_dev_as) {
>> -        struct vtd_as_key *new_key = g_malloc(sizeof(*new_key));
>> +        new_key = g_malloc(sizeof(*new_key));
>>
>>          new_key->bus = bus;
>>          new_key->devfn = devfn;
>> @@ -4302,9 +4306,29 @@ VTDAddressSpace
>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>                                              &vtd_dev_as->nodmar, 0);
>>
>>          vtd_switch_address_space(vtd_dev_as);
>> +    }
>>
>> -        g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
>> +    if (new_key != NULL) {
>> +        VTDAddressSpace *second_vtd_dev_as;
>> +
>> +        /*
>> +         * Take the lock again and recheck as the AS might have
>> +         * been created in the meantime.
>> +         */
>> +        vtd_iommu_lock(s);
>> +
>> +        second_vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, 
>> &key);
>> +        if (!second_vtd_dev_as) {
>> +            g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
>> +        } else {
>> +            vtd_dev_as = second_vtd_dev_as;
>> +            g_free(vtd_dev_as);
>> +            g_free(new_key);
> 
> We need to release memory regions under this vtd_dev_as to avoid leak.


Indeed, I'll have to take the BQL again.

Is it ok for you if it look like this:

vtd_iommu_lock(s);

second_vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
if (!second_vtd_dev_as) {
     g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
     vtd_iommu_unlock(s);
} else {
     vtd_iommu_unlock(s);
     BQL_LOCK_GUARD();

     memory_region_del_subregion(&vtd_dev_as->root, &vtd_dev_as->nodmar);
     memory_region_del_subregion(&vtd_dev_as->root,
                                 MEMORY_REGION(&vtd_dev_as->iommu));
     memory_region_del_subregion(&vtd_dev_as->root,
                                 &vtd_dev_as->iommu_ir_fault);
     memory_region_del_subregion(&vtd_dev_as->root,
                                 &vtd_dev_as->iommu_ir);

     memory_region_unref(MEMORY_REGION(&vtd_dev_as->iommu));
     memory_region_unref(&vtd_dev_as->iommu_ir_fault);
     memory_region_unref(&vtd_dev_as->iommu_ir);
     memory_region_unref(&vtd_dev_as->nodmar);

     address_space_destroy(&vtd_dev_as->as);

     g_free(vtd_dev_as);
     g_free(new_key);

     vtd_dev_as = second_vtd_dev_as;
}

...

return vtd_dev_as;


> 
> Thanks
> Zhenzhong