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 <[email protected]>
>> 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 <[email protected]>
>> ---
>> 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
