Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Connector hotplugging or unplugging while DRM_CLIENT_CAP_LUMINANCE is active causes state desynchronization and triggers kernel warnings. - [High] Concurrent calls to DRM_IOCTL_SET_CLIENT_CAP can leak `luminance_clients` reference counts, permanently inhibiting legacy sysfs backlight control (DoS). --
commit d72da18676683e0c9579165708469f0cf974aa5b Author: Mario Limonciello <[email protected]> drm: add DRM_CLIENT_CAP_LUMINANCE This commit adds a DRIVER_CONNECTOR_LUMINANCE feature and a DRM_CLIENT_CAP_LUMINANCE client capability. When a client enables this capability, the kernel inhibits legacy sysfs backlight control by taking over the backlight devices. > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > --- a/drivers/gpu/drm/drm_file.c > +++ b/drivers/gpu/drm/drm_file.c [ ... ] > @@ -252,6 +253,10 @@ void drm_file_free(struct drm_file *file) > if (drm_core_check_feature(dev, DRIVER_MODESET)) { > drm_fb_release(file); > drm_property_destroy_user_blobs(dev, file); > + if (file->supports_luminance_control) { > + drm_backlight_uninhibit_legacy_all(dev); > + file->supports_luminance_control = false; > + } > } [Severity: High] Will this correctly handle connectors that are hotplugged or unplugged after the compositor sets DRM_CLIENT_CAP_LUMINANCE? drm_backlight_uninhibit_legacy_all() iterates over the list of currently present connectors. If a compositor enables DRM_CLIENT_CAP_LUMINANCE and a monitor is later hotplugged via DP MST, the new connector will not have its legacy sysfs backlight control inhibited. When the compositor exits and drm_file_free() runs: For newly hotplugged connectors, it will try to uninhibit them even though they were never inhibited, triggering WARN_ON(b->luminance_clients == 0) in drm_backlight_uninhibit_legacy(). Conversely, for connectors unplugged before the compositor exits, drm_backlight_connector_cleanup() will fire WARN_ON(b->luminance_clients) because the connector is destroyed while its luminance_clients count is still 1. > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c [ ... ] > @@ -392,6 +394,21 @@ drm_setclientcap(struct drm_device *dev, void *data, > struct drm_file *file_priv) > file_priv->plane_color_pipeline = req->value; > break; > } > + case DRM_CLIENT_CAP_LUMINANCE: > + if (!drm_core_check_feature(dev, DRIVER_CONNECTOR_LUMINANCE)) > + return -EOPNOTSUPP; > + if (!file_priv->atomic) > + return -EINVAL; > + if (req->value > 1) > + return -EINVAL; > + if (req->value == file_priv->supports_luminance_control) > + break; > + if (req->value) > + drm_backlight_inhibit_legacy_all(dev); > + else > + drm_backlight_uninhibit_legacy_all(dev); > + file_priv->supports_luminance_control = req->value; > + break; [Severity: High] Does this check-then-act sequence lack synchronization? If two threads share a single DRM file descriptor and concurrently invoke DRM_IOCTL_SET_CLIENT_CAP with DRM_CLIENT_CAP_LUMINANCE=1, both threads could evaluate the supports_luminance_control condition as false before either updates it. Both threads would then call drm_backlight_inhibit_legacy_all(dev), causing a double-increment of the luminance_clients reference count. When the file descriptor is closed, drm_file_free() will only call drm_backlight_uninhibit_legacy_all() once, permanently leaving the legacy sysfs backlight interface inhibited and returning -EBUSY. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
