* [PATCH 0/2] Misc fixes on vfio display @ 2024-06-28 9:28 Zhenzhong Duan 2024-06-28 9:28 ` [PATCH 1/2] vfio/display: Fix potential memleak of edid info Zhenzhong Duan 2024-06-28 9:28 ` [PATCH 2/2] vfio/display: Fix vfio_display_edid_init() error path Zhenzhong Duan 0 siblings, 2 replies; 6+ messages in thread From: Zhenzhong Duan @ 2024-06-28 9:28 UTC (permalink / raw) To: qemu-devel; +Cc: alex.williamson, clg, kraxel, chao.p.peng, 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 Zhenzhong Duan (2): vfio/display: Fix potential memleak of edid info vfio/display: Fix vfio_display_edid_init() error path hw/vfio/display.c | 15 +++++++++------ hw/vfio/helpers.c | 1 + 2 files changed, 10 insertions(+), 6 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] vfio/display: Fix potential memleak of edid info 2024-06-28 9:28 [PATCH 0/2] Misc fixes on vfio display Zhenzhong Duan @ 2024-06-28 9:28 ` Zhenzhong Duan 2024-06-29 12:15 ` Marc-André Lureau 2024-06-28 9:28 ` [PATCH 2/2] vfio/display: Fix vfio_display_edid_init() error path Zhenzhong Duan 1 sibling, 1 reply; 6+ messages in thread From: Zhenzhong Duan @ 2024-06-28 9:28 UTC (permalink / raw) To: qemu-devel; +Cc: alex.williamson, clg, kraxel, chao.p.peng, Zhenzhong Duan EDID related device region info is leaked in three paths: 1. In vfio_get_dev_region_info(), when edid info isn't find, the last device region info is leaked. 2. In vfio_display_edid_init() error path, edid info is leaked. 3. In VFIODisplay destroying path, edid info is leaked. Fixes: 08479114b0de ("vfio/display: add edid support.") Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- hw/vfio/display.c | 2 ++ hw/vfio/helpers.c | 1 + 2 files changed, 3 insertions(+) diff --git a/hw/vfio/display.c b/hw/vfio/display.c index 661e921616..5926bd6628 100644 --- a/hw/vfio/display.c +++ b/hw/vfio/display.c @@ -171,6 +171,7 @@ 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_regs = NULL; return; @@ -182,6 +183,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); diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c index b14edd46ed..3dd32b26a4 100644 --- a/hw/vfio/helpers.c +++ b/hw/vfio/helpers.c @@ -586,6 +586,7 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type, g_free(*info); } + g_free(*info); *info = NULL; return -ENODEV; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vfio/display: Fix potential memleak of edid info 2024-06-28 9:28 ` [PATCH 1/2] vfio/display: Fix potential memleak of edid info Zhenzhong Duan @ 2024-06-29 12:15 ` Marc-André Lureau 2024-07-01 1:31 ` Duan, Zhenzhong 0 siblings, 1 reply; 6+ messages in thread From: Marc-André Lureau @ 2024-06-29 12:15 UTC (permalink / raw) To: Zhenzhong Duan; +Cc: qemu-devel, alex.williamson, clg, kraxel, chao.p.peng [-- Attachment #1: Type: text/plain, Size: 1803 bytes --] Hi On Fri, Jun 28, 2024 at 1:32 PM Zhenzhong Duan <zhenzhong.duan@intel.com> wrote: > EDID related device region info is leaked in three paths: > 1. In vfio_get_dev_region_info(), when edid info isn't find, the last > device region info is leaked. > 2. In vfio_display_edid_init() error path, edid info is leaked. > 3. In VFIODisplay destroying path, edid info is leaked. > > Fixes: 08479114b0de ("vfio/display: add edid support.") > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/vfio/display.c | 2 ++ > hw/vfio/helpers.c | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/hw/vfio/display.c b/hw/vfio/display.c > index 661e921616..5926bd6628 100644 > --- a/hw/vfio/display.c > +++ b/hw/vfio/display.c > @@ -171,6 +171,7 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev) > > err: > trace_vfio_display_edid_write_error(); > + g_free(dpy->edid_info); > It would be better to set it to NULL. > g_free(dpy->edid_regs); > dpy->edid_regs = NULL; > return; > @@ -182,6 +183,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); > diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c > index b14edd46ed..3dd32b26a4 100644 > --- a/hw/vfio/helpers.c > +++ b/hw/vfio/helpers.c > @@ -586,6 +586,7 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, > uint32_t type, > g_free(*info); > } > > + g_free(*info); > This seems incorrect, it is freed at the end of the loop above if it didn't retun. > *info = NULL; > return -ENODEV; > } > -- > 2.34.1 > > > -- Marc-André Lureau [-- Attachment #2: Type: text/html, Size: 2810 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vfio/display: Fix potential memleak of edid info 2024-06-29 12:15 ` Marc-André Lureau @ 2024-07-01 1:31 ` Duan, Zhenzhong 0 siblings, 0 replies; 6+ messages in thread From: Duan, Zhenzhong @ 2024-07-01 1:31 UTC (permalink / raw) To: Marc-André Lureau Cc: qemu-devel, alex.williamson, clg, kraxel, chao.p.peng Hi, On 6/29/2024 8:15 PM, Marc-André Lureau wrote: > Hi > > On Fri, Jun 28, 2024 at 1:32 PM Zhenzhong Duan > <zhenzhong.duan@intel.com> wrote: > > EDID related device region info is leaked in three paths: > 1. In vfio_get_dev_region_info(), when edid info isn't find, the last > device region info is leaked. > 2. In vfio_display_edid_init() error path, edid info is leaked. > 3. In VFIODisplay destroying path, edid info is leaked. > > Fixes: 08479114b0de ("vfio/display: add edid support.") > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/vfio/display.c | 2 ++ > hw/vfio/helpers.c | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/hw/vfio/display.c b/hw/vfio/display.c > index 661e921616..5926bd6628 100644 > --- a/hw/vfio/display.c > +++ b/hw/vfio/display.c > @@ -171,6 +171,7 @@ static void > vfio_display_edid_init(VFIOPCIDevice *vdev) > > err: > trace_vfio_display_edid_write_error(); > + g_free(dpy->edid_info); > > > It would be better to set it to NULL. Will do. > > g_free(dpy->edid_regs); > dpy->edid_regs = NULL; > return; > @@ -182,6 +183,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); > diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c > index b14edd46ed..3dd32b26a4 100644 > --- a/hw/vfio/helpers.c > +++ b/hw/vfio/helpers.c > @@ -586,6 +586,7 @@ int vfio_get_dev_region_info(VFIODevice > *vbasedev, uint32_t type, > g_free(*info); > } > > + g_free(*info); > > > This seems incorrect, it is freed at the end of the loop above if it > didn't retun. Good catch! Will remove it. Thanks Zhenzhong > > *info = NULL; > return -ENODEV; > } > -- > 2.34.1 > > > > > -- > Marc-André Lureau ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] vfio/display: Fix vfio_display_edid_init() error path 2024-06-28 9:28 [PATCH 0/2] Misc fixes on vfio display Zhenzhong Duan 2024-06-28 9:28 ` [PATCH 1/2] vfio/display: Fix potential memleak of edid info Zhenzhong Duan @ 2024-06-28 9:28 ` Zhenzhong Duan 2024-06-29 12:17 ` Marc-André Lureau 1 sibling, 1 reply; 6+ messages in thread From: Zhenzhong Duan @ 2024-06-28 9:28 UTC (permalink / raw) To: qemu-devel; +Cc: alex.williamson, clg, kraxel, chao.p.peng, 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> --- 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 5926bd6628..462845ce69 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,14 +168,15 @@ 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_regs = NULL; - return; + return false; } static void vfio_display_edid_exit(VFIODisplay *dpy) @@ -367,8 +369,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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] vfio/display: Fix vfio_display_edid_init() error path 2024-06-28 9:28 ` [PATCH 2/2] vfio/display: Fix vfio_display_edid_init() error path Zhenzhong Duan @ 2024-06-29 12:17 ` Marc-André Lureau 0 siblings, 0 replies; 6+ messages in thread From: Marc-André Lureau @ 2024-06-29 12:17 UTC (permalink / raw) To: Zhenzhong Duan; +Cc: qemu-devel, alex.williamson, clg, kraxel, chao.p.peng [-- Attachment #1: Type: text/plain, Size: 2613 bytes --] On Fri, Jun 28, 2024 at 1:31 PM Zhenzhong Duan <zhenzhong.duan@intel.com> wrote: > 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 5926bd6628..462845ce69 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,14 +168,15 @@ 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_regs = NULL; > - return; > + return false; > } > > static void vfio_display_edid_exit(VFIODisplay *dpy) > @@ -367,8 +369,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 > > > -- Marc-André Lureau [-- Attachment #2: Type: text/html, Size: 3714 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-07-01 1:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-06-28 9:28 [PATCH 0/2] Misc fixes on vfio display Zhenzhong Duan 2024-06-28 9:28 ` [PATCH 1/2] vfio/display: Fix potential memleak of edid info Zhenzhong Duan 2024-06-29 12:15 ` Marc-André Lureau 2024-07-01 1:31 ` Duan, Zhenzhong 2024-06-28 9:28 ` [PATCH 2/2] vfio/display: Fix vfio_display_edid_init() error path Zhenzhong Duan 2024-06-29 12:17 ` Marc-André Lureau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).