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

**Positive aspects:**
- The bug report with stack traces is excellent and makes the problem clear.
- The SRCU-based argument for safety between `drm_file_free()` and the unplug path is sound: after `synchronize_srcu()`, no new `drm_dev_enter()` can succeed, so the guarded block in `drm_file_free()` can't run concurrently with `drm_modeset_unplug_all()`.

**Issue 1 (Medium): Missing locking in `drm_framebuffer_unplug()` and `drm_property_blob_unplug()`**

`drm_framebuffer_unplug()` iterates `dev->mode_config.fb_list` without holding `fb_lock`, and does `list_del_init(&fb->filp_head)` without holding the owning `file_priv->fbs_lock`:

```c
void drm_framebuffer_unplug(struct drm_device *dev)
{
	struct drm_framebuffer *fb, *next;

	list_for_each_entry_safe(fb, next, &dev->mode_config.fb_list, head) {
		if (!list_empty(&fb->filp_head)) {
			list_del_init(&fb->filp_head);
			drm_framebuffer_put(fb);
		}
	}
}
```

While the SRCU synchronization prevents concurrent `drm_fb_release()` / `drm_property_destroy_user_blobs()` via `drm_file_free()`, it does **not** prevent concurrent ioctls. `drm_dev_is_unplugged()` in `drm_ioctl()` uses `drm_dev_enter()`/`drm_dev_exit()` but exits the SRCU read section immediately — it doesn't protect the full ioctl duration. So an in-flight `drm_mode_addfb2()` could be doing `list_add(&fb->filp_head, &file_priv->fbs)` under `fbs_lock` while `drm_framebuffer_unplug()` concurrently does `list_del_init(&fb->filp_head)` on the same per-file list without `fbs_lock`. Similarly, `drm_framebuffer_init()` modifies `fb_list` under `fb_lock` while `drm_framebuffer_unplug()` iterates it without that lock.

The same concern applies to `drm_property_blob_unplug()` which iterates `property_blob_list` and modifies `head_file` without holding `blob_lock`.

At minimum, `drm_framebuffer_unplug()` should hold `fb_lock` while iterating `fb_list`, and `drm_property_blob_unplug()` should hold `blob_lock`. The `filp_head` / `head_file` manipulation is trickier since the per-file lock isn't easily accessible when iterating by device, but at least the global list iteration should be protected.

**Issue 2 (Minor): `drm_framebuffer_put()` may call driver destroy during unplug**

If the file reference is the last reference on a framebuffer, `drm_framebuffer_put()` will call `fb->funcs->destroy()`, which invokes driver-specific cleanup (e.g., `intel_user_framebuffer_destroy()`). At unplug time, driver state may be partially torn down. The comment says:

```
 * There is no need to call drm_framebuffer_remove, as the driver
 * is now gone and will perform it's own cleanup of framebuffers.
```

This seems to assume the put won't be the last reference, but that's not guaranteed for fbs that aren't actively displayed. It would be worth verifying that the driver's destroy callback is safe to call during unplug.

**Issue 3 (Nit): Typo in comment**

```c
 * is now gone and will perform it's own cleanup of framebuffers.
```

Should be "its own" (possessive), not "it's" (contraction of "it is").

**Issue 4 (Nit): Trailing blank line**

```c
void drm_modeset_unplug_all(struct drm_device *dev)
{
	/* With drm_dev_enter() now failing, it's safe to unplug all file_priv->fb's and user mode blobs */
	drm_framebuffer_unplug(dev);
	drm_property_blob_unplug(dev);

}
```

There's a spurious blank line before the closing brace.

**Issue 5 (Question): Fixes tag / Cc stable**

The author asks whether this needs a Fixes tag or Cc stable. Given this is a UAF crash triggered by a real workflow (module reload with running compositor), it should have both. The Fixes tag should reference the commit that introduced `drm_dev_unplug()` (likely `3f04e0a6` or similar), and `Cc: stable@vger.kernel.org` would be appropriate since it's fixing a crash.

**Summary:** The overall approach is correct and addresses a real bug. The main concern is the missing locking in the new unplug helpers — at minimum the global list locks (`fb_lock`, `blob_lock`) should be held while iterating. The ioctl race window is narrow but should be acknowledged, and the author should consider whether there's a better synchronization point (or at least document why the race is benign).

---
Generated by Claude Code Patch Reviewer