1. Patchew
  2. QEMU
  3. [PATCH v4 0/3] intel_iommu: Fix locking issues
  4. View patch

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

CLEMENT MATHIEU--DRIF posted 3 patches 6 months, 3 weeks ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, Yi Liu <yi.l.liu@intel.com>, "Clément Mathieu--Drif" <clement.mathieu--drif@eviden.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
There is a newer version of this series
[PATCH v4 3/3] intel_iommu: Take the VTD lock when looking for and creating address spaces
Posted by CLEMENT MATHIEU--DRIF 6 months, 3 weeks ago 
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--drif@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);
+        }
+
+        vtd_iommu_unlock(s);
     }
+
     return vtd_dev_as;
 }
 
-- 
2.49.0
RE: [PATCH v4 3/3] intel_iommu: Take the VTD lock when looking for and creating address spaces
Posted by Duan, Zhenzhong 6 months, 3 weeks ago 
Hi Clement,

>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@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--drif@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.

Thanks
Zhenzhong
Re: [PATCH v4 3/3] intel_iommu: Take the VTD lock when looking for and creating address spaces
Posted by CLEMENT MATHIEU--DRIF 6 months, 3 weeks ago 
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--drif@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--drif@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
RE: [PATCH v4 3/3] intel_iommu: Take the VTD lock when looking for and creating address spaces
Posted by Duan, Zhenzhong 6 months, 2 weeks ago 

>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Subject: Re: [PATCH v4 3/3] intel_iommu: Take the VTD lock when looking for and
>creating address spaces
>
>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--drif@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--drif@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);

s/memory_region_unref/object_unparent?

>
>     address_space_destroy(&vtd_dev_as->as);

object_unparent(vtd_dev_as->root);

Thanks
Zhenzhong
Re: [PATCH v4 3/3] intel_iommu: Take the VTD lock when looking for and creating address spaces
Posted by CLEMENT MATHIEU--DRIF 6 months, 2 weeks ago 

On 29/04/2025 8:01 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.
>
>
>> -----Original Message-----
>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>> Subject: Re: [PATCH v4 3/3] intel_iommu: Take the VTD lock when looking for and
>> creating address spaces
>>
>> 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--drif@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--drif@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);
> s/memory_region_unref/object_unparent?
>
>>      address_space_destroy(&vtd_dev_as->as);
> object_unparent(vtd_dev_as->root);

Hi Zhenzhong,

I think the issue can be fixed differently in a cleaner way.
I sent a v5 for that.

Thanks

>
> Thanks
> Zhenzhong

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.