[PATCH 1/2] drm/vkms: Add drm_dev_enter/exit guards for hot-removable paths

[PATCH 1/2] drm/vkms: Add drm_dev_enter/exit guards for hot-removable paths

[Sebastian Wick]

In preparation for switching to drm_dev_unplug() for ConfigFS device
removal, add drm_dev_enter()/drm_dev_exit() guards to vkms callbacks
that can be reached while the device is being torn down.
drm_dev_unplug() relies on these SRCU guards to wait for in-flight
operations before proceeding with teardown. Without them,
synchronize_srcu() completes immediately and the teardown path can race
with in-flight operations.

Guard the following callbacks:

 - vkms_connector_detect() and vkms_conn_get_modes(): called from the
   MODE_GETCONNECTOR ioctl via fill_modes, which holds mode_config.mutex.
   Without the guard, an in-flight getconnector can hold locks that
   drm_atomic_helper_shutdown() needs.

 - vkms_wb_connector_get_modes(): same fill_modes path for the writeback
   connector.

 - vkms_set_crc_source(): called from the CRC debugfs release path. The
   debugfs proxy explicitly does not protect the release callback
   against removal races, so this needs its own guard.

Atomic commit callbacks are intentionally not guarded: they are
serialized against drm_atomic_helper_shutdown() via modeset ww_mutex
locks, and must remain functional after unplug so shutdown can disable
CRTCs.

