From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: mm/zone_device: Do not touch device folio after calling ->folio_free() Date: Sun, 12 Apr 2026 09:15:49 +1000 Message-ID: In-Reply-To: <20260410230346.4009855-1-matthew.brost@intel.com> References: <20260410230346.4009855-1-matthew.brost@intel.com> <20260410230346.4009855-1-matthew.brost@intel.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 **The bug:** In `free_zone_device_folio()`, a local variable `pgmap` is saved from `foli= o->pgmap` at function entry (line 418 of the current tree): ```c struct dev_pagemap *pgmap =3D folio->pgmap; ``` This local is used correctly throughout the function =E2=80=94 except at li= ne 457, where the code dereferences `folio->pgmap` again instead of using t= he local: ```c pgmap->ops->folio_free(folio); - percpu_ref_put_many(&folio->pgmap->ref, nr); + percpu_ref_put_many(&pgmap->ref, nr); ``` After `folio_free()` returns, the driver may have already reallocated the f= olio's backing memory with a different order, meaning `folio->pgmap` can co= ntain garbage. The stack trace confirms this =E2=80=94 RAX holds `0x0900000= 000000000`, a non-canonical address, which is what you'd expect from readin= g a corrupted `pgmap` pointer out of a recycled folio. **The fix:** The one-line change replaces `folio->pgmap->ref` with `pgmap->ref`, using t= he local variable that was already captured before the folio was freed. Thi= s is the obviously correct fix. **Additional observations:** 1. **Correctness**: The fix is correct. `pgmap` is captured before any muta= tion of the folio, and `percpu_ref_put_many` on `pgmap->ref` is the right t= hing to do after the folio has been freed. 2. **Fixes tag**: The `Fixes:` tag references `d245f9b4ab80 ("mm/zone_devic= e: support large zone device private folios")` which is the commit that int= roduced the `nr` variable and the `percpu_ref_put_many` call (replacing an = earlier `percpu_ref_put`). This seems like a reasonable attribution =E2=80= =94 the use of `folio->pgmap` instead of the local `pgmap` was likely intro= duced or preserved in that commit. 3. **Scope check**: Looking at the other cases in the same `switch` stateme= nt, `MEMORY_DEVICE_PCI_P2PDMA` (line 472-476) also calls `pgmap->ops->folio= _free(folio)` but does not call `percpu_ref_put_many` afterward, so it does= n't have this bug. The `MEMORY_DEVICE_GENERIC` and `MEMORY_DEVICE_FS_DAX` c= ases don't call `folio_free` at all. 4. **Cc list**: The patch is appropriately CC'd to mm maintainers and relev= ant mailing lists. **Reviewed-by worthy**: Yes, this is a clean, minimal, correct fix for a re= al use-after-free. No concerns. --- Generated by Claude Code Patch Reviewer