Re: [PATCH v6 01/10] Revert "backlight: Remove notifier"

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Low] Multiple copy-paste errors and typos in the newly added kerneldoc comments for the backlight notifier API.
- [Low] Missing include or forward declaration for `struct notifier_block` in `<linux/backlight.h>`.

Pre-existing issues:
- [High] Concurrent `backlight_update_status()` and `backlight_device_unregister()` can cause a Use-After-Free of `bd->ops` or execution of unmapped memory.
- [High] `backlight_device_get_by_type` returns a pointer to a `backlight_device` without incrementing its reference count, leading to a Use-After-Free.
- [High] `backlight_enable()` and `backlight_disable()` modify `bd->props.state` locklessly, causing data races and state corruption.
--

commit 692a77b3d3423be93e071e88f6a741af5c0ebe7d
Author: Mario Limonciello (AMD) <superm1@kernel.org>
Subject: Revert "backlight: Remove notifier"

This reverts a previous commit to restore backlight notifier support.
The backlight notifier support is needed in order to add backlight
control support into DRM connectors.

> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index a22d0bbb6e639..ff2c2084c73a4 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
[ ... ]
> @@ -484,6 +488,9 @@ void backlight_device_unregister(struct backlight_device *bd)
>  	mutex_unlock(&pmac_backlight_mutex);
>  #endif
>  
> +	blocking_notifier_call_chain(&backlight_notifier,
> +				     BACKLIGHT_UNREGISTERED, bd);
> +
>  	mutex_lock(&bd->ops_lock);
>  	bd->ops = NULL;
>  	mutex_unlock(&bd->ops_lock);

[Severity: High]
This is a pre-existing issue, but can the lockless check in
backlight_update_status() race with this teardown?

If a panel driver concurrently calls backlight_update_status(), which checks
bd->ops under the update_lock but without the ops_lock, and this function
sets bd->ops to NULL under the ops_lock:

include/linux/backlight.h:backlight_update_status() {
    ...
    mutex_lock(&bd->update_lock);
    if (bd->ops && bd->ops->update_status)
        ret = bd->ops->update_status(bd);
    ...
}

Could it lead to a use-after-free or executing unmapped module memory if the
driver is unbound right after the check passes?

[ ... ]
> +/**
> + * backlight_unregister_notifier - unregister a backlight notifier
> + * @nb: notifier block to unregister
> + *
> + * Register a notifier to get notified when backlight devices get registered
> + * or unregistered.

[Severity: Low]
This kerneldoc description appears to be copied directly from
backlight_register_notifier().

Should this be updated to describe unregistering a notifier instead?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
use-after-free in backlight_device_get_by_type() in the same file.

drivers/video/backlight/backlight.c:backlight_device_get_by_type() {
    ...
    mutex_unlock(&backlight_dev_list_mutex);
    return found ? bd : NULL;
}

This function returns a matched backlight_device pointer without taking a
reference count via get_device(). If the backlight device is concurrently
unregistered, the device could be freed, leaving the caller with a dangling
pointer.

Should this function increment the reference count before returning?

> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index f29a9ef1052e7..d905173c7f73c 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
[ ... ]
> +enum backlight_notification {
> +	/**
> +	 * @BACKLIGHT_REGISTERED: The backlight device is registered.
> +	 */
> +	BACKLIGHT_REGISTERED,
> +
> +	/**
> +	 * @BACKLIGHT_UNREGISTERED: The backlight revice is unregistered.
> +	 */

[Severity: Low]
Is the word revice a typo for device here?

> +	BACKLIGHT_UNREGISTERED,
> +};
[ ... ]
>  void backlight_force_update(struct backlight_device *bd,
>  			    enum backlight_update_reason reason);
> +int backlight_register_notifier(struct notifier_block *nb);
> +int backlight_unregister_notifier(struct notifier_block *nb);
>  struct backlight_device *backlight_device_get_by_name(const char *name);

[Severity: Low]
Does this header need a forward declaration of struct notifier_block, or an
include of linux/notifier.h?

Relying on implicit includes could cause build failures in downstream modules
that include this header.

[Severity: High]
This is a pre-existing issue, but there appears to be a potential data race
regarding state in backlight_enable() and backlight_disable().

include/linux/backlight.h:backlight_enable() {
    ...
    bd->props.power = BACKLIGHT_POWER_ON;
    bd->props.state &= ~BL_CORE_FBBLANK;
    ...
}

These functions modify the state locklessly. Since other backlight
functions like backlight_suspend() modify the same state while holding
the ops_lock, could a concurrent call to backlight_enable() silently
overwrite and lose bits like BL_CORE_SUSPENDED?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624165751.2014759-1-mario.limonciello@amd.com?part=1