Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider: - [Critical] Concurrent backlight unlinking and connector teardown causes a Use-After-Free due to missing references and early return bypassing `cancel_work_sync`. - [High] Turning off the backlight asynchronously during pipeline disable breaks hardware shutdown sequencing, risking screen corruption. - [High] Backlight hardware state is mutated asynchronously before atomic commit validation, failing to roll back on commit failure. - [High] Lockless, in-place modification of `connector->state->luminance` from asynchronous notifier context causes Write-After-Free. - [High] Lockless read of `crtc->state` in `drm_atomic_helper_connector_apply_luminance` causes a Use-After-Free. - [High] Lockless read of `connector->state` of unrelated connectors in `drm_atomic_crtc_set_backlight` causes a Use-After-Free. - [Medium] Concurrency between `drm_backlight_inhibit_legacy_all` and `drm_backlight_link` leads to double-counted or overwritten `luminance_clients`. - [Medium] Triggering a `WARN_ON` for routine hardware communication failures allows trivial kernel log spam and potential system panics. - [Medium] Legacy DPMS changes incorrectly dim all connectors on shared CRTCs, and conflict with the atomic commit tail logic. - [Medium] Backlight is enabled on disconnected/un-driven panels during DPMS ON. - [Low] The fallback brightness assignment in `drm_backlight_bd_set_luminance` is mathematically guaranteed to fail due to a hardcoded `U16_MAX`. - [Low] Property bounds are not updated when dynamically linking a backend with a different brightness range. - [Low] Broken teardown symmetry leaves the backlight sysfs symlink unremoved, resulting in dead code. --
commit 4014f9d380d34194182229a03855db38075d5806 Author: Mario Limonciello <[email protected]> Subject: drm: add connector backlight (LUMINANCE) infrastructure This commit introduces a per-connector LUMINANCE range property to control backlight brightness through the DRM atomic modeset path. It embeds a backlight state in each connector and synchronizes brightness changes asynchronously via a workqueue. > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index 285aac3554dfd..110cd81e5d6db 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c [ ... ] > @@ -1230,6 +1231,10 @@ drm_atomic_helper_commit_encoder_bridge_disable(struct > drm_device *dev, > * it away), so we won't call disable hooks twice. > */ > bridge = drm_bridge_chain_get_first_bridge(encoder); > + > + /* Turn the backlight off before disabling the pipeline. */ > + drm_backlight_set_luminance(connector, 0); > + > drm_atomic_bridge_chain_disable(bridge, state); > drm_bridge_put(bridge); [Severity: High] Does this sequence break the display hardware shutdown requirements? By queuing the backlight off operation asynchronously via drm_backlight_set_luminance() and immediately disabling the bridge chain, the pipeline might be shut down before the backlight actually turns off. Could this lead to screen corruption or panel damage? [ ... ] > @@ -1775,6 +1780,24 @@ void drm_atomic_helper_commit_modeset_enables(struct > drm_device *dev, > } > EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables); > > +/** > + * drm_atomic_helper_connector_apply_luminance - apply connector luminance > from atomic state > + * @conn_state: atomic connector state to apply luminance for > + * > + * Updates the backlight luminance from the atomic connector state. If the > + * connector has a linked backlight and is associated with an active CRTC, > + * push the luminance value to hardware. > + */ > +void drm_atomic_helper_connector_apply_luminance(const struct > drm_connector_state *conn_state) > +{ > + struct drm_connector *connector = conn_state->connector; > + > + if (conn_state->crtc && conn_state->crtc->state && > + conn_state->crtc->state->active) [Severity: High] Can a concurrent CRTC modeset free the CRTC state while it is being accessed here? Since the CRTC lock might not be held during pure connector property commits, reading conn_state->crtc->state->active could result in a use-after-free if the CRTC state is swapped and freed simultaneously. > + drm_backlight_set_luminance(connector, conn_state->luminance); > +} > +EXPORT_SYMBOL(drm_atomic_helper_connector_apply_luminance); > + [ ... ] > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c > b/drivers/gpu/drm/drm_atomic_uapi.c > index 5ea593b3a98ec..07d4bcb16cd23 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c [ ... ] > @@ -1133,6 +1144,22 @@ static struct drm_pending_vblank_event > *create_vblank_event( > return e; > } > > +static void drm_atomic_crtc_set_backlight(struct drm_crtc *crtc, bool active) > +{ > + struct drm_connector_list_iter conn_iter; > + struct drm_connector *connector; > + > + drm_connector_list_iter_begin(crtc->dev, &conn_iter); > + drm_for_each_connector_iter(connector, &conn_iter) { > + if (!connector->state || connector->state->crtc != crtc) > + continue; [Severity: High] Could this lockless read of connector->state trigger a use-after-free? This function iterates all connectors and reads their states while holding only the connection_mutex. If another connector receives a concurrent property update, its state could be swapped and freed while being read here. > + > + drm_backlight_set_luminance(connector, > + active ? > connector->state->luminance : 0); > + } > + drm_connector_list_iter_end(&conn_iter); > +} > + > int drm_atomic_connector_commit_dpms(struct drm_atomic_commit *state, > struct drm_connector *connector, > int mode) > @@ -1155,9 +1182,29 @@ int drm_atomic_connector_commit_dpms(struct > drm_atomic_commit *state, > if (connector->dpms == mode) > goto out; > > + crtc = connector->state ? connector->state->crtc : NULL; > + > + /* Handle backlight brightness coordination with DPMS state changes */ > + if (old_mode != DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_OFF) { > + /* DPMS ON -> OFF: dim all connectors driven by this CRTC. */ > + if (crtc) > + drm_atomic_crtc_set_backlight(crtc, false); [Severity: Medium] Will this cause unintended dimming or flickering on cloned displays? When one connector on a shared CRTC transitions to DPMS OFF, calling drm_atomic_crtc_set_backlight() queues an OFF request for all connectors driven by that CRTC, even if the other connectors are still supposed to remain active. > + else > + drm_backlight_set_luminance(connector, 0); > + } > + > connector->dpms = mode; > > - crtc = connector->state->crtc; > + /* DPMS OFF -> ON: restore brightness to property value */ > + if (old_mode == DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_ON && > + connector->state) { > + if (crtc) > + drm_atomic_crtc_set_backlight(crtc, true); > + else > + drm_backlight_set_luminance(connector, > + > connector->state->luminance); [Severity: Medium] Is it safe to turn on the backlight for a disconnected panel? If a connector is turned on (DPMS ON) but has no active CRTC (crtc is NULL), this branch restores the committed luminance. Illuminating a panel that isn't receiving a video signal could violate hardware sequencing constraints and stress the panel. [Severity: High] Does this rollback path properly revert the physical backlight changes? The physical backlight state is mutated asynchronously before calling drm_atomic_commit(state). If the commit fails (for example, due to validation failure), the software DPMS state is rolled back at the out label: out: if (ret != 0) connector->dpms = old_mode; However, the physical backlight might remain turned off since the asynchronous work was already dispatched and cannot be rolled back here. > + } > + > if (!crtc) > goto out; > ret = drm_atomic_add_affected_connectors(state, crtc); [ ... ] > diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c > new file mode 100644 > index 0000000000000..dd02d1878727e > --- /dev/null > +++ b/drivers/gpu/drm/drm_backlight.c [ ... ] > +static void __drm_backlight_worker(struct work_struct *w) > +{ > + struct drm_backlight *b = container_of(w, struct drm_backlight, work); [ ... ] > + /* > + * The backend stays valid here: drm_backlight_do_unlink() clears > + * @backend and drops its reference only after cancel_work_sync(), so an > + * in-flight worker always sees a live backend. > + */ > + if (funcs && funcs->set_luminance) > + WARN_ON(funcs->set_luminance(b, v)); [Severity: Medium] Could this WARN_ON() be triggered by routine hardware communication failures? If the backend communicates over I2C or similar buses, transient timeouts or disconnections might routinely return error codes. Using WARN_ON() could allow local userspace to spam the kernel log or cause a system panic if panic_on_warn is enabled. [ ... ] > +/* caller must hold @drm_backlight_lock */ > +static void __drm_backlight_readback(struct drm_backlight *b, unsigned int v) > +{ [ ... ] > + /* > + * Reflect a hardware-side brightness change (firmware hotkeys, or a > + * legacy sysfs write while not inhibited) back into the connector's > + * committed luminance so a read-back returns the real value. > + */ > + if (connector->state) > + connector->state->luminance = min(v, max); [Severity: High] Can this in-place modification of connector->state cause a write-after-free? Since this function is called asynchronously from a sysfs handler holding only drm_backlight_lock, a concurrent atomic commit could duplicate, commit, and free the connector->state object, causing this write to corrupt memory. > +} [ ... ] > +static void drm_backlight_do_unlink(struct drm_backlight *b) > +{ > + struct backlight_device *bd; > + unsigned int clients; > + > + /* > + * Stop new work first, but leave @backend in place so an in-flight > + * worker keeps a valid backend to operate on. Capture the linked > + * backlight_device (if any) while @funcs is still set. > + */ > + scoped_guard(spinlock, &drm_backlight_lock) { > + if (!b->funcs) > + return; [Severity: Critical] Does this early return leak a pending or running worker? If b->funcs is NULL, the function returns immediately without calling cancel_work_sync(&b->work). This could leave the worker operating on memory that might be subsequently freed. > + bd = drm_backlight_bd(b); > + clients = b->luminance_clients; > + b->funcs = NULL; > + list_del_init(&b->list); > + } [ ... ] > +int drm_backlight_link(struct drm_connector *connector, > + struct backlight_device *bd) > +{ [ ... ] > + max = bd->props.max_brightness; > + if (max && !connector->luminance_property) { > + ret = drm_backlight_create_property(connector, max, false); [Severity: Low] Will the LUMINANCE property bounds remain stale if a new backend is linked? If a backend is unlinked and a new one is linked with a different max_brightness range, the property is not recreated because connector->luminance_property already exists. This might artificially clamp the new backend to the old backend's maximum brightness. > + if (ret) > + return ret; > + } > + > + scoped_guard(spinlock, &drm_backlight_lock) { > + b->funcs = &drm_backlight_bd_funcs; > + b->backend = bd; > + backlight_device_ref(bd); > + list_add(&b->list, &drm_backlight_list); > + /* > + * Inherit the device-wide inhibit level so a connector linked > + * (or hotplugged) while luminance-aware clients are active also > + * has its legacy sysfs control inhibited. > + */ > + b->luminance_clients = > connector->dev->mode_config.luminance_clients; [Severity: Medium] Is there a race condition here that could double-count luminance_clients? If a new connector is linked concurrently with drm_backlight_inhibit_legacy_all(), it might inherit the already-incremented global dev->mode_config.luminance_clients count here. > + if (b->luminance_clients) > + atomic_add(b->luminance_clients, &bd->drm_takeover); [ ... ] > +void drm_backlight_inhibit_legacy_all(struct drm_device *dev) > +{ > + struct drm_connector_list_iter iter; > + struct drm_connector *connector; > + > + scoped_guard(spinlock, &drm_backlight_lock) > + dev->mode_config.luminance_clients++; > + > + drm_connector_list_iter_begin(dev, &iter); > + drm_for_each_connector_iter(connector, &iter) > + drm_backlight_inhibit_legacy(connector); [Severity: Medium] Since the lock is dropped between incrementing the global luminance_clients count and iterating the connectors, the iterator could hit the newly linked connector and increment its count a second time. This double-count would prevent the legacy sysfs interface from ever being uninhibited. > + drm_connector_list_iter_end(&iter); > +} [ ... ] > +static int drm_backlight_bd_set_luminance(struct drm_backlight *b, > + unsigned int value) > +{ > + struct backlight_device *bd = b->backend; > + int rc; > + > + rc = backlight_set_brightness(bd, value, BACKLIGHT_UPDATE_DRM); > + if (rc) > + backlight_set_brightness(bd, U16_MAX, BACKLIGHT_UPDATE_DRM); [Severity: Low] Is this fallback brightness assignment guaranteed to fail? Because backlight_set_brightness() strictly bounds-checks the requested value against bd->props.max_brightness (which is typically around 255), passing U16_MAX (65535) will mathematically guarantee an -EINVAL failure, making this error recovery path ineffective. > + > + return rc; > +} [ ... ] > +static int drm_backlight_notify(struct notifier_block *self, > + unsigned long event, void *data) > +{ [ ... ] > + for (;;) { > + struct drm_backlight *found = NULL; > + > + scoped_guard(spinlock, &drm_backlight_lock) { > + list_for_each_entry(b, &drm_backlight_list, > list) { > + if (drm_backlight_bd(b) == bd) { > + found = b; > + break; > + } > + } > + } > + if (!found) > + break; > + drm_backlight_do_unlink(found); [Severity: Critical] Is there a risk of the connector being freed concurrently here? The notifier iterates the list under the lock, finds a match, drops the lock, and then calls drm_backlight_do_unlink(found). Without holding a reference to the connector, a concurrent teardown (like a hardware hotplug) could free the connector memory, resulting in a use-after-free when drm_backlight_do_unlink accesses found->funcs. > + } > + break; [ ... ] > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 8b4baed060f3a..92ed0c1879d53 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c [ ... ] > @@ -944,6 +948,8 @@ EXPORT_SYMBOL(drm_connector_dynamic_register); > void drm_connector_unregister(struct drm_connector *connector) > { > mutex_lock(&connector->mutex); > + drm_backlight_unregister(connector); > + > if (connector->registration_state != DRM_CONNECTOR_REGISTERED) { [Severity: Low] Does unregistering the backlight here break the sysfs teardown symmetry? Because this clears the backend link before drm_sysfs_connector_remove_early() is called, the latter will fail to fetch the backlight device and miss calling sysfs_remove_link(&connector->kdev->kobj, "backlight"). [ ... ] > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index ef4e923a87284..2f4ce7d040f9d 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c [ ... ] > void drm_sysfs_connector_remove_early(struct drm_connector *connector) > { > + struct backlight_device *bd = drm_backlight_get_device(connector); > + > + if (bd) { > + sysfs_remove_link(&connector->kdev->kobj, "backlight"); [Severity: Low] Because drm_backlight_unregister() has already been called in drm_connector_unregister(), drm_backlight_get_device() will return NULL, leaving this cleanup path unreachable. > + backlight_device_unref(bd); > + } > + > if (connector->ddc) > sysfs_remove_link(&connector->kdev->kobj, "ddc"); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
