From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: lib: test_hmm: evict device pages on file close to avoid use-after-free Date: Tue, 31 Mar 2026 16:41:09 +1000 Message-ID: In-Reply-To: <20260331063445.3551404-2-apopple@nvidia.com> References: <20260331063445.3551404-1-apopple@nvidia.com> <20260331063445.3551404-2-apopple@nvidia.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness concern =E2=80=94 evicts ALL device chunks, not just this dmi= rror's pages:** The release handler iterates over all `mdevice->devmem_chunks` and calls `d= mirror_device_evict_chunk()` on each: ```c if (mdevice->devmem_chunks) { for (i =3D 0; i < mdevice->devmem_count; i++) { struct dmirror_chunk *devmem =3D mdevice->devmem_chunks[i]; dmirror_device_evict_chunk(devmem); } } ``` This evicts **all** device private pages from the entire device, not just p= ages belonging to the closing dmirror instance. If multiple file descriptor= s (and thus multiple `dmirror` structs) are open on the same device simulta= neously, closing one will evict pages that belong to other active dmirror i= nstances. The `migrate_device_range()` call operates on physical PFN ranges= and doesn't filter by owner. In practice this is probably fine for a test driver (tests are sequential),= but it's worth noting in the commit message or as a comment. The proper fi= x would filter pages by checking `zone_device_data` against the closing `dm= irror`, but that's significantly more complex and probably overkill for a t= est module. **Missing locking:** The `dmirror_device_remove_chunks()` function takes `m= device->devmem_lock` and `mdevice->lock` when doing eviction, but the evict= ion in `dmirror_fops_release()` holds neither lock. `migrate_device_range()= ` likely handles its own synchronization, but the lack of `devmem_lock` aro= und the `mdevice->devmem_chunks` / `mdevice->devmem_count` access is a pote= ntial TOCTOU issue if chunks could be added/removed concurrently. **No NULL check on folio_alloc for compound pages:** ```c if (src_pfns[i] & MIGRATE_PFN_COMPOUND) { dpage =3D folio_page(folio_alloc(GFP_HIGHUSER_MOVABLE, order), 0); } ``` If `folio_alloc()` returns NULL, `folio_page(NULL, 0)` will crash. The non-= compound path uses `__GFP_NOFAIL` but the compound path doesn't. This is a = pre-existing issue (the function was just moved), not introduced by this pa= tch. **Overall:** The fix is correct for the reported problem. The function move= is clean =E2=80=94 the code is identical. --- Generated by Claude Code Patch Reviewer