[PATCH v2 0/4] vfio-user fixes

[PATCH v2 0/4] vfio-user fixes

[John Levon]

Some small Coverity and related fixes for the recently merged vfio-user series.

thanks
john

John Levon (4):
  hw/vfio-user: add Cédric Le Goater as a maintainer
  hw/vfio: fix region fd initialization
  hw/vfio-user: wait for proxy close correctly
  hw/vfio-user: fix use of uninitialized variable

 MAINTAINERS              |  1 +
 hw/vfio-user/container.c |  6 +-----
 hw/vfio-user/proxy.c     | 10 ++++++----
 hw/vfio/device.c         |  6 +++++-
 4 files changed, 13 insertions(+), 10 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] hw/vfio-user: add Cédric Le Goater as a maintainer

[John Levon]

Signed-off-by: John Levon <john.levon@nutanix.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e88ed2c0a9..30e9b71e6e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4287,6 +4287,7 @@ F: tests/functional/test_multiprocess.py
 VFIO-USER:
 M: John Levon <john.levon@nutanix.com>
 M: Thanos Makatos <thanos.makatos@nutanix.com>
+M: Cédric Le Goater <clg@redhat.com>
 S: Supported
 F: docs/interop/vfio-user.rst
 F: docs/system/devices/vfio-user.rst
-- 
2.43.0

[PATCH v2 2/4] hw/vfio: fix region fd initialization

[John Levon]

We were not initializing the region fd array to -1, so we would
accidentally try to close(0) on cleanup for any region that is not
referenced.

Fixes: 95cdb024 ("vfio: add region info cache")
Signed-off-by: John Levon <john.levon@nutanix.com>
---
 hw/vfio/device.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 96cf21462c..52a1996dc4 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -463,6 +463,8 @@ void vfio_device_detach(VFIODevice *vbasedev)
 void vfio_device_prepare(VFIODevice *vbasedev, VFIOContainerBase *bcontainer,
                          struct vfio_device_info *info)
 {
+    int i;
+
     vbasedev->num_irqs = info->num_irqs;
     vbasedev->num_regions = info->num_regions;
     vbasedev->flags = info->flags;
@@ -477,6 +479,9 @@ void vfio_device_prepare(VFIODevice *vbasedev, VFIOContainerBase *bcontainer,
                                vbasedev->num_regions);
     if (vbasedev->use_region_fds) {
         vbasedev->region_fds = g_new0(int, vbasedev->num_regions);
+        for (i = 0; i < vbasedev->num_regions; i++) {
+            vbasedev->region_fds[i] = -1;
+        }
     }
 }
 
@@ -489,7 +494,6 @@ void vfio_device_unprepare(VFIODevice *vbasedev)
         if (vbasedev->region_fds != NULL && vbasedev->region_fds[i] != -1) {
             close(vbasedev->region_fds[i]);
         }
-
     }
 
     g_clear_pointer(&vbasedev->reginfo, g_free);
-- 
2.43.0

[PATCH v2 3/4] hw/vfio-user: wait for proxy close correctly

[John Levon]

Coverity reported:

CID 1611806: Concurrent data access violations (BAD_CHECK_OF_WAIT_COND)

A wait is performed without a loop. If there is a spurious wakeup, the
condition may not be satisfied.

Fix this by checking ->state for VFIO_PROXY_CLOSED in a loop.

Resolves: Coverity CID 1611806
Fixes: 0b3d881a ("vfio-user: implement message receive infrastructure")
Signed-off-by: John Levon <john.levon@nutanix.com>
---
 hw/vfio-user/proxy.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
index c418954440..2275d3fe39 100644
--- a/hw/vfio-user/proxy.c
+++ b/hw/vfio-user/proxy.c
@@ -32,7 +32,6 @@ static void vfio_user_recycle(VFIOUserProxy *proxy, VFIOUserMsg *msg);
 
 static void vfio_user_recv(void *opaque);
 static void vfio_user_send(void *opaque);
-static void vfio_user_cb(void *opaque);
 
 static void vfio_user_request(void *opaque);
 
@@ -492,7 +491,7 @@ static void vfio_user_send(void *opaque)
     }
 }
 
-static void vfio_user_cb(void *opaque)
+static void vfio_user_close_cb(void *opaque)
 {
     VFIOUserProxy *proxy = opaque;
 
@@ -984,8 +983,11 @@ void vfio_user_disconnect(VFIOUserProxy *proxy)
      * handler to run after the proxy fd handlers were
      * deleted above.
      */
-    aio_bh_schedule_oneshot(proxy->ctx, vfio_user_cb, proxy);
-    qemu_cond_wait(&proxy->close_cv, &proxy->lock);
+    aio_bh_schedule_oneshot(proxy->ctx, vfio_user_close_cb, proxy);
+
+    while (proxy->state != VFIO_PROXY_CLOSED) {
+        qemu_cond_wait(&proxy->close_cv, &proxy->lock);
+    }
 
     /* we now hold the only ref to proxy */
     qemu_mutex_unlock(&proxy->lock);
-- 
2.43.0

[PATCH v2 4/4] hw/vfio-user: fix use of uninitialized variable

[John Levon]

Coverity reported:

CID 1611805:         Uninitialized variables

in vfio_user_dma_map(). This can occur in the happy path when
->async_ops was not set; as this doesn't typically happen, it wasn't
caught during testing.

Align both map and unmap implementations to initialize ret the same way
to resolve this.

Resolves: Coverity CID 1611805
Fixes: 18e899e6 ("vfio-user: implement VFIO_USER_DMA_MAP/UNMAP")
Reported-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: John Levon <john.levon@nutanix.com>
---
 hw/vfio-user/container.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/vfio-user/container.c b/hw/vfio-user/container.c
index d318e6a339..d589dd90f5 100644
--- a/hw/vfio-user/container.c
+++ b/hw/vfio-user/container.c
@@ -64,8 +64,6 @@ static int vfio_user_dma_unmap(const VFIOContainerBase *bcontainer,
                               0, &local_err)) {
             error_report_err(local_err);
             ret = -EFAULT;
-        } else {
-            ret = 0;
         }
     } else {
         if (!vfio_user_send_wait(container->proxy, &msgp->hdr, NULL,
@@ -92,7 +90,7 @@ static int vfio_user_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
                                                 bcontainer);
     int fd = memory_region_get_fd(mrp);
     Error *local_err = NULL;
-    int ret;
+    int ret = 0;
 
     VFIOUserFDs *fds = NULL;
     VFIOUserDMAMap *msgp = g_malloc0(sizeof(*msgp));
@@ -135,8 +133,6 @@ static int vfio_user_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
                               0, &local_err)) {
             error_report_err(local_err);
             ret = -EFAULT;
-        } else {
-            ret = 0;
         }
     } else {
         VFIOUserFDs local_fds = { 1, 0, &fd };
-- 
2.43.0

Re: [PATCH v2 0/4] vfio-user fixes

[Cédric Le Goater]

On 7/15/25 07:52, John Levon wrote:
> Some small Coverity and related fixes for the recently merged vfio-user series.
> 
> thanks
> john

Mark,

Would you please re-add your R-b ?

Reviewed-by: Cédric Le Goater <clg@redhat.com>

and applied to vfio-next.

Thanks,

C.


> John Levon (4):
>    hw/vfio-user: add Cédric Le Goater as a maintainer
>    hw/vfio: fix region fd initialization
>    hw/vfio-user: wait for proxy close correctly
>    hw/vfio-user: fix use of uninitialized variable
> 
>   MAINTAINERS              |  1 +
>   hw/vfio-user/container.c |  6 +-----
>   hw/vfio-user/proxy.c     | 10 ++++++----
>   hw/vfio/device.c         |  6 +++++-
>   4 files changed, 13 insertions(+), 10 deletions(-)
> 

Re: [PATCH v2 0/4] vfio-user fixes

[Cédric Le Goater]

On 7/15/25 07:56, Cédric Le Goater wrote:
> On 7/15/25 07:52, John Levon wrote:
>> Some small Coverity and related fixes for the recently merged vfio-user series.
>>
>> thanks
>> john
> 
> Mark,
> 
> Would you please re-add your R-b ?

No need. b4 picked them up.


Thanks,

C.


> 
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> 
> and applied to vfio-next.
> 
> Thanks,
> 
> C.
> 
> 
>> John Levon (4):
>>    hw/vfio-user: add Cédric Le Goater as a maintainer
>>    hw/vfio: fix region fd initialization
>>    hw/vfio-user: wait for proxy close correctly
>>    hw/vfio-user: fix use of uninitialized variable
>>
>>   MAINTAINERS              |  1 +
>>   hw/vfio-user/container.c |  6 +-----
>>   hw/vfio-user/proxy.c     | 10 ++++++----
>>   hw/vfio/device.c         |  6 +++++-
>>   4 files changed, 13 insertions(+), 10 deletions(-)
>>
> 

Re: [PATCH v2 1/4] hw/vfio-user: add Cédric Le Goater as a maintainer

[Mark Cave-Ayland]

On 15/07/2025 06:52, John Levon wrote:

> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
>   MAINTAINERS | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e88ed2c0a9..30e9b71e6e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4287,6 +4287,7 @@ F: tests/functional/test_multiprocess.py
>   VFIO-USER:
>   M: John Levon <john.levon@nutanix.com>
>   M: Thanos Makatos <thanos.makatos@nutanix.com>
> +M: Cédric Le Goater <clg@redhat.com>
>   S: Supported
>   F: docs/interop/vfio-user.rst
>   F: docs/system/devices/vfio-user.rst

Acked-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>


ATB,

Mark.

Re: [PATCH v2 3/4] hw/vfio-user: wait for proxy close correctly

[Mark Cave-Ayland]

On 15/07/2025 06:52, John Levon wrote:

> Coverity reported:
> 
> CID 1611806: Concurrent data access violations (BAD_CHECK_OF_WAIT_COND)
> 
> A wait is performed without a loop. If there is a spurious wakeup, the
> condition may not be satisfied.
> 
> Fix this by checking ->state for VFIO_PROXY_CLOSED in a loop.
> 
> Resolves: Coverity CID 1611806
> Fixes: 0b3d881a ("vfio-user: implement message receive infrastructure")
> Signed-off-by: John Levon <john.levon@nutanix.com>

Is this definitely the right patch? The v2 posted at 
https://patchew.org/QEMU/20250711124500.1611628-1-john.levon@nutanix.com/ 
contains the updated commit message mentioning the rename of the 
callback, whereas this one doesn't?

> ---
>   hw/vfio-user/proxy.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
> index c418954440..2275d3fe39 100644
> --- a/hw/vfio-user/proxy.c
> +++ b/hw/vfio-user/proxy.c
> @@ -32,7 +32,6 @@ static void vfio_user_recycle(VFIOUserProxy *proxy, VFIOUserMsg *msg);
>   
>   static void vfio_user_recv(void *opaque);
>   static void vfio_user_send(void *opaque);
> -static void vfio_user_cb(void *opaque);
>   
>   static void vfio_user_request(void *opaque);
>   
> @@ -492,7 +491,7 @@ static void vfio_user_send(void *opaque)
>       }
>   }
>   
> -static void vfio_user_cb(void *opaque)
> +static void vfio_user_close_cb(void *opaque)
>   {
>       VFIOUserProxy *proxy = opaque;
>   
> @@ -984,8 +983,11 @@ void vfio_user_disconnect(VFIOUserProxy *proxy)
>        * handler to run after the proxy fd handlers were
>        * deleted above.
>        */
> -    aio_bh_schedule_oneshot(proxy->ctx, vfio_user_cb, proxy);
> -    qemu_cond_wait(&proxy->close_cv, &proxy->lock);
> +    aio_bh_schedule_oneshot(proxy->ctx, vfio_user_close_cb, proxy);
> +
> +    while (proxy->state != VFIO_PROXY_CLOSED) {
> +        qemu_cond_wait(&proxy->close_cv, &proxy->lock);
> +    }
>   
>       /* we now hold the only ref to proxy */
>       qemu_mutex_unlock(&proxy->lock);


ATB,

Mark.

Re: [PATCH v2 3/4] hw/vfio-user: wait for proxy close correctly

[John Levon]

On Tue, Jul 15, 2025 at 10:01:59AM +0100, Mark Cave-Ayland wrote:

> On 15/07/2025 06:52, John Levon wrote:
> 
> > Coverity reported:
> > 
> > CID 1611806: Concurrent data access violations (BAD_CHECK_OF_WAIT_COND)
> > 
> > A wait is performed without a loop. If there is a spurious wakeup, the
> > condition may not be satisfied.
> > 
> > Fix this by checking ->state for VFIO_PROXY_CLOSED in a loop.
> > 
> > Resolves: Coverity CID 1611806
> > Fixes: 0b3d881a ("vfio-user: implement message receive infrastructure")
> > Signed-off-by: John Levon <john.levon@nutanix.com>
> 
> Is this definitely the right patch? The v2 posted at
> https://patchew.org/QEMU/20250711124500.1611628-1-john.levon@nutanix.com/
> contains the updated commit message mentioning the rename of the callback,
> whereas this one doesn't?

Yep, sorry, picked the wrong commit message (but same commit contents).

Should I resend?

regards
john

Re: [PATCH v2 3/4] hw/vfio-user: wait for proxy close correctly

[Cédric Le Goater]

On 7/15/25 11:33, John Levon wrote:
> On Tue, Jul 15, 2025 at 10:01:59AM +0100, Mark Cave-Ayland wrote:
> 
>> On 15/07/2025 06:52, John Levon wrote:
>>
>>> Coverity reported:
>>>
>>> CID 1611806: Concurrent data access violations (BAD_CHECK_OF_WAIT_COND)
>>>
>>> A wait is performed without a loop. If there is a spurious wakeup, the
>>> condition may not be satisfied.
>>>
>>> Fix this by checking ->state for VFIO_PROXY_CLOSED in a loop.
>>>
>>> Resolves: Coverity CID 1611806
>>> Fixes: 0b3d881a ("vfio-user: implement message receive infrastructure")
>>> Signed-off-by: John Levon <john.levon@nutanix.com>
>>
>> Is this definitely the right patch? The v2 posted at
>> https://patchew.org/QEMU/20250711124500.1611628-1-john.levon@nutanix.com/
>> contains the updated commit message mentioning the rename of the callback,
>> whereas this one doesn't?
> 
> Yep, sorry, picked the wrong commit message (but same commit contents).
> 
> Should I resend?
yep. Please pick up the trailers.

Thanks,

C.