Claude review: fbdev: defio: Disconnect deferred I/O from the lifetime of struct fb_info

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

Patch Review

**Positive aspects:**

1. **Correct kref lifecycle**: `fb_deferred_io_state_alloc()` initializes the refcount to 1, `fb_deferred_io_mmap()` takes a reference per VMA, `vm_close` puts it, and `fb_deferred_io_cleanup()` drops the init reference. This ensures the state survives until the last VMA is closed.

2. **Proper mutex protection for info access**: All paths that dereference `fbdefio_state->info` (fault, track_page, work) hold `fbdefio_state->lock`. The cleanup path sets `info = NULL` under the same lock:

   ```c
   mutex_lock(&fbdefio_state->lock);
   fbdefio_state->info = NULL;
   mutex_unlock(&fbdefio_state->lock);
   ```

3. **Clean SIGBUS on hot-unplug**: Both `fb_deferred_io_fault` and `fb_deferred_io_track_page` correctly check for NULL info and return `VM_FAULT_SIGBUS`:

   ```c
   info = fbdefio_state->info;
   if (!info) {
       ret = VM_FAULT_SIGBUS; /* our device is gone */
       goto err_mutex_unlock;
   }
   ```

4. **Lock scope expansion is correct**: The mutex previously only protected pageref list operations in `track_page` and `work`. Now it additionally protects the fault path, which is necessary since fault now needs to validate the info pointer. No deadlock is introduced since none of the callees (`fb_deferred_io_get_page`, `vmalloc_to_page`, `get_page`) take this mutex.

**Observations / minor concerns:**

5. **Race window in cleanup**: Between `flush_delayed_work` and setting `fbdefio_state->info = NULL`, a concurrent `page_mkwrite` could still see a valid `info`, complete `track_page`, and call `schedule_delayed_work(&info->deferred_work, ...)`. The scheduled work would then fire after `info` is freed. This is a narrow window and exists identically in the upstream code, so it's not a regression introduced by this backport. In practice, `flush_delayed_work` makes this extremely difficult to hit because all dirty pages are processed and write-protected, requiring a new user-space write to race with teardown.

   ```c
   fb_deferred_io_lastclose(info);     /* flush_delayed_work */

   info->fbdefio_state = NULL;

   mutex_lock(&fbdefio_state->lock);
   fbdefio_state->info = NULL;         /* <-- race window above */
   mutex_unlock(&fbdefio_state->lock);
   ```

6. **`schedule_delayed_work` after mutex unlock**: In `fb_deferred_io_track_page`, after releasing the lock, `info` and `fbdefio` are accessed without protection:

   ```c
   mutex_unlock(&fbdefio_state->lock);

   /* come back after delay to process the deferred IO */
   schedule_delayed_work(&info->deferred_work, fbdefio->delay);
   ```

   This is the same pattern as the pre-existing code and upstream. It's safe in the common case because cleanup flushes first, but is related to concern #5 above.

7. **Missing `module_get`/`module_put`**: The drm-next version of this code adds `try_module_get(THIS_MODULE)` in `fb_deferred_io_mmap()` and `fb_deferred_io_vm_open()`, with corresponding `module_put` in `vm_close`. These are absent in this backport. Without them, the fbdev module could be unloaded while VMAs still reference the `vm_ops` function pointers. This is likely addressed by a separate upstream commit and would be a good follow-up backport.

8. **`fb_deferred_io_fsync` not protected**: The fsync path still accesses `info` directly via `file->private_data` without going through the state object:

   ```c
   struct fb_info *info = file->private_data;
   ...
   flush_delayed_work(&info->deferred_work);
   ```

   If fsync races with hot-unplug after `info` is freed, this is a UAF. Same as upstream — the fbdev framework's `fb_release` ordering likely prevents this in practice, but it's worth noting as an area for future hardening.

9. **`BUG_ON` retained**: The backport keeps `BUG_ON(!info->fbdefio->mapping)` in the fault handler, whereas drm-next softened this to `fb_WARN_ON_ONCE`. This is correct for a stable backport (match the existing code style), but a crash here on a misconfigured driver is harsh.

10. **Backport adaptation is clean**: The `kzalloc(sizeof(*fbdefio_state), GFP_KERNEL)` substitution for `kzalloc_obj()` is the only manual adaptation and is correctly noted in the commit trailer. The alloc function is simpler than upstream's `fb_deferred_io_state_alloc(unsigned long len)` because pageref allocation remains in `fb_deferred_io_init()` — this is intentional and correct for a minimal backport.

---
Generated by Claude Code Patch Reviewer