Re: [PATCH] drm/virtio: Destroy obj_restore_lock on the final drm_dev_put()

[Abhin Parekadan Jose <abhinjoses@gmail.com>]

On Thu, Oct 15, 2026 at 12:03:32AM +0300, Dmitry Osipenko wrote:
> On 10/14/26 22:53, Abhin Parekadan Jose wrote:
> > On Wed, Oct 14, 2026 at 01:16:57AM +0300, Dmitry Osipenko wrote:
> >> On 10/12/26 08:44, Abhin Parekadan Jose wrote:
> >>> Gentle ping on this fix for the syzbot report.
> >>>
> >>> Dmitry, Gerd, do you have a preference between moving mutex_destroy()
> >>> to virtio_gpu_release() (this patch) and using drmm_mutex_init()? I can
> >>> send a v2 with the other option if you prefer it.
> >>>
> >>> Thanks,
> >>> Abhin
> >>>
> >>> On Tue, Sep 29, 2026 at 07:07:14AM +0000, Abhin Parekadan Jose wrote:
> >> Calling virtio_gpu_deinit() from virtio_gpu_release() might be a better
> >> option
> > 
> > Wouldn't that end up freeing the virtqueues twice, or using them after
> > they are freed? virtio_gpu_deinit() would then run from both the
> > virtio_gpu_remove() path and the virtio_gpu_release() path.
> > or do you mean removing it from virtio_gpu_remove() and calling it only
> > from virtio_gpu_release()?
> > This is only from a high-level look so far. I'll look into it more
> > closely and get back to you.
> 
> Remove from virtio_gpu_remove() and call only from virtio_gpu_release().

I tried this out and here are my findings.

That would undo b1df3a2b24a9 ("drm/virtio: add drm_driver.release
callback."), which split virtio_gpu_deinit() on purpose: "move the drm
shutdown and release to virtio_gpu_release() ... use
drm_dev_{enter,exit,unplug} to avoid touching hardware after device
removal". That matches what the documentation asks for. drm_dev_unplug()
exists so the device can be unplugged "while there are still open users
of @dev", and Documentation/gpu/drm-uapi.rst (Device Hot-Unplug) says
the DRM instance is torn down only after userspace has closed all
descriptors and mmaps. Meanwhile the virtio side has to be gone when
->remove() returns: the example in
Documentation/driver-api/virtio/writing_virtio_drivers.rst resets the
device and deletes the vqs in remove(), and virtio_dev_remove() warns
("Driver should have reset device") if the driver did not.

So the virtio teardown (reset, flushing the vq workers, del_vqs) has to
stay in remove(), and everything that open files and GEM objects can
still reach has to wait for release(). I tried moving the deinit call
into release() to be sure: the virtio core warns that the device was
not reset and that the vq MSI vectors are still allocated at PCI
removal, and the deferred reset then runs on a virtio_device that
virtio_pci_remove() already freed (KASAN use-after-free in
virtio_reset_device()). It only makes things more complicated.

obj_restore_lock is on the wrong side of that split. It is only used
from GEM paths (the ->free callbacks via
virtio_gpu_remove_from_restore_list(), object creation, and the PM
freeze/restore ops), and the free callbacks can run after unplug, like
the vbuf cache and host_visible_mm that release() already handles. So
the mutex belongs in virtio_gpu_release(), which is what this patch
does.

It only looks like the mutex_destroy(...) landed in the wrong function
virtio_gpu_deinit() instead of virtio_gpu_release() in 54a970048296.

Is there something I am missing in the framework, or have I understood
it wrong?

Regards,
Abhin