virtio_gpu_alloc_vbufs() creates a kmem_cache with the fixed name "virtio-gpu-vbufs" for every virtio-gpu device and destroys it from virtio_gpu_release(), which only runs once the last drm_device reference is dropped.
Two live virtio-gpu devices are enough to hit this: the second kmem_cache_create() call finds the name already taken and kmem_cache_sanity_check() WARNs. This reproduces at plain boot with two "-device virtio-gpu-pci" on the QEMU command line, no sysfs remove/rescan or held-open fd needed. kmem_cache of name 'virtio-gpu-vbufs' already exists WARNING: mm/slab_common.c:111 at __kmem_cache_create_args Call Trace: virtio_gpu_alloc_vbufs virtio_gpu_init virtio_gpu_probe ... syzbot found the same WARNing through a different path: open an fbdev node, then remove and rescan its PCI device over sysfs. The drm_device reference the open fd holds delays virtio_gpu_release(), so the old cache is still around when the rescan probes the device again and calls virtio_gpu_alloc_vbufs() a second time. Either way kmem_cache_create() still returns a valid (merged) cache, so the device keeps working; the WARN is not a probe failure. But it is a real defect: as the comment next to the check says, a duplicate name "confuses slabtop, et al", and it is exactly what syzbot and any panic_on_warn setup treat as a crash. The vbuf size is the same for every device, so create the cache once at module init and destroy it at module exit instead of per device. Reported-by: [email protected] Closes: https://syzkaller.appspot.com/bug?extid=1b129b44597a126d2d79 Signed-off-by: Nguyen Ngoc Thang <[email protected]> --- No Fixes: tag. I could not pin down which commit introduced the dedicated per-device cache (dc5698e80cf7, the original driver commit, still used a plain kzalloc() pool); happy to add the right one if someone can point to it. v3: back to one cache shared at module scope (Dmitry), not kzalloc()/ kfree() (v2) -- kmem_cache keeps the allocation off the general kmalloc slabs for this latency-sensitive path. Commit message corrected: the duplicate-name WARN does not fail the probe (the cache still gets created, merged with the existing one), and the trigger is any two live virtio-gpu devices, not just the syzbot remove/rescan path. Tested with two "-device virtio-gpu-pci" at once, and with one device torn down and rescanned while its fbdev node stays open. v2: https://lore.kernel.org/r/[email protected] v1: https://lore.kernel.org/r/[email protected] drivers/gpu/drm/virtio/virtgpu_drv.c | 13 ++++++++++++- drivers/gpu/drm/virtio/virtgpu_drv.h | 5 ++--- drivers/gpu/drm/virtio/virtgpu_kms.c | 8 -------- drivers/gpu/drm/virtio/virtgpu_vq.c | 26 ++++++++++++++------------ 4 files changed, 28 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c index 2aaa7cb08085..eec6f73a4c15 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.c +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c @@ -283,6 +283,10 @@ static int __init virtio_gpu_driver_init(void) struct pci_dev *pdev; int ret; + ret = virtio_gpu_vbufs_init(); + if (ret) + return ret; + pdev = pci_get_device(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_DEVICE_ID_VIRTIO_GPU, NULL); @@ -291,7 +295,7 @@ static int __init virtio_gpu_driver_init(void) VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM); if (ret) { pci_dev_put(pdev); - return ret; + goto err_vbufs; } } @@ -305,12 +309,19 @@ static int __init virtio_gpu_driver_init(void) pci_dev_put(pdev); } + if (ret) + goto err_vbufs; + return 0; + +err_vbufs: + virtio_gpu_vbufs_exit(); return ret; } static void __exit virtio_gpu_driver_exit(void) { unregister_virtio_driver(&virtio_gpu_driver); + virtio_gpu_vbufs_exit(); } module_init(virtio_gpu_driver_init); diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h index 9df4c7117341..f6009fc8472f 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.h +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h @@ -261,7 +261,6 @@ struct virtio_gpu_device { struct virtio_gpu_queue ctrlq; struct virtio_gpu_queue cursorq; bool vqs_released; - struct kmem_cache *vbufs; atomic_t pending_commands; @@ -361,8 +360,8 @@ void virtio_gpu_array_put_free_delayed(struct virtio_gpu_device *vgdev, void virtio_gpu_array_put_free_work(struct work_struct *work); /* virtgpu_vq.c */ -int virtio_gpu_alloc_vbufs(struct virtio_gpu_device *vgdev); -void virtio_gpu_free_vbufs(struct virtio_gpu_device *vgdev); +int virtio_gpu_vbufs_init(void); +void virtio_gpu_vbufs_exit(void); void virtio_gpu_reclaim_vbufs(struct virtio_gpu_device *vgdev); void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev, struct virtio_gpu_object *bo, diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c index 1d4d3bf46a20..047b591b5da2 100644 --- a/drivers/gpu/drm/virtio/virtgpu_kms.c +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c @@ -264,11 +264,6 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev) DRM_ERROR("failed to find virt queues\n"); goto err_vqs; } - ret = virtio_gpu_alloc_vbufs(vgdev); - if (ret) { - DRM_ERROR("failed to alloc vbufs\n"); - goto err_vbufs; - } /* get display info */ virtio_cread_le(vgdev->vdev, struct virtio_gpu_config, @@ -324,8 +319,6 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev) virtio_reset_device(vgdev->vdev); virtio_gpu_modeset_fini(vgdev); err_scanouts: - virtio_gpu_free_vbufs(vgdev); -err_vbufs: vgdev->vdev->config->del_vqs(vgdev->vdev); err_vqs: dev->dev_private = NULL; @@ -365,7 +358,6 @@ void virtio_gpu_release(struct drm_device *dev) return; virtio_gpu_modeset_fini(vgdev); - virtio_gpu_free_vbufs(vgdev); virtio_gpu_cleanup_cap_cache(vgdev); if (vgdev->has_host_visible) diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c index c02c03c10d92..769b6e70feda 100644 --- a/drivers/gpu/drm/virtio/virtgpu_vq.c +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c @@ -70,21 +70,23 @@ void virtio_gpu_cursor_ack(struct virtqueue *vq) schedule_work(&vgdev->cursorq.dequeue_work); } -int virtio_gpu_alloc_vbufs(struct virtio_gpu_device *vgdev) +/* Shared by all devices: a per-device cache would collide by name. */ +static struct kmem_cache *virtio_gpu_vbufs; + +int virtio_gpu_vbufs_init(void) { - vgdev->vbufs = kmem_cache_create("virtio-gpu-vbufs", - VBUFFER_SIZE, - __alignof__(struct virtio_gpu_vbuffer), - 0, NULL); - if (!vgdev->vbufs) + virtio_gpu_vbufs = kmem_cache_create("virtio-gpu-vbufs", + VBUFFER_SIZE, + __alignof__(struct virtio_gpu_vbuffer), + 0, NULL); + if (!virtio_gpu_vbufs) return -ENOMEM; return 0; } -void virtio_gpu_free_vbufs(struct virtio_gpu_device *vgdev) +void virtio_gpu_vbufs_exit(void) { - kmem_cache_destroy(vgdev->vbufs); - vgdev->vbufs = NULL; + kmem_cache_destroy(virtio_gpu_vbufs); } /* For drm_panic */ @@ -93,7 +95,7 @@ virtio_gpu_panic_get_vbuf(struct virtio_gpu_device *vgdev, int size) { struct virtio_gpu_vbuffer *vbuf; - vbuf = kmem_cache_zalloc(vgdev->vbufs, GFP_ATOMIC); + vbuf = kmem_cache_zalloc(virtio_gpu_vbufs, GFP_ATOMIC); vbuf->buf = (void *)vbuf + sizeof(*vbuf); vbuf->size = size; @@ -110,7 +112,7 @@ virtio_gpu_get_vbuf(struct virtio_gpu_device *vgdev, { struct virtio_gpu_vbuffer *vbuf; - vbuf = kmem_cache_zalloc(vgdev->vbufs, GFP_KERNEL | __GFP_NOFAIL); + vbuf = kmem_cache_zalloc(virtio_gpu_vbufs, GFP_KERNEL | __GFP_NOFAIL); BUG_ON(size > MAX_INLINE_CMD_SIZE || size < sizeof(struct virtio_gpu_ctrl_hdr)); @@ -205,7 +207,7 @@ static void free_vbuf(struct virtio_gpu_device *vgdev, if (vbuf->resp_size > MAX_INLINE_RESP_SIZE) kfree(vbuf->resp_buf); kvfree(vbuf->data_buf); - kmem_cache_free(vgdev->vbufs, vbuf); + kmem_cache_free(virtio_gpu_vbufs, vbuf); } void virtio_gpu_reclaim_vbufs(struct virtio_gpu_device *vgdev) -- 2.43.0
