[PATCH v2 0/2] Misc fixes on vfio display

[PATCH v2 0/2] Misc fixes on vfio display

[Zhenzhong Duan]

Hi,

This is trying to address an issue Cédric found.
See https://www.mail-archive.com/qemu-devel@nongnu.org/msg1043142.html
While looking into it, also found a potential memory leak.

I'm sorry that I didn't find how to test this fix, because it looks
a GFX card is needed. Any idea on how to test or help test are quite
appreciated.

Thanks
Zhenzhong

v2:
- set dpy->edid_info to NULL in vfio_display_edid_init() err path (Marc-André)
- remove a wrongly added g_free(*info) in vfio_get_dev_region_info() (Marc-André)
- add R-B on patch2


Zhenzhong Duan (2):
  vfio/display: Fix potential memleak of edid info
  vfio/display: Fix vfio_display_edid_init() error path

 hw/vfio/display.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] vfio/display: Fix potential memleak of edid info

[Zhenzhong Duan]

EDID related device region info is leaked in vfio_display_edid_init()
error path and VFIODisplay destroying path.

Fixes: 08479114b0de ("vfio/display: add edid support.")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 661e921616..9c57fd3888 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -171,7 +171,9 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
 
 err:
     trace_vfio_display_edid_write_error();
+    g_free(dpy->edid_info);
     g_free(dpy->edid_regs);
+    dpy->edid_info = NULL;
     dpy->edid_regs = NULL;
     return;
 }
@@ -182,6 +184,7 @@ static void vfio_display_edid_exit(VFIODisplay *dpy)
         return;
     }
 
+    g_free(dpy->edid_info);
     g_free(dpy->edid_regs);
     g_free(dpy->edid_blob);
     timer_free(dpy->edid_link_timer);
-- 
2.34.1

[PATCH v2 2/2] vfio/display: Fix vfio_display_edid_init() error path

[Zhenzhong Duan]

vfio_display_edid_init() can fail for many reasons and return silently.
It would be good to report the error.

Old mdev driver may not support vfio edid region and we allow to go
through in this case.

vfio_display_edid_update() isn't changed because it can be called at
runtime when UI changes (i.e. window resize).

Fixes: 08479114b0de ("vfio/display: add edid support.")
Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/vfio/display.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 9c57fd3888..ea87830fe0 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -124,7 +124,7 @@ static void vfio_display_edid_ui_info(void *opaque, uint32_t idx,
     }
 }
 
-static void vfio_display_edid_init(VFIOPCIDevice *vdev)
+static bool vfio_display_edid_init(VFIOPCIDevice *vdev, Error **errp)
 {
     VFIODisplay *dpy = vdev->dpy;
     int fd = vdev->vbasedev.fd;
@@ -135,7 +135,8 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
                                    VFIO_REGION_SUBTYPE_GFX_EDID,
                                    &dpy->edid_info);
     if (ret) {
-        return;
+        /* Failed to get GFX edid info, allow to go through without edid. */
+        return true;
     }
 
     trace_vfio_display_edid_available();
@@ -167,15 +168,16 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
                                         vfio_display_edid_link_up, vdev);
 
     vfio_display_edid_update(vdev, true, 0, 0);
-    return;
+    return true;
 
 err:
+    error_setg(errp, "vfio: failed to read GFX edid field");
     trace_vfio_display_edid_write_error();
     g_free(dpy->edid_info);
     g_free(dpy->edid_regs);
     dpy->edid_info = NULL;
     dpy->edid_regs = NULL;
-    return;
+    return false;
 }
 
 static void vfio_display_edid_exit(VFIODisplay *dpy)
@@ -368,8 +370,7 @@ static bool vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
             return false;
         }
     }
-    vfio_display_edid_init(vdev);
-    return true;
+    return vfio_display_edid_init(vdev, errp);
 }
 
 static void vfio_display_dmabuf_exit(VFIODisplay *dpy)
-- 
2.34.1

Re: [PATCH v2 1/2] vfio/display: Fix potential memleak of edid info

[Cédric Le Goater]

On 7/1/24 3:48 AM, Zhenzhong Duan wrote:
> EDID related device region info is leaked in vfio_display_edid_init()
> error path and VFIODisplay destroying path.
> 
> Fixes: 08479114b0de ("vfio/display: add edid support.")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>


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

Thanks,

C.


> ---
>   hw/vfio/display.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index 661e921616..9c57fd3888 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -171,7 +171,9 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
>   
>   err:
>       trace_vfio_display_edid_write_error();
> +    g_free(dpy->edid_info);
>       g_free(dpy->edid_regs);
> +    dpy->edid_info = NULL;
>       dpy->edid_regs = NULL;
>       return;
>   }
> @@ -182,6 +184,7 @@ static void vfio_display_edid_exit(VFIODisplay *dpy)
>           return;
>       }
>   
> +    g_free(dpy->edid_info);
>       g_free(dpy->edid_regs);
>       g_free(dpy->edid_blob);
>       timer_free(dpy->edid_link_timer);

Re: [PATCH v2 1/2] vfio/display: Fix potential memleak of edid info

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]

On Mon, Jul 1, 2024 at 5:51 AM Zhenzhong Duan <zhenzhong.duan@intel.com>
wrote:

> EDID related device region info is leaked in vfio_display_edid_init()
> error path and VFIODisplay destroying path.
>
> Fixes: 08479114b0de ("vfio/display: add edid support.")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  hw/vfio/display.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index 661e921616..9c57fd3888 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -171,7 +171,9 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
>
>  err:
>      trace_vfio_display_edid_write_error();
> +    g_free(dpy->edid_info);
>      g_free(dpy->edid_regs);
> +    dpy->edid_info = NULL;
>      dpy->edid_regs = NULL;
>      return;
>  }
> @@ -182,6 +184,7 @@ static void vfio_display_edid_exit(VFIODisplay *dpy)
>          return;
>      }
>
> +    g_free(dpy->edid_info);
>      g_free(dpy->edid_regs);
>      g_free(dpy->edid_blob);
>      timer_free(dpy->edid_link_timer);
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2063 bytes --]

Re: [PATCH v2 0/2] Misc fixes on vfio display

[Cédric Le Goater]

On 7/1/24 3:48 AM, Zhenzhong Duan wrote:
> Hi,
> 
> This is trying to address an issue Cédric found.
> See https://www.mail-archive.com/qemu-devel@nongnu.org/msg1043142.html
> While looking into it, also found a potential memory leak.
> 
> I'm sorry that I didn't find how to test this fix, because it looks
> a GFX card is needed. Any idea on how to test or help test are quite
> appreciated.
> 
> Thanks
> Zhenzhong
> 
> v2:
> - set dpy->edid_info to NULL in vfio_display_edid_init() err path (Marc-André)
> - remove a wrongly added g_free(*info) in vfio_get_dev_region_info() (Marc-André)
> - add R-B on patch2
> 
> 
> Zhenzhong Duan (2):
>    vfio/display: Fix potential memleak of edid info
>    vfio/display: Fix vfio_display_edid_init() error path
> 
>   hw/vfio/display.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 


Applied to vfio-next.

Thanks,

C.