1. Patchew
  2. QEMU
  3. [PATCH 0/7] vhost-user-gpu: fix several security issues
  4. View patch

[PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'

Li Qiang posted 7 patches 4 years, 9 months ago
Maintainers: Gerd Hoffmann <kraxel@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>
There is a newer version of this series
[PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
Posted by Li Qiang 4 years, 9 months ago 
If 'virgl_renderer_resource_attach_iov' failed, the 'res_iovs' will
be leaked.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 contrib/vhost-user-gpu/virgl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index c669d73a1d..a16a311d80 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
         return;
     }
 
-    virgl_renderer_resource_attach_iov(att_rb.resource_id,
+    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
                                        res_iovs, att_rb.nr_entries);
+    if (ret != 0) {
+        g_free(res_iovs);
+    }
 }
 
 static void
-- 
2.25.1
Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
Posted by P J P 4 years, 9 months ago 
+-- On Tue, 4 May 2021, Li Qiang wrote --+
| diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
| index c669d73a1d..a16a311d80 100644
| --- a/contrib/vhost-user-gpu/virgl.c
| +++ b/contrib/vhost-user-gpu/virgl.c
| @@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
|          return;
|      }
|  
| -    virgl_renderer_resource_attach_iov(att_rb.resource_id,
| +    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
|                                         res_iovs, att_rb.nr_entries);
| +    if (ret != 0) {
| +        g_free(res_iovs);
| +    }
|  }

* Similar to earlier, 
  hw/display/virtio-gpu-3d.c:virgl_resource_attach_backing() calls 
  'virtio_gpu_cleanup_mapping_iov'

* should it be same for vhost-user-gpu?


Thank you.
--
 - P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
Posted by Marc-André Lureau 4 years, 9 months ago 
Hi

On Wed, May 5, 2021 at 12:03 PM P J P <ppandit@redhat.com> wrote:

> +-- On Tue, 4 May 2021, Li Qiang wrote --+
> | diff --git a/contrib/vhost-user-gpu/virgl.c
> b/contrib/vhost-user-gpu/virgl.c
> | index c669d73a1d..a16a311d80 100644
> | --- a/contrib/vhost-user-gpu/virgl.c
> | +++ b/contrib/vhost-user-gpu/virgl.c
> | @@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
> |          return;
> |      }
> |
> | -    virgl_renderer_resource_attach_iov(att_rb.resource_id,
> | +    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
> |                                         res_iovs, att_rb.nr_entries);
> | +    if (ret != 0) {
> | +        g_free(res_iovs);
> | +    }
> |  }
>
> * Similar to earlier,
>   hw/display/virtio-gpu-3d.c:virgl_resource_attach_backing() calls
>   'virtio_gpu_cleanup_mapping_iov'
>
> * should it be same for vhost-user-gpu?
>
>
>
Good question, that's also what you did when you fixed it for virtio-gpu in:

commit 33243031dad02d161225ba99d782616da133f689
Author: Li Qiang <liq3ea@gmail.com>
Date:   Thu Dec 29 03:11:26 2016 -0500

    virtio-gpu-3d: fix memory leak in resource attach backing


Btw, for each patch, it would be nice to have a reference to the original
fix in virtio-gpu.

Thanks!
Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
Posted by Li Qiang 4 years, 9 months ago 
Marc-André Lureau <marcandre.lureau@gmail.com> 于2021年5月5日周三 下午5:08写道：
>
> Hi
>
> On Wed, May 5, 2021 at 12:03 PM P J P <ppandit@redhat.com> wrote:
>>
>> +-- On Tue, 4 May 2021, Li Qiang wrote --+
>> | diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
>> | index c669d73a1d..a16a311d80 100644
>> | --- a/contrib/vhost-user-gpu/virgl.c
>> | +++ b/contrib/vhost-user-gpu/virgl.c
>> | @@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
>> |          return;
>> |      }
>> |
>> | -    virgl_renderer_resource_attach_iov(att_rb.resource_id,
>> | +    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
>> |                                         res_iovs, att_rb.nr_entries);
>> | +    if (ret != 0) {
>> | +        g_free(res_iovs);
>> | +    }
>> |  }
>>
>> * Similar to earlier,
>>   hw/display/virtio-gpu-3d.c:virgl_resource_attach_backing() calls
>>   'virtio_gpu_cleanup_mapping_iov'
>>
>> * should it be same for vhost-user-gpu?
>>
>>
>
> Good question, that's also what you did when you fixed it for virtio-gpu in:
>
> commit 33243031dad02d161225ba99d782616da133f689
> Author: Li Qiang <liq3ea@gmail.com>
> Date:   Thu Dec 29 03:11:26 2016 -0500
>
>     virtio-gpu-3d: fix memory leak in resource attach backing
>

Do you mean this;
-->https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01156.html

I think there is no need for this.

The virtio_gpu_cleanup_mapping_iov is needed in virtio-gpu is because
it need map guest memory.
But in vhost-user-gpu case, the 'vg_create_mapping_iov' calls
'vu_gpa_to_va' and this function don't need
do map memory.

But for the beauty of symmetry we can abstract a function called
'vg_destroy_mapping_iov' and it just calls g_free(res->iov).
Like the pair 'virtio_gpu_create_mapping_iov' and 'virtio_gpu_cleanup_mapping'.

Thanks,
Li Qiang

>
> Btw, for each patch, it would be nice to have a reference to the original fix in virtio-gpu.
>
> Thanks!
>
Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
Posted by Marc-André Lureau 4 years, 9 months ago 
Hi

On Wed, May 5, 2021 at 1:24 PM Li Qiang <liq3ea@gmail.com> wrote:

> Marc-André Lureau <marcandre.lureau@gmail.com> 于2021年5月5日周三 下午5:08写道：
> >
> > Hi
> >
> > On Wed, May 5, 2021 at 12:03 PM P J P <ppandit@redhat.com> wrote:
> >>
> >> +-- On Tue, 4 May 2021, Li Qiang wrote --+
> >> | diff --git a/contrib/vhost-user-gpu/virgl.c
> b/contrib/vhost-user-gpu/virgl.c
> >> | index c669d73a1d..a16a311d80 100644
> >> | --- a/contrib/vhost-user-gpu/virgl.c
> >> | +++ b/contrib/vhost-user-gpu/virgl.c
> >> | @@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
> >> |          return;
> >> |      }
> >> |
> >> | -    virgl_renderer_resource_attach_iov(att_rb.resource_id,
> >> | +    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
> >> |                                         res_iovs, att_rb.nr_entries);
> >> | +    if (ret != 0) {
> >> | +        g_free(res_iovs);
> >> | +    }
> >> |  }
> >>
> >> * Similar to earlier,
> >>   hw/display/virtio-gpu-3d.c:virgl_resource_attach_backing() calls
> >>   'virtio_gpu_cleanup_mapping_iov'
> >>
> >> * should it be same for vhost-user-gpu?
> >>
> >>
> >
> > Good question, that's also what you did when you fixed it for virtio-gpu
> in:
> >
> > commit 33243031dad02d161225ba99d782616da133f689
> > Author: Li Qiang <liq3ea@gmail.com>
> > Date:   Thu Dec 29 03:11:26 2016 -0500
> >
> >     virtio-gpu-3d: fix memory leak in resource attach backing
> >
>
> Do you mean this;
> -->https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01156.html
>
> I think there is no need for this.
>
> The virtio_gpu_cleanup_mapping_iov is needed in virtio-gpu is because
> it need map guest memory.
> But in vhost-user-gpu case, the 'vg_create_mapping_iov' calls
> 'vu_gpa_to_va' and this function don't need
> do map memory.
>
> But for the beauty of symmetry we can abstract a function called
> 'vg_destroy_mapping_iov' and it just calls g_free(res->iov).
> Like the pair 'virtio_gpu_create_mapping_iov' and
> 'virtio_gpu_cleanup_mapping'.
>
>
Right. I think I like the suggestion to add a 'virtio_gpu_cleanup_mapping'
(with a comment to explain it) to avoid this kind of question again. Feel
free to add a patch on top if you want.

thanks


>
> >
> > Btw, for each patch, it would be nice to have a reference to the
> original fix in virtio-gpu.
> >
> > Thanks!
> >
>


-- 
Marc-André Lureau

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.