Re: [PATCH v6 10/10] drm/i915/display: use drm backlight

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Calling kobject_uevent_env() (which can sleep) while holding the drm_backlight_lock spinlock in drm_backlight_link().
- [High] Error paths in drm_connector_register() fail to unregister the backlight device, leading to a freed pointer on the global backlight list.
--

commit 91fa46bfbc0b179001176c9331c615c49eadeb08
Author: Mario Limonciello (AMD) <superm1@kernel.org>

drm/i915/display: use drm backlight

Convert Intel display drivers (i915 and xe) to use the new DRM backlight
infrastructure. This allows brightness control via DRM connector
properties.

> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index b128896cb1c2d..4543c2a8f3568 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
[ ... ]
> @@ -1004,6 +1005,8 @@ int intel_backlight_device_register(struct intel_connector *connector)
>  		    "[CONNECTOR:%d:%s] backlight device %s registered\n",
>  		    connector->base.base.id, connector->base.name, name);
>  
> +	drm_backlight_link(connector->base.backlight, bd);
> +

[Severity: High]
This is a pre-existing issue, but does calling drm_backlight_link() here
introduce a sleep-in-atomic bug?

Looking at drm_backlight_link() in drm_backlight.c, it uses a scope-based
spinlock:

	guard(spinlock)(&drm_backlight_lock);
	send_uevent = __drm_backlight_link(b, bd);

	if (send_uevent)
		kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE,
				   (char **)ep);

kobject_uevent_env() allocates memory with GFP_KERNEL and can sleep. Since
the spinlock guard is still active when kobject_uevent_env() is called, this
could trigger lockdep warnings or system crashes.

>  out:
>  	kfree(name);
[ ... ]
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0ce0c09835f6d..cd35cce329a55 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
[ ... ]
> @@ -7421,6 +7422,13 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
>  				    type, &intel_dp->aux.ddc);
>  	drm_connector_helper_add(&connector->base, &intel_dp_connector_helper_funcs);
>  
> +	if (type == DRM_MODE_CONNECTOR_eDP) {
> +		int r = drm_backlight_alloc(&connector->base);
> +

[Severity: High]
This is a pre-existing issue, but does allocating the backlight here expose a
list corruption issue during connector registration failures?

If a failure occurs during drm_connector_register() (for example, in
drm_sysfs_connector_add()), the error path does not unregister the
backlight device.

Since it was never fully registered, drm_connector_cleanup() will eventually
skip calling drm_connector_unregister() but will directly call
drm_backlight_free(), which does:

	kfree(b);
	connector->backlight = NULL;

Because the backlight was never removed from the global drm_backlight_list,
freeing it here leaves a dangling pointer on the global list, which could
cause a use-after-free panic during later list traversals.

> +		if (r)
> +			drm_err(display->drm, "Failed to allocate backlight: %d\n", r);
> +	}
> +
>  	if (!HAS_GMCH(display) && DISPLAY_VER(display) < 12)
>  		connector->base.interlace_allowed = true;
>

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