Re: [PATCH v19 22/40] dept: track PG_locked with dept

[sashiko-bot@kernel.org]

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 <byungchul@sk.com>
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/1674268856-31807-1-git-send-email-byungchul.park@lge.com/

[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/20260706061928.66713-1-byungchul@sk.com?part=22