From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: fbdev: defio: Disconnect deferred I/O from the lifetime of struct fb_info Date: Fri, 27 Feb 2026 15:29:26 +1000 Message-ID: In-Reply-To: <20260224082657.207284-2-tzimmermann@suse.de> References: <20260224082657.207284-1-tzimmermann@suse.de> <20260224082657.207284-2-tzimmermann@suse.de> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This is the core fix, tagged with `Cc: stable` back to v2.6.22. It introduc= es `struct fb_deferred_io_state` with kref-based lifetime management, decou= pled from `struct fb_info`. **Key design decisions (all good):** - VMAs now store `fbdefio_state` in `vm_private_data` instead of `info`, an= d hold a kref reference via `vm_open`/`vm_close` ops. - The state lock replaces `fbdefio->lock` for protecting the pageref list. - Fault and mkwrite handlers check `fbdefio_state->info !=3D NULL` under th= e lock, returning `VM_FAULT_SIGBUS` when the device is gone. **Concern: race between schedule_delayed_work and cleanup** In `fb_deferred_io_track_page()`, after unlocking the state mutex: ```c mutex_unlock(&fbdefio_state->lock); /* come back after delay to process the deferred IO */ schedule_delayed_work(&info->deferred_work, fbdefio->delay); ``` At this point `info` is a local copy obtained under the lock, but the lock = is no longer held. A concurrent `fb_deferred_io_cleanup()` could proceed th= rough: ```c fb_deferred_io_lastclose(info); /* cancel_delayed_work_sync */ info->fbdefio_state =3D NULL; mutex_lock(&fbdefio_state->lock); fbdefio_state->info =3D NULL; mutex_unlock(&fbdefio_state->lock); fb_deferred_io_state_put(fbdefio_state); kvfree(info->pagerefs); ``` If cleanup's `cancel_delayed_work_sync` runs *before* thread A's `schedule_= delayed_work`, the work gets re-queued on potentially soon-to-be-freed `inf= o`. When the work fires, `fb_deferred_io_work` does `info->fbdefio_state` w= hich could be NULL. This is a very narrow window and is essentially pre-existing (the old code = had the same pattern with `fbdefio->lock`), so it's not a regression. But i= f you wanted to close it, one option would be to have `fb_deferred_io_work`= check `fbdefio_state->info` under the lock before proceeding, and to ensur= e `info->fbdefio_state` is not set to NULL before the lock/NULL/unlock sequ= ence. Alternatively, moving `schedule_delayed_work` inside the lock would n= arrow (but not fully eliminate) the window. **Minor nit:** In `fb_deferred_io_cleanup`, setting `info->fbdefio_state = =3D NULL` is done without holding the state lock. This is fine for the faul= t/mkwrite paths (they go through the VMA's `vm_private_data`, not through `= info`), but `fb_deferred_io_work` reads `info->fbdefio_state` without synch= ronization. This is safe only because `cancel_delayed_work_sync` prevents t= he work from running concurrently with this assignment =E2=80=94 but see th= e race discussion above. **Code quality is good.** The refcounting is straightforward, the SIGBUS re= turn for dead devices is the right behavior, and the locking strategy is re= asonable. --- --- Generated by Claude Code Patch Reviewer