Signed-off-by: Sebastian Wick <sebastian.wick@redhat.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c  |  8 +++++++-
 drivers/gpu/drm/vkms/vkms_connector.c | 14 +++++++++++++-
 drivers/gpu/drm/vkms/vkms_writeback.c | 13 +++++++++++--
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 0fc915a954ba..e0576edd7168 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -5,6 +5,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_blend.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_colorop.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_fixed.h>
@@ -731,11 +732,16 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 {
 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
 	bool enabled = false;
-	int ret = 0;
+	int ret, idx;
+
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return -ENODEV;
 
 	ret = vkms_crc_parse_source(src_name, &enabled);
 
 	vkms_set_composer(out, enabled);
 
+	drm_dev_exit(idx);
+
 	return ret;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_connector.c b/drivers/gpu/drm/vkms/vkms_connector.c
index b0a6b212d3f4..81cd845cf84f 100644
--- a/drivers/gpu/drm/vkms/vkms_connector.c
+++ b/drivers/gpu/drm/vkms/vkms_connector.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_probe_helper.h>
@@ -16,6 +17,10 @@ static enum drm_connector_status vkms_connector_detect(struct drm_connector *con
 	struct vkms_connector *vkms_connector;
 	enum drm_connector_status status;
 	struct vkms_config_connector *connector_cfg;
+	int idx;
+
+	if (!drm_dev_enter(dev, &idx))
+		return connector_status_disconnected;
 
 	vkms_connector = drm_connector_to_vkms_connector(connector);
 
@@ -31,6 +36,8 @@ static enum drm_connector_status vkms_connector_detect(struct drm_connector *con
 			status = vkms_config_connector_get_status(connector_cfg);
 	}
 
+	drm_dev_exit(idx);
+
 	return status;
 }
 
@@ -44,12 +51,17 @@ static const struct drm_connector_funcs vkms_connector_funcs = {
 
 static int vkms_conn_get_modes(struct drm_connector *connector)
 {
-	int count;
+	int count, idx;
+
+	if (!drm_dev_enter(connector->dev, &idx))
+		return 0;
 
 	/* Use the default modes list from DRM */
 	count = drm_add_modes_noedid(connector, XRES_MAX, YRES_MAX);
 	drm_set_preferred_mode(connector, XRES_DEF, YRES_DEF);
 
+	drm_dev_exit(idx);
+
 	return count;
 }
 
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index ecf29a2c0c8e..e779ace7b383 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -3,6 +3,7 @@
 #include <linux/iosys-map.h>
 
 #include <drm/drm_atomic.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_writeback.h>
@@ -67,9 +68,17 @@ static int vkms_wb_atomic_check(struct drm_connector *connector,
 static int vkms_wb_connector_get_modes(struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
+	int count, idx;
 
-	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
-				    dev->mode_config.max_height);
+	if (!drm_dev_enter(dev, &idx))
+		return 0;
+
+	count = drm_add_modes_noedid(connector, dev->mode_config.max_width,
+				     dev->mode_config.max_height);
+
+	drm_dev_exit(idx);
+
+	return count;
 }
 
 static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
-- 
2.56.0

[PATCH 2/2] drm/vkms: Use drm_dev_unplug() for ConfigFS device removal

[Sebastian Wick]

VKMS ConfigFS devices can be disabled while userspace still holds open
file descriptors. Using drm_dev_unregister() leaves the device in a
state where ioctls are still accepted but internal locks can contend
with the teardown path. Specifically, drmModeGetConnector() triggers
fill_modes which acquires mode_config locks that contend with
drm_atomic_helper_shutdown().

Use drm_dev_unplug() instead, which marks the device as unplugged so
the ioctl layer rejects subsequent calls with -ENODEV. The previous
commit added drm_dev_enter()/drm_dev_exit() guards to vkms callbacks,
so synchronize_srcu() in drm_dev_unplug() properly waits for any
in-flight operations before the teardown proceeds.

Signed-off-by: Sebastian Wick <sebastian.wick@redhat.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 5a640b531d88..716f9c966c21 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -259,7 +259,7 @@ void vkms_destroy(struct vkms_config *config)
 
 	fdev = config->dev->faux_dev;
 
-	drm_dev_unregister(&config->dev->drm);
+	drm_dev_unplug(&config->dev->drm);
 	drm_atomic_helper_shutdown(&config->dev->drm);
 	devres_release_group(&fdev->dev, NULL);
 	faux_device_destroy(fdev);
-- 
2.56.0

Re: [PATCH 2/2] drm/vkms: Use drm_dev_unplug() for ConfigFS device removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/vkms: broken CRC debugfs unplug handling in vkms_destroy()
--

commit 58c6203197ea5aa86a82e01783503c18d44dd2a1
Author: Sebastian Wick <sebastian.wick@redhat.com>

drm/vkms: Use drm_dev_unplug() for ConfigFS device removal

This commit switches vkms_destroy() from drm_dev_unregister() to
drm_dev_unplug() so that ioctls on open file descriptors are rejected with
-ENODEV and in-flight operations guarded by drm_dev_enter()/drm_dev_exit()
complete before ConfigFS device removal proceeds.

> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 5a640b531d88f..716f9c966c211 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -259,7 +259,7 @@ void vkms_destroy(struct vkms_config *config)
>  
>  	fdev = config->dev->faux_dev;
>  
> -	drm_dev_unregister(&config->dev->drm);
> +	drm_dev_unplug(&config->dev->drm);
>  	drm_atomic_helper_shutdown(&config->dev->drm);
>  	devres_release_group(&fdev->dev, NULL);

[Severity: High]
How does this interact with an open CRTC CRC debugfs file descriptor
(/sys/kernel/debug/dri/<N>/crtc-<M>/crc/data) when the VKMS device is
disabled via ConfigFS?

In drivers/gpu/drm/vkms/vkms_composer.c, vkms_set_crc_source() guards the
callback with drm_dev_enter():

drivers/gpu/drm/vkms/vkms_composer.c:vkms_set_crc_source() {
	...
	if (!drm_dev_enter(crtc->dev, &idx))
		return -ENODEV;

	ret = vkms_crc_parse_source(src_name, &enabled);

	vkms_set_composer(out, enabled);

	drm_dev_exit(idx);
	return ret;
}

When the crc/data file descriptor is closed, full_proxy_release() invokes
crtc_crc_release() in drivers/gpu/drm/drm_debugfs_crc.c, which calls
crtc->funcs->set_crc_source(crtc, NULL).

Two issues seem possible here:

1. Unlike /dev/dri/card<N> file descriptors, opening crc/data via
crtc_crc_open() does not acquire a drm_dev_get() reference on crtc->dev,
and debugfs_remove_recursive() during drm_dev_unregister() does not wait
for open file descriptors to close.

In vkms_destroy(), drm_dev_unplug() runs synchronize_srcu() before
devres_release_group(&fdev->dev, NULL) drops the devm reference via
devm_drm_dev_init_release() -> drm_dev_put(). If no /dev/dri/card<N> fd is
open (or after it is closed first, such as during process exit when card<N>
has a lower fd number than crc/data), drm_dev_release() frees struct
vkms_output (embedding struct drm_crtc) and struct vkms_device (embedding
struct drm_device). When the crc/data fd is subsequently closed, wouldn't
crtc_crc_release() and drm_dev_enter(crtc->dev, &idx) in
vkms_set_crc_source() dereference the freed struct drm_crtc and
crtc->dev?

2. Conversely, if struct drm_device is still kept alive by an open
/dev/dri/card<N> fd when crc/data is closed after drm_dev_unplug() sets
dev->unplugged = true, drm_dev_enter(crtc->dev, &idx) in
vkms_set_crc_source(crtc, NULL) will return false and exit early with
-ENODEV. Doesn't that skip vkms_set_composer(out, false), leaving
out->composer_enabled set to true and leaking the vblank reference from
drm_crtc_vblank_put(&out->crtc)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031225901.3200167-1-sebastian.wick@redhat.com?part=2