[v5] intel_iommu: Fix locking issues

[PATCH v5 2/2] intel_iommu: Take locks when looking for and creating address spaces

Posted by CLEMENT MATHIEU--DRIF 6 months, 2 weeks ago

vtd_find_add_as can be called by multiple threads which leads to a race
condition. Taking the IOMMU lock ensures we avoid such a race.
Moreover we also need to take the bql to avoid an assert to fail in
memory_region_add_subregion_overlap when actually allocating a new
address space.

Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com>
---
 hw/i386/intel_iommu.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index dad1d9f300..144e25622a 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -4205,9 +4205,30 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
     VTDAddressSpace *vtd_dev_as;
     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));
+        struct vtd_as_key *new_key;
+        /* Slow path */
+
+        /*
+        * memory_region_add_subregion_overlap requires the bql,
+        * make sure we own it.
+        */
+        BQL_LOCK_GUARD();
+        vtd_iommu_lock(s);
+
+        /* Check again as we released the lock for a moment */
+        vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
+        if (vtd_dev_as) {
+            vtd_iommu_unlock(s);
+            return vtd_dev_as;
+        }
+
+        /* Still nothing, allocate a new address space */
+        new_key = g_malloc(sizeof(*new_key));
 
         new_key->bus = bus;
         new_key->devfn = devfn;
@@ -4298,6 +4319,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
         vtd_switch_address_space(vtd_dev_as);
 
         g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
+    
+        vtd_iommu_unlock(s);
     }
     return vtd_dev_as;
 }
-- 
2.49.0

Re: [PATCH v5 2/2] intel_iommu: Take locks when looking for and creating address spaces

Posted by Michael S. Tsirkin 6 months ago

On Wed, Apr 30, 2025 at 12:48:06PM +0000, CLEMENT MATHIEU--DRIF wrote:
> vtd_find_add_as can be called by multiple threads which leads to a race
> condition. Taking the IOMMU lock ensures we avoid such a race.
> Moreover we also need to take the bql to avoid an assert to fail in
> memory_region_add_subregion_overlap when actually allocating a new
> address space.
> 
> Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com>
> ---
>  hw/i386/intel_iommu.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index dad1d9f300..144e25622a 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -4205,9 +4205,30 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>      VTDAddressSpace *vtd_dev_as;
>      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));
> +        struct vtd_as_key *new_key;
> +        /* Slow path */
> +
> +        /*
> +        * memory_region_add_subregion_overlap requires the bql,
> +        * make sure we own it.
> +        */


not how comments should look

> +        BQL_LOCK_GUARD();
> +        vtd_iommu_lock(s);
> +
> +        /* Check again as we released the lock for a moment */
> +        vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
> +        if (vtd_dev_as) {
> +            vtd_iommu_unlock(s);
> +            return vtd_dev_as;
> +        }
> +
> +        /* Still nothing, allocate a new address space */
> +        new_key = g_malloc(sizeof(*new_key));
>  
>          new_key->bus = bus;
>          new_key->devfn = devfn;
> @@ -4298,6 +4319,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>          vtd_switch_address_space(vtd_dev_as);
>  
>          g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
> +    

trailing whitespace here

> +        vtd_iommu_unlock(s);
>      }
>      return vtd_dev_as;
>  }



I fixed these up but pls take care, Clement.

> -- 
> 2.49.0

Re: [PATCH v5 2/2] intel_iommu: Take locks when looking for and creating address spaces

Posted by CLEMENT MATHIEU--DRIF 6 months ago


On 14/05/2025 1:48 pm, Michael S. Tsirkin 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.
> 
> 
> On Wed, Apr 30, 2025 at 12:48:06PM +0000, CLEMENT MATHIEU--DRIF wrote:
>> vtd_find_add_as can be called by multiple threads which leads to a race
>> condition. Taking the IOMMU lock ensures we avoid such a race.
>> Moreover we also need to take the bql to avoid an assert to fail in
>> memory_region_add_subregion_overlap when actually allocating a new
>> address space.
>>
>> Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com>
>> ---
>>   hw/i386/intel_iommu.c | 25 ++++++++++++++++++++++++-
>>   1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index dad1d9f300..144e25622a 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -4205,9 +4205,30 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>       VTDAddressSpace *vtd_dev_as;
>>       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));
>> +        struct vtd_as_key *new_key;
>> +        /* Slow path */
>> +
>> +        /*
>> +        * memory_region_add_subregion_overlap requires the bql,
>> +        * make sure we own it.
>> +        */
> 
> 
> not how comments should look
> 
>> +        BQL_LOCK_GUARD();
>> +        vtd_iommu_lock(s);
>> +
>> +        /* Check again as we released the lock for a moment */
>> +        vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
>> +        if (vtd_dev_as) {
>> +            vtd_iommu_unlock(s);
>> +            return vtd_dev_as;
>> +        }
>> +
>> +        /* Still nothing, allocate a new address space */
>> +        new_key = g_malloc(sizeof(*new_key));
>>
>>           new_key->bus = bus;
>>           new_key->devfn = devfn;
>> @@ -4298,6 +4319,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>           vtd_switch_address_space(vtd_dev_as);
>>
>>           g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
>> +
> 
> trailing whitespace here
> 
>> +        vtd_iommu_unlock(s);
>>       }
>>       return vtd_dev_as;
>>   }
> 
> 
> 
> I fixed these up but pls take care, Clement.

Ouch, sorry, thanks Michael

> 
>> --
>> 2.49.0
>