| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6x2iqra6gpna5gdd4m73gkgo46bp6w2e3gm7raibelbn3gmnkg@2g6oeskj73yv>
Date: Tue, 10 Jun 2025 15:27:38 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Manos Pitsidianakis <manos.pitsidianakis@...aro.org>
Cc: David Airlie <airlied@...hat.com>, Gerd Hoffmann <kraxel@...hat.com>,
Dmitry Osipenko <dmitry.osipenko@...labora.com>, Gurchetan Singh <gurchetansingh@...omium.org>,
Chia-I Wu <olvaffe@...il.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
Simona Vetter <simona@...ll.ch>, Alex Bennée <alex.bennee@...aro.org>,
Viresh Kumar <viresh.kumar@...aro.org>, dri-devel@...ts.freedesktop.org, virtualization@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] virtgpu: add virtio_gpu_queue_cleanup()
On Mon, May 05, 2025 at 11:59:14AM +0300, Manos Pitsidianakis wrote:
>When virtio_gpu_remove() is called, the queues are flushed and used
>buffers from the virtqueues are freed. However, the VIRTIO device might
>have left unused buffers in the avail rings, resulting in memory leaks.
>KASAN, slab debug and drm_mm_takedown all report the errors:
>
> BUG virtio-gpu-vbufs: Objects remaining in virtio-gpu-vbufs on
> __kmem_cache_shutdown()
> <- Snipped backtrace ->
> Object 0xffffff801b07c008 @offset=8
> Allocated in virtio_gpu_get_vbuf.isra.0+0x38/0xb0 age=4314 cpu=3
> pid=540
> kmem_cache_alloc+0x330/0x3a8
> virtio_gpu_get_vbuf.isra.0+0x38/0xb0
> virtio_gpu_cmd_resource_create_3d+0x60/0x1f0
> virtio_gpu_object_create+0x388/0x468
> virtio_gpu_resource_create_ioctl+0x1f0/0x420
> drm_ioctl_kernel+0x170/0x248
> drm_ioctl+0x33c/0x680
> __arm64_sys_ioctl+0xdc/0x128
> invoke_syscall+0x84/0x1c8
> el0_svc_common.constprop.0+0x11c/0x150
> do_el0_svc+0x38/0x50
> el0_svc+0x38/0x70
> el0t_64_sync_handler+0x120/0x130
> el0t_64_sync+0x190/0x198
>
> ------------[ cut here ]------------
> kmem_cache_destroy virtio-gpu-vbufs: Slab cache still has objects when
> called from virtio_gpu_free_vbufs+0x48/0x70
> WARNING: CPU: 0 PID: 483 at mm/slab_common.c:498
> kmem_cache_destroy+0x114/0x178
> <- Snipped info ->
>
> ------------[ cut here ]------------
> Memory manager not clean during takedown.
> <- Snipped info ->
> ---[ end trace 0000000000000000 ]---
> [drm:drm_mm_takedown] *ERROR* node [001000eb + 00000080]: inserted at
> drm_mm_insert_node_in_range+0x48c/0x6a8
> drm_vma_offset_add+0x84/0xb0
> drm_gem_create_mmap_offset+0x50/0x70
> __drm_gem_shmem_create+0x94/0x1d8
> drm_gem_shmem_create+0x1c/0x30
> virtio_gpu_object_create+0x68/0x468
> virtio_gpu_resource_create_ioctl+0x1f0/0x420
> drm_ioctl_kernel+0x170/0x248
> drm_ioctl+0x33c/0x680
> __arm64_sys_ioctl+0xdc/0x128
> invoke_syscall+0x84/0x1c8
> el0_svc_common.constprop.0+0x11c/0x150
> do_el0_svc+0x38/0x50
> el0_svc+0x38/0x70
> el0t_64_sync_handler+0x120/0x130
> el0t_64_sync+0x190/0x198
> [drm:drm_mm_takedown] *ERROR* node [0010016b + 000000eb]: inserted at
> <- Snipped info ->
>
>The leaked objects are also reported in /sys/kernel/debug/kmemleak.
>
>This commit adds a cleanup function that is called from
>virtio_gpu_deinit().
>
>The function cleans up any unused buffers from the virtqueues and calls
>the appropriate freeing functions. This is safe to do so because
>virtio_gpu_deinit() calls virtio_reset_device() before calling the
>cleanup function, ensuring no one is going to read from the virtqueues.
>
>The cleanup function checks for used buffers on the queues, and
>additionally calls virtqueue_detach_unused_buf on each queue to get any
>buffers that did not have time to be processed by the VIRTIO backend.
>
>Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@...aro.org>
>---
> drivers/gpu/drm/virtio/virtgpu_drv.h | 1 +
> drivers/gpu/drm/virtio/virtgpu_kms.c | 1 +
> drivers/gpu/drm/virtio/virtgpu_vq.c | 55 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 57 insertions(+)
>
>diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
>index f17660a71a3e7a22b5d4fefa6b754c227a294037..b3d367be6f204dbc98bf1c6e5c43a37ac8c0d8b3 100644
>--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
>+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
>@@ -419,6 +419,7 @@ void virtio_gpu_cursor_ack(struct virtqueue *vq);
> void virtio_gpu_dequeue_ctrl_func(struct work_struct *work);
> void virtio_gpu_dequeue_cursor_func(struct work_struct *work);
> void virtio_gpu_panic_notify(struct virtio_gpu_device *vgdev);
>+void virtio_gpu_queue_cleanup(struct virtio_gpu_device *vgdev);
> void virtio_gpu_notify(struct virtio_gpu_device *vgdev);
>
> int
>diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
>index 7dfb2006c561ca13b15d979ddb8bf2d753e35dad..da70d9248072b64786a5d48b71bccaa80b8aae8f 100644
>--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
>+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
>@@ -286,6 +286,7 @@ void virtio_gpu_deinit(struct drm_device *dev)
> flush_work(&vgdev->cursorq.dequeue_work);
> flush_work(&vgdev->config_changed_work);
> virtio_reset_device(vgdev->vdev);
>+ virtio_gpu_queue_cleanup(vgdev);
> vgdev->vdev->config->del_vqs(vgdev->vdev);
> }
>
>diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
>index
>55a15e247dd1ad53a2b43b19fca8879b956f0e1a..fd150827e413cedcec4d82b0da8d792cb67e243f
>100644
>--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
>+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
>@@ -299,6 +299,61 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct *work)
> wake_up(&vgdev->cursorq.ack_queue);
> }
>
>+/* deallocate all in-flight virtqueue elements */
>+void virtio_gpu_queue_cleanup(struct virtio_gpu_device *vgdev)
>+{
>+ struct list_head reclaim_list;
>+ struct virtio_gpu_vbuffer *entry, *tmp;
>+
>+ INIT_LIST_HEAD(&reclaim_list);
>+ spin_lock(&vgdev->ctrlq.qlock);
>+ do {
>+ virtqueue_disable_cb(vgdev->ctrlq.vq);
>+ reclaim_vbufs(vgdev->ctrlq.vq, &reclaim_list);
>+ } while (!virtqueue_enable_cb(vgdev->ctrlq.vq));
IIUC this function (virtio_gpu_queue_cleanup) is called after device is
reset, so do we really need this cycle to disable notification and
double check if the device queued new stuff?
I mean, maybe we can just call reclaim_vbufs(), since the device may not
queue new elements anymore.
>+ /* detach unused buffers */
>+ while ((entry = virtqueue_detach_unused_buf(vgdev->ctrlq.vq)) != NULL) {
What about implementing a new reclaim_unused_vbufs(), similar to
reclaim_vbufs() but calling virtqueue_detach_unused_buf() and queuing on
reclaim_list, so we can do a single loop to clean all buffer in the same
way?
>+ if (entry->resp_cb)
>+ entry->resp_cb(vgdev, entry);
Is it fine to call `entry->resp_cb` on an "unused" buffer?
>+ if (entry->objs)
>+ virtio_gpu_array_put_free(entry->objs);
>+ free_vbuf(vgdev, entry);
>+ }
>+ spin_unlock(&vgdev->ctrlq.qlock);
>+
>+ list_for_each_entry_safe(entry, tmp, &reclaim_list, list) {
>+ if (entry->resp_cb)
>+ entry->resp_cb(vgdev, entry);
>+ if (entry->objs)
>+ virtio_gpu_array_put_free(entry->objs);
>+ list_del(&entry->list);
>+ free_vbuf(vgdev, entry);
>+ }
>+
Similar comments also on the next section.
They looks very similar, so not sure if we can avoid code duplication
adding a new function (e.g. renaming this function in
`virtio_gpu_queues_cleanup()` and add a new `virtio_gpu_queue_cleanup()`
to be called on the 2 virtqueues). Just an idea, not a strong opinion.
Thanks,
Stefano
>+ spin_lock(&vgdev->cursorq.qlock);
>+ do {
>+ virtqueue_disable_cb(vgdev->cursorq.vq);
>+ reclaim_vbufs(vgdev->cursorq.vq, &reclaim_list);
>+ } while (!virtqueue_enable_cb(vgdev->cursorq.vq));
>+ spin_unlock(&vgdev->cursorq.qlock);
>+ while ((entry = virtqueue_detach_unused_buf(vgdev->cursorq.vq)) != NULL) {
>+ if (entry->resp_cb)
>+ entry->resp_cb(vgdev, entry);
>+ if (entry->objs)
>+ virtio_gpu_array_put_free(entry->objs);
>+ free_vbuf(vgdev, entry);
>+ }
>+
>+ list_for_each_entry_safe(entry, tmp, &reclaim_list, list) {
>+ if (entry->resp_cb)
>+ entry->resp_cb(vgdev, entry);
>+ if (entry->objs)
>+ virtio_gpu_array_put_free(entry->objs);
>+ list_del(&entry->list);
>+ free_vbuf(vgdev, entry);
>+ }
>+}
>+
> /* Create sg_table from a vmalloc'd buffer. */
> static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size, int *sg_ents)
> {
>
>--
>2.47.2
>
>
Powered by blists - more mailing lists