[PATCH 0/2] backends/iommufd: Remove mutex

[PATCH 0/2] backends/iommufd: Remove mutex

[Cédric Le Goater]

Hello !

Coverity has some reports regarding the IOMMUFDBackend mutex. Since
the IOMMUFDBackend routines are called from the QEMU main thread, this
series simply suggests removing the mutex and rely on the BQL to
handle concurrent access.

Thanks,

C.

Cédric Le Goater (2):
  backends/iommufd: Remove check on number of backend users
  backends/iommufd: Remove mutex

 include/sysemu/iommufd.h |  2 --
 backends/iommufd.c       | 12 ------------
 2 files changed, 14 deletions(-)

-- 
2.43.0

[PATCH 1/2] backends/iommufd: Remove check on number of backend users

[Cédric Le Goater]

QOM already has a ref count on objects and it will assert much
earlier, when INT_MAX is reached.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 backends/iommufd.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/backends/iommufd.c b/backends/iommufd.c
index ba58a0eb0d0ba9aae625c987eb728547543dba66..393c0d9a3719e3de1a6b51a8ff2e75e184badc82 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -80,11 +80,6 @@ int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
     int fd, ret = 0;
 
     qemu_mutex_lock(&be->lock);
-    if (be->users == UINT32_MAX) {
-        error_setg(errp, "too many connections");
-        ret = -E2BIG;
-        goto out;
-    }
     if (be->owned && !be->users) {
         fd = qemu_open_old("/dev/iommu", O_RDWR);
         if (fd < 0) {
-- 
2.43.0

RE: [PATCH 1/2] backends/iommufd: Remove check on number of backend users

[Duan, Zhenzhong]

Hi Cédric,

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Tuesday, January 2, 2024 8:32 PM
>To: qemu-devel@nongnu.org
>Cc: Liu, Yi L <yi.l.liu@intel.com>; Eric Auger <eric.auger@redhat.com>; Duan,
>Zhenzhong <zhenzhong.duan@intel.com>; Cédric Le Goater
><clg@redhat.com>
>Subject: [PATCH 1/2] backends/iommufd: Remove check on number of
>backend users
>
>QOM already has a ref count on objects and it will assert much
>earlier, when INT_MAX is reached.
>
>Signed-off-by: Cédric Le Goater <clg@redhat.com>

IIUC, /dev/iommu is opened on demand, be->users is used to catch underflow
or overflow due to mismatched iommufd_backend_connect/disconnect
pairs. It looks different from object ref count in purpose, but I agree
a correctly written code will never trigger such overflow/underflow.

So, for the series:
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

BRs.
Zhenzhong

>---
> backends/iommufd.c | 5 -----
> 1 file changed, 5 deletions(-)
>
>diff --git a/backends/iommufd.c b/backends/iommufd.c
>index
>ba58a0eb0d0ba9aae625c987eb728547543dba66..393c0d9a3719e3de1a6b
>51a8ff2e75e184badc82 100644
>--- a/backends/iommufd.c
>+++ b/backends/iommufd.c
>@@ -80,11 +80,6 @@ int iommufd_backend_connect(IOMMUFDBackend
>*be, Error **errp)
>     int fd, ret = 0;
>
>     qemu_mutex_lock(&be->lock);
>-    if (be->users == UINT32_MAX) {
>-        error_setg(errp, "too many connections");
>-        ret = -E2BIG;
>-        goto out;
>-    }
>     if (be->owned && !be->users) {
>         fd = qemu_open_old("/dev/iommu", O_RDWR);
>         if (fd < 0) {
>--
>2.43.0

Re: [PATCH 1/2] backends/iommufd: Remove check on number of backend users

[Cédric Le Goater]

Hello Zhenzhong,

On 1/3/24 02:40, Duan, Zhenzhong wrote:
> Hi Cédric,
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Tuesday, January 2, 2024 8:32 PM
>> To: qemu-devel@nongnu.org
>> Cc: Liu, Yi L <yi.l.liu@intel.com>; Eric Auger <eric.auger@redhat.com>; Duan,
>> Zhenzhong <zhenzhong.duan@intel.com>; Cédric Le Goater
>> <clg@redhat.com>
>> Subject: [PATCH 1/2] backends/iommufd: Remove check on number of
>> backend users
>>
>> QOM already has a ref count on objects and it will assert much
>> earlier, when INT_MAX is reached.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> 
> IIUC, /dev/iommu is opened on demand, be->users is used to catch underflow
> or overflow due to mismatched iommufd_backend_connect/disconnect
> pairs. 
>
> It looks different from object ref count in purpose, but I agree
> a correctly written code will never trigger such overflow/underflow.

Well, we could limit the number of devices sharing the same iommufd
backend but UINT32_MAX seems really too large and the object refcount
will fail earlier anyhow. The max open files per process limit will
also be reached before, since vfio opens a /dev/vfio/devices/vfiox
file per device.

So, this check didn't seem necessary after all.
  
> So, for the series:
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

You should reply to the cover letter with your R-b tag so that
it applies on the whole series.

Thanks,

C.




> BRs.
> Zhenzhong
> 
>> ---
>> backends/iommufd.c | 5 -----
>> 1 file changed, 5 deletions(-)
>>
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index
>> ba58a0eb0d0ba9aae625c987eb728547543dba66..393c0d9a3719e3de1a6b
>> 51a8ff2e75e184badc82 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -80,11 +80,6 @@ int iommufd_backend_connect(IOMMUFDBackend
>> *be, Error **errp)
>>      int fd, ret = 0;
>>
>>      qemu_mutex_lock(&be->lock);
>> -    if (be->users == UINT32_MAX) {
>> -        error_setg(errp, "too many connections");
>> -        ret = -E2BIG;
>> -        goto out;
>> -    }
>>      if (be->owned && !be->users) {
>>          fd = qemu_open_old("/dev/iommu", O_RDWR);
>>          if (fd < 0) {
>> --
>> 2.43.0
> 

RE: [PATCH 1/2] backends/iommufd: Remove check on number of backend users

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH 1/2] backends/iommufd: Remove check on number of
>backend users
>
>Hello Zhenzhong,
>
>On 1/3/24 02:40, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Sent: Tuesday, January 2, 2024 8:32 PM
>>> To: qemu-devel@nongnu.org
>>> Cc: Liu, Yi L <yi.l.liu@intel.com>; Eric Auger <eric.auger@redhat.com>;
>Duan,
>>> Zhenzhong <zhenzhong.duan@intel.com>; Cédric Le Goater
>>> <clg@redhat.com>
>>> Subject: [PATCH 1/2] backends/iommufd: Remove check on number of
>>> backend users
>>>
>>> QOM already has a ref count on objects and it will assert much
>>> earlier, when INT_MAX is reached.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>
>> IIUC, /dev/iommu is opened on demand, be->users is used to catch
>underflow
>> or overflow due to mismatched iommufd_backend_connect/disconnect
>> pairs.
>>
>> It looks different from object ref count in purpose, but I agree
>> a correctly written code will never trigger such overflow/underflow.
>
>Well, we could limit the number of devices sharing the same iommufd
>backend but UINT32_MAX seems really too large and the object refcount
>will fail earlier anyhow. The max open files per process limit will
>also be reached before, since vfio opens a /dev/vfio/devices/vfiox
>file per device.

Clear, thanks Cédric.

>
>So, this check didn't seem necessary after all.
>
>> So, for the series:
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
>You should reply to the cover letter with your R-b tag so that
>it applies on the whole series.

Sure.

BRs.
Zhenzhong

[PATCH 2/2] backends/iommufd: Remove mutex

[Cédric Le Goater]

Coverity reports a concurrent data access violation because be->users
is being accessed in iommufd_backend_can_be_deleted() without holding
the mutex.

However, these routines are called from the QEMU main thread when a
device is created. In this case, the code paths should be protected by
the BQL lock and it should be safe to drop the IOMMUFD backend mutex.
Simply remove it.

Fixes: CID 1531550
Fixes: CID 1531549
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 include/sysemu/iommufd.h | 2 --
 backends/iommufd.c       | 7 -------
 2 files changed, 9 deletions(-)

diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
index 9c5524b0ed15ef5f81be159415bc216572a283d8..9af27ebd6ccb78ca8e16aa3c62629aab9f7f31e4 100644
--- a/include/sysemu/iommufd.h
+++ b/include/sysemu/iommufd.h
@@ -2,7 +2,6 @@
 #define SYSEMU_IOMMUFD_H
 
 #include "qom/object.h"
-#include "qemu/thread.h"
 #include "exec/hwaddr.h"
 #include "exec/cpu-common.h"
 
@@ -19,7 +18,6 @@ struct IOMMUFDBackend {
     /*< protected >*/
     int fd;            /* /dev/iommu file descriptor */
     bool owned;        /* is the /dev/iommu opened internally */
-    QemuMutex lock;
     uint32_t users;
 
     /*< public >*/
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 393c0d9a3719e3de1a6b51a8ff2e75e184badc82..1ef683c7b080e688af46c5b98e61eafa73e39895 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -29,7 +29,6 @@ static void iommufd_backend_init(Object *obj)
     be->fd = -1;
     be->users = 0;
     be->owned = true;
-    qemu_mutex_init(&be->lock);
 }
 
 static void iommufd_backend_finalize(Object *obj)
@@ -52,10 +51,8 @@ static void iommufd_backend_set_fd(Object *obj, const char *str, Error **errp)
         error_prepend(errp, "Could not parse remote object fd %s:", str);
         return;
     }
-    qemu_mutex_lock(&be->lock);
     be->fd = fd;
     be->owned = false;
-    qemu_mutex_unlock(&be->lock);
     trace_iommu_backend_set_fd(be->fd);
 }
 
@@ -79,7 +76,6 @@ int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
 {
     int fd, ret = 0;
 
-    qemu_mutex_lock(&be->lock);
     if (be->owned && !be->users) {
         fd = qemu_open_old("/dev/iommu", O_RDWR);
         if (fd < 0) {
@@ -93,13 +89,11 @@ int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
 out:
     trace_iommufd_backend_connect(be->fd, be->owned,
                                   be->users, ret);
-    qemu_mutex_unlock(&be->lock);
     return ret;
 }
 
 void iommufd_backend_disconnect(IOMMUFDBackend *be)
 {
-    qemu_mutex_lock(&be->lock);
     if (!be->users) {
         goto out;
     }
@@ -110,7 +104,6 @@ void iommufd_backend_disconnect(IOMMUFDBackend *be)
     }
 out:
     trace_iommufd_backend_disconnect(be->fd, be->users);
-    qemu_mutex_unlock(&be->lock);
 }
 
 int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
-- 
2.43.0

Re: [PATCH 0/2] backends/iommufd: Remove mutex

[Eric Auger]

Hi Cédric,

On 1/2/24 13:32, Cédric Le Goater wrote:
> Hello !
>
> Coverity has some reports regarding the IOMMUFDBackend mutex. Since
> the IOMMUFDBackend routines are called from the QEMU main thread, this
> series simply suggests removing the mutex and rely on the BQL to
> handle concurrent access.
>
> Thanks,
>
> C.
>
> Cédric Le Goater (2):
>   backends/iommufd: Remove check on number of backend users
>   backends/iommufd: Remove mutex

for the series:
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thank you for the fix!

Eric
>
>  include/sysemu/iommufd.h |  2 --
>  backends/iommufd.c       | 12 ------------
>  2 files changed, 14 deletions(-)
>

RE: [PATCH 0/2] backends/iommufd: Remove mutex

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Tuesday, January 2, 2024 8:32 PM
>To: qemu-devel@nongnu.org
>Cc: Liu, Yi L <yi.l.liu@intel.com>; Eric Auger <eric.auger@redhat.com>; Duan,
>Zhenzhong <zhenzhong.duan@intel.com>; Cédric Le Goater
><clg@redhat.com>
>Subject: [PATCH 0/2] backends/iommufd: Remove mutex
>
>Hello !
>
>Coverity has some reports regarding the IOMMUFDBackend mutex. Since
>the IOMMUFDBackend routines are called from the QEMU main thread, this
>series simply suggests removing the mutex and rely on the BQL to
>handle concurrent access.

For the whole series,
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>
>Thanks,
>
>C.
>
>Cédric Le Goater (2):
>  backends/iommufd: Remove check on number of backend users
>  backends/iommufd: Remove mutex
>
> include/sysemu/iommufd.h |  2 --
> backends/iommufd.c       | 12 ------------
> 2 files changed, 14 deletions(-)
>
>--
>2.43.0

Re: [PATCH 0/2] backends/iommufd: Remove mutex

[Cédric Le Goater]

On 1/2/24 13:32, Cédric Le Goater wrote:
> Hello !
> 
> Coverity has some reports regarding the IOMMUFDBackend mutex. Since
> the IOMMUFDBackend routines are called from the QEMU main thread, this
> series simply suggests removing the mutex and rely on the BQL to
> handle concurrent access.
> 
> Thanks,
> 
> C.
> 
> Cédric Le Goater (2):
>    backends/iommufd: Remove check on number of backend users
>    backends/iommufd: Remove mutex
> 
>   include/sysemu/iommufd.h |  2 --
>   backends/iommufd.c       | 12 ------------
>   2 files changed, 14 deletions(-)
> 


Applied to vfio-next.

Thanks,

C.