Claude review: drm: Fix use-after-free on framebuffers and property blobs when calling drm_dev_unplug

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**The `drm_file_free` change (drm_file.c):**

The `drm_dev_enter`/`drm_dev_exit` guard is a reasonable approach to skip `drm_fb_release()` and `drm_property_destroy_user_blobs()` when the device has been unplugged:

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

However, this raises a **resource leak concern**: if `drm_dev_enter` fails (device unplugged), we skip both `drm_fb_release` and `drm_property_destroy_user_blobs` entirely. This means the refcounts held by the file's `fbs` list and `blobs` list are never dropped. The patch relies on `drm_mode_config_cleanup()` to handle cleanup instead, but:

1. For framebuffers: `drm_mode_config_cleanup` calls `drm_framebuffer_free` which bypasses `kref_put` and directly invokes the free path. This works to destroy the objects but is unusual — it forcibly frees regardless of refcount.

2. For property blobs: the `property_blob_list` iteration in `drm_mode_config_cleanup` drops the global list reference, but the per-file `head_file` list reference from `file_priv->blobs` is never cleaned up. If a blob has both a global reference and a per-file reference, the per-file `drm_property_blob_put` is skipped, potentially leaking the blob entirely since `drm_mode_config_cleanup` only drops the `head_global` reference.

**The `drm_mode_config_cleanup` change (drm_mode_config.c):**

```c
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]");

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

Several issues here:

1. **The `list_del_init(&fb->filp_head)` is done to satisfy the `drm_WARN_ON(!list_empty(&fb->filp_head))` in `drm_framebuffer_free`** (drm_framebuffer.c:833). This is correct in intent — these fbs still have a `filp_head` because `drm_fb_release` was skipped — but it papers over the problem by just detaching the node right before the check.

2. **The diagnostic condition is inverted in meaning**: the original code unconditionally printed leaked fb info. The new condition `list_empty(&fb->filp_head) || refcount > 1` prints the warning only for fbs that either have no file association (truly leaked) or have extra references. But fbs that are on the `filp_head` list with refcount == 1 (the normal unplug case) are silently freed. This seems intentional but the logic is a bit subtle and deserves a comment.

3. **Calling `drm_framebuffer_free(&fb->base.refcount)` directly** bypasses `kref_put`, so it doesn't check whether the refcount is actually zero. If any other code path still holds a reference (e.g. an in-flight atomic commit), this will cause a double-free. The original code had the same pattern, so this isn't a regression, but it's worth noting.

4. **No corresponding fix for property blobs**: The cover letter and commit message mention property blob crashes, and the `drm_file_free` change skips `drm_property_destroy_user_blobs`, but there's no change to the blob cleanup path in `drm_mode_config_cleanup`. The blobs on `file_priv->blobs` are never cleaned up — their `head_file` links still point into the (about-to-be-freed) file structure. This seems like an incomplete fix. The blob list iteration at `drm_mode_config.c:571` walks `head_global`, so it won't encounter the per-file references, meaning blobs created by the user could leak.

**Missing `Fixes:` tag**: The cover letter asks whether this should have a fixes tag. Yes — this is clearly a bug fix for a crash/oops, so a `Fixes:` tag pointing to the commit that introduced `drm_dev_unplug` (or the relevant mode_config cleanup) and `Cc: stable@vger.kernel.org` would be appropriate.

**Summary of concerns:**
- The property blob cleanup appears incomplete — skipping `drm_property_destroy_user_blobs` in `drm_file_free` without compensating in `drm_mode_config_cleanup` may leak blobs
- The approach of directly calling `drm_framebuffer_free` (bypassing kref) is fragile if any other references exist
- A `Fixes:` tag and `Cc: stable` should be added
- The conditional leak-warning logic in `drm_mode_config_cleanup` could use a brief comment explaining the reasoning

---
Generated by Claude Code Patch Reviewer