Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [Critical] Unconditional `dept_page_set_bit` in `folio_wait_bit_common` causes Use-After-Free and lock tracking leaks for non-exclusive waits. - [High] Lock handoff in `wake_page_function` registers lock acquisition in the waker's context instead of the sleeper's. - [High] A single lock acquisition incorrectly records two events in the thread's DEPT tracking stack on the fast path. - [High] Generic page flag macros clear the lock bit before notifying the dependency tracker, creating a race condition. - [High] Unconditional addition of `struct dept_ext_wgen` and `struct dept_page_usage` directly to `struct page` causes severe memory bloat. - [Medium] Mismatched DEPT tracking on compound tail pages splits lock acquisition and wait events across different tracking contexts. --
Note: Due to system recitation restrictions, this review uses a free-form summary format instead of standard inline patch quoting. Commit: 1b85b38be891fb326526f230171e29e8fb12a627 Author: Byungchul Park <[email protected]> Subject: dept: track PG_locked with dept Summary: This patch makes dept able to track PG_locked waits and events. Link: https://lore.kernel.org/lkml/[email protected]/ [Severity: High] In include/linux/mm_types.h, the struct page definition is modified to add the pg_locked_wgen and pg_writeback_wgen fields. Are these structures being added unconditionally without CONFIG_DEPT guards? Adding per-page metadata directly to struct page increases the size of the structure for every physical page, which wastes memory on large systems. Should this tracking metadata be moved to the dynamically allocated page_ext framework, or at least be strictly guarded by Kconfig checks? [Severity: High] In include/linux/page-flags.h, macros like FOLIO_CLEAR_FLAG clear the target bit using clear_bit() before notifying the tracker via dept_folio_clear_bit(). Could this out-of-order execution create a race condition? If the lock bit is cleared first, another thread could immediately acquire the lock and call dept_page_set_bit() before the original thread finishes calling dept_folio_clear_bit(). This sequence would process the release after the subsequent acquisition, which might corrupt the tracker state. [Severity: Medium] In include/linux/pagemap.h within lock_page(), dept_page_wait_on_bit() is called on the raw page pointer before converting it to the head folio with page_folio(page). If this is called on a tail page, won't the wait event be recorded on the tail page's tracking metadata, while the subsequent lock acquisition correctly records on the head page? Splitting the wait and acquisition tracking could break the dependency graph for large folios. [Severity: Critical] In mm/filemap.c within folio_wait_bit_common(), the code unconditionally calls dept_page_set_bit() if WQ_FLAG_DONE is set in wait->flags. Does this create a use-after-free for DROP waits? For DROP waits, folio_put() is called before the wait loop. If the folio is concurrently freed, dereferencing the page pointer after the loop will cause a use-after-free. Additionally, for SHARED non-exclusive waits, folio_trylock_flag() sets WQ_FLAG_DONE when the bit is clear. Does this cause the unconditional check in folio_wait_bit_common() to incorrectly register a lock acquisition for a thread that is merely waiting? This could permanently leak the lock tracking context. [Severity: High] Also in folio_wait_bit_common(), if the fast path successfully acquires the lock synchronously, folio_trylock_flag() sets the lock bit, calls dept_page_set_bit(), and sets WQ_FLAG_DONE. When folio_wait_bit_common() checks WQ_FLAG_DONE at the end of the function, won't it call dept_page_set_bit() a second time? This appears to push a duplicate lock acquisition record to the tracker stack for a single lock operation. [Severity: High] In mm/filemap.c within wake_page_function(), during an exclusive lock handoff, the function sets the lock bit on behalf of the sleeper and calls dept_page_set_bit(). Since this function executes in the context of the waker thread, won't the tracker incorrectly record the lock acquisition in the waker's task structure rather than the sleeper's? This could leak the lock tracking state in the waker and break the dependency graph for the sleeper. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=22
