Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/virtio: missing pending_commands reset in virtio_gpu_reclaim_vbufs() - [High] drm/virtio: race in virtio_gpu_deinit() flushing work before device reset - [High] drm/virtio: uninterruptible hang in virtio_gpu_reclaim_vbufs() on imported dmabuf --
commit f24f642758ec3795a40ce21fd3080771f4fdffb2 Author: Anuj Bolewar <[email protected]> drm/virtio: reclaim pending vbufs before tearing down vqs This commit drains unused vbuffers from the control and cursor virtqueues in virtio_gpu_deinit() after resetting the device and before deleting the virtqueues, releasing any held GEM object references and pending RESOURCE_UNREF objects before the vbufs kmem_cache is destroyed. Link: https://patch.msgid.link/[email protected] [ ... ] > diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c > b/drivers/gpu/drm/virtio/virtgpu_kms.c > index 1c15cbf326b78..59173746c9494 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_reclaim_vbufs(vgdev); > vgdev->vdev->config->del_vqs(vgdev->vdev); > } [Severity: High] Could flushing the workqueues before virtio_reset_device() in virtio_gpu_deinit() race with virtio_gpu_reclaim_vbufs() and del_vqs()? In drivers/gpu/drm/virtio/virtgpu_kms.c:virtio_gpu_deinit(): flush_work(&vgdev->obj_free_work); flush_work(&vgdev->ctrlq.dequeue_work); flush_work(&vgdev->cursorq.dequeue_work); flush_work(&vgdev->config_changed_work); virtio_reset_device(vgdev->vdev); virtio_gpu_reclaim_vbufs(vgdev); vgdev->vdev->config->del_vqs(vgdev->vdev); Until virtio_reset_device(vgdev->vdev) completes, the virtio device can still raise virtqueue and config interrupts (virtio_gpu_ctrl_ack(), virtio_gpu_cursor_ack(), and virtio_gpu_config_changed()), which call schedule_work() and can re-queue ctrlq.dequeue_work, cursorq.dequeue_work, and config_changed_work after flush_work() has already returned. In the virtio_gpu_probe() -> err_deinit path, drm_dev_unplug() is not called, so config_changed_work can also queue new buffers into ctrlq.vq. Since virtio_reset_device() does not cancel or flush already-scheduled work, virtio_gpu_dequeue_ctrl_func() and virtio_gpu_dequeue_cursor_func() can run concurrently with virtio_gpu_reclaim_vbufs() and del_vqs(): 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)); spin_unlock(&vgdev->ctrlq.qlock); Because virtio_gpu_reclaim_vbufs() calls virtqueue_detach_unused_buf() without holding ctrlq.qlock or cursorq.qlock while dequeue_work is calling virtqueue_get_buf(), can this corrupt virtqueue descriptor state (such as vq->split.desc_state[i].data, vq->free_head, and vq->vq.num_free), double-free vbuffers, or access the virtqueues while del_vqs() is freeing them? Additionally, flush_work(&vgdev->obj_free_work) runs before flush_work(&vgdev->ctrlq.dequeue_work), even though virtio_gpu_dequeue_ctrl_func() schedules vgdev->obj_free_work via virtio_gpu_array_put_free_delayed(). Can virtio_gpu_array_put_free_work() end up running concurrently with or after virtio_gpu_release() -> virtio_gpu_free_vbufs() and touch vgdev->vbufs while or after the kmem_cache is destroyed? > diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c > b/drivers/gpu/drm/virtio/virtgpu_vq.c > index 8181b22b9b46a..68cf7e4996884 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_vq.c > +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c > @@ -207,6 +207,21 @@ static void free_vbuf(struct virtio_gpu_device *vgdev, > kmem_cache_free(vgdev->vbufs, vbuf); > } > > +void virtio_gpu_reclaim_vbufs(struct virtio_gpu_device *vgdev) > +{ > + struct virtio_gpu_vbuffer *vbuf; > + > + while ((vbuf = virtqueue_detach_unused_buf(vgdev->ctrlq.vq))) { > + if (vbuf->objs) > + virtio_gpu_array_put_free(vbuf->objs); > + if (vbuf->resp_cb_data) > + virtio_gpu_cleanup_object(vbuf->resp_cb_data); > + free_vbuf(vgdev, vbuf); > + } > + while ((vbuf = virtqueue_detach_unused_buf(vgdev->cursorq.vq))) > + free_vbuf(vgdev, vbuf); > +} [Severity: High] Should virtio_gpu_reclaim_vbufs() also reset vgdev->pending_commands to avoid a use-after-free on vgdev->ctrlq.vq in virtio_gpu_notify()? Whenever a control buffer is queued via virtio_gpu_queue_ctrl_sgs() (or virtio_gpu_panic_queue_ctrl_sgs()), vgdev->pending_commands is incremented: ret = virtqueue_add_sgs(vq, sgs, outcnt, incnt, vbuf, GFP_ATOMIC); WARN_ON(ret); vbuf->seqno = ++vgdev->ctrlq.seqno; trace_virtio_gpu_cmd_queue(vq, virtio_gpu_vbuf_ctrl_hdr(vbuf), vbuf->seqno); atomic_inc(&vgdev->pending_commands); Several command submission paths (such as virtio_gpu_object_create(), virtio_gpu_vram_create(), virtio_gpu_mode_dumb_create(), virtio_gpu_resource_create_ioctl(), virtio_gpu_resource_create_blob_ioctl(), virtio_gpu_resource_assign_uuid(), virtio_gpu_prepare_imported_obj(), and virtio_gpu_create_context()) queue vbuffers into vgdev->ctrlq.vq without immediately calling virtio_gpu_notify(vgdev), leaving vgdev->pending_commands non-zero. When virtio_gpu_reclaim_vbufs() detaches those vbuffers, it does not reset or decrement vgdev->pending_commands, and virtio_gpu_deinit() immediately frees vgdev->ctrlq.vq via vgdev->vdev->config->del_vqs(vgdev->vdev). If an open drm_file later releases a GEM object or closes (calling virtio_gpu_gem_object_close(), virtio_gpu_free_object(), virtio_gpu_vram_free(), virtgpu_dma_buf_free_obj(), or virtio_gpu_driver_postclose() -> virtio_gpu_notify()), virtio_gpu_notify() has no drm_dev_enter() guard and only checks vgdev->pending_commands: void virtio_gpu_notify(struct virtio_gpu_device *vgdev) { bool notify; if (!atomic_read(&vgdev->pending_commands)) return; spin_lock(&vgdev->ctrlq.qlock); atomic_set(&vgdev->pending_commands, 0); notify = virtqueue_kick_prepare(vgdev->ctrlq.vq); spin_unlock(&vgdev->ctrlq.qlock); if (notify) virtqueue_notify(vgdev->ctrlq.vq); } Won't virtio_gpu_notify() see the stale non-zero vgdev->pending_commands and call virtqueue_kick_prepare() and virtqueue_notify() on the freed vgdev->ctrlq.vq? Also, if virtio_gpu_array_put_free(vbuf->objs) inside virtio_gpu_reclaim_vbufs() drops the final reference to a created GEM object, virtio_gpu_free_object() synchronously calls virtio_gpu_notify(vgdev), which would kick vgdev->ctrlq.vq after virtio_reset_device() has already reset the device while virtqueue_detach_unused_buf() is draining the ring. [Severity: High] Can calling virtio_gpu_array_put_free(vbuf->objs) from virtio_gpu_reclaim_vbufs() hang uninterruptibly in dma_fence_wait() when releasing an imported DMA-BUF GEM object? When VIRTGPU_PARAM_RESOURCE_BLOB is enabled without virgl 3D (vgdev->has_resource_blob && !vgdev->has_virgl_3d), importing a DMA-BUF via virtgpu_gem_prime_import() creates a GEM object with obj->funcs = &virtgpu_gem_dma_buf_funcs (.free = virtgpu_dma_buf_free_obj) and bo->attached = true. If that object is used on the primary plane via virtio_gpu_primary_plane_update() -> virtio_gpu_resource_flush() and the 50ms wait times out, the vbuf remains in ctrlq.vq holding a reference to the imported GEM object in vbuf->objs. During virtio_gpu_remove(), drm_dev_unplug(dev) marks the DRM device unplugged, drm_atomic_helper_shutdown(dev) drops the plane state's framebuffer reference, and virtio_gpu_deinit() resets the device and calls virtio_gpu_reclaim_vbufs(). When virtio_gpu_array_put_free(vbuf->objs) drops the last reference to the imported object, it triggers virtgpu_dma_buf_free_obj() -> virtgpu_dma_buf_unmap() -> virtio_gpu_detach_object_fenced(bo): int virtio_gpu_detach_object_fenced(struct virtio_gpu_object *bo) { struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private; struct virtio_gpu_fence *fence; if (!bo->attached) return 0; fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context, 0); if (!fence) return -ENOMEM; virtio_gpu_object_detach(vgdev, bo, fence); virtio_gpu_notify(vgdev); dma_fence_wait(&fence->f, false); dma_fence_put(&fence->f); return 0; } Inside virtio_gpu_object_detach() -> virtio_gpu_cmd_resource_detach_backing() -> virtio_gpu_queue_fenced_ctrl_buffer() -> virtio_gpu_queue_ctrl_sgs(), drm_dev_enter() returns false because the device is unplugged: if (!drm_dev_enter(vgdev->ddev, &idx)) { if (fence && vbuf->objs) virtio_gpu_array_unlock_resv(vbuf->objs); free_vbuf(vgdev, vbuf); return -ENODEV; } Since virtio_gpu_queue_ctrl_sgs() returns -ENODEV before virtio_gpu_fence_emit() is called, and virtio_gpu_object_detach() ignores this error, won't dma_fence_wait(&fence->f, false) in virtio_gpu_detach_object_fenced() sleep forever waiting on a fence that was never emitted and can never be signaled? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
