[PATCH 6.12 102/242] Revert "drm: Fix use-after-free on framebuffers and property blobs when calling drm_dev_unplug"

Greg Kroah-Hartman
2026-04-08T18:43:43Z

From: Maarten Lankhorst <dev@lankhorst.se>

commit 45ebe43ea00d6b9f5b3e0db9c35b8ca2a96b7e70 upstream.

This reverts commit 6bee098b91417654703e17eb5c1822c6dfd0c01d.

Den 2026-03-25 kl. 22:11, skrev Simona Vetter:
> On Wed, Mar 25, 2026 at 10:26:40AM -0700, Guenter Roeck wrote:
>> Hi,
>>
>> On Fri, Mar 13, 2026 at 04:17:27PM +0100, Maarten Lankhorst wrote:
>>> When trying to do a rather aggressive test of igt's "xe_module_load
>>> --r reload" with a full desktop environment and game running I noticed
>>> a few OOPSes when dereferencing freed pointers, related to
>>> framebuffers and property blobs after the compositor exits.
>>>
>>> Solve this by guarding the freeing in drm_file with drm_dev_enter/exit,
>>> and immediately put the references from struct drm_file objects during
>>> drm_dev_unplug().
>>>
>>
>> With this patch in v6.18.20, I get the warning backtraces below.
>> The backtraces are gone with the patch reverted.
>
> Yeah, this needs to be reverted, reasoning below. Maarten, can you please
> take care of that and feed the revert through the usual channels? I don't
> think it's critical enough that we need to fast-track this into drm.git
> directly.
>
> Quoting the patch here again:
>
>> drivers/gpu/drm/drm_file.c | 5 ++++-
>> drivers/gpu/drm/drm_mode_config.c | 9 ++++++---
>> 2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index ec820686b3021..f52141f842a1f 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -233,6 +233,7 @@ static void drm_events_release(struct drm_file *file_priv)
>> void drm_file_free(struct drm_file *file)
>> {
>> struct drm_device *dev;
>> + int idx;
>>
>> if (!file)
>> return;
>> @@ -249,9 +250,11 @@ void drm_file_free(struct drm_file *file)
>>
>> drm_events_release(file);
>>
>> - if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>> + if (drm_core_check_feature(dev, DRIVER_MODESET) &&
>> + drm_dev_enter(dev, &idx)) {
>
> This is misplaced for two reasons:
>
> - Even if we'd want to guarantee that we hold a drm_dev_enter/exit
> reference during framebuffer teardown, we'd need to do this
> _consistently over all callsites. Not ad-hoc in just one place that a
> testcase hits. This also means kerneldoc updates of the relevant hooks
> and at least a bunch of acks from other driver people to document the
> consensus.
>
> - More importantly, this is driver responsibilities in general unless we
> have extremely good reasons to the contrary. Which means this must be
> placed in xe.
>
>> drm_fb_release(file);
>> drm_property_destroy_user_blobs(dev, file);
>> + drm_dev_exit(idx);
>> }
>>
>> if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>> index 84ae8a23a3678..e349418978f79 100644
>> --- a/drivers/gpu/drm/drm_mode_config.c
>> +++ b/drivers/gpu/drm/drm_mode_config.c
>> @@ -583,10 +583,13 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>> */
>> WARN_ON(!list_empty(&dev->mode_config.fb_list));
>> list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
>> - struct drm_printer p = drm_dbg_printer(dev, DRM_UT_KMS, "[leaked fb]");
>> + if (list_empty(&fb->filp_head) || drm_framebuffer_read_refcount(fb) > 1) {
>> + struct drm_printer p = drm_dbg_printer(dev, DRM_UT_KMS, "[leaked fb]");
>
> This is also wrong:
>
> - Firstly, it's a completely independent bug, we do not smash two bugfixes
> into one patch.
>
> - Secondly, it's again a driver bug: drm_mode_cleanup must be called when
> the last drm_device reference disappears (hence the existence of
> drmm_mode_config_init), not when the driver gets unbound. The fact that
> this shows up in a callchain from a devres cleanup means the intel
> driver gets this wrong (like almost everyone else because historically
> we didn't know better).
>
> If we don't follow this rule, then we get races with this code here
> running concurrently with drm_file fb cleanups, which just does not
> work. Review pointed that out, but then shrugged it off with a confused
> explanation:
>
> https://lore.kernel.org/all/e61e64c796ccfb17ae673331a3df4b877bf42d82.camel@linux.intel.com/
>
> Yes this also means a lot of the other drm_device teardown that drivers
> do happens way too early. There is a massive can of worms here of a
> magnitude that most likely is much, much bigger than what you can
> backport to stable kernels. Hotunplug is _hard_.

Back to the drawing board, and fixing it in the intel display driver
instead.

Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Fixes: 6bee098b9141 ("drm: Fix use-after-free on framebuffers and property blobs when calling drm_dev_unplug")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Simona Vetter <simona.vetter@ffwll.ch>
Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
Link: https://patch.msgid.link/20260326082217.39941-2-dev@lankhorst.se
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/gpu/drm/drm_file.c | 5 +----
drivers/gpu/drm/drm_mode_config.c | 9 +++------
2 files changed, 4 insertions(+), 10 deletions(-)

--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -223,7 +223,6 @@ static void drm_events_release(struct dr
void drm_file_free(struct drm_file *file)
{
struct drm_device *dev;
- int idx;

if (!file)
return;
@@ -237,11 +236,9 @@ void drm_file_free(struct drm_file *file

drm_events_release(file);

- if (drm_core_check_feature(dev, DRIVER_MODESET) &&
- drm_dev_enter(dev, &idx)) {
+ if (drm_core_check_feature(dev, DRIVER_MODESET)) {
drm_fb_release(file);
drm_property_destroy_user_blobs(dev, file);
- drm_dev_exit(idx);
}

if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -553,13 +553,10 @@ void drm_mode_config_cleanup(struct drm_
*/
WARN_ON(!list_empty(&dev->mode_config.fb_list));
list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
- if (list_empty(&fb->filp_head) || drm_framebuffer_read_refcount(fb) > 1) {
- struct drm_printer p = drm_dbg_printer(dev, DRM_UT_KMS, "[leaked fb]");
+ struct drm_printer p = drm_dbg_printer(dev, DRM_UT_KMS, "[leaked fb]");

- drm_printf(&p, "framebuffer[%u]:\n", fb->base.id);
- drm_framebuffer_print_info(&p, 1, fb);
- }
- list_del_init(&fb->filp_head);
+ drm_printf(&p, "framebuffer[%u]:\n", fb->base.id);
+ drm_framebuffer_print_info(&p, 1, fb);
drm_framebuffer_free(&fb->base.refcount);
}