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) <[email protected]> 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/[email protected]?part=1
