* Re: [PATCH v19 23/40] dept: print staged wait's stacktrace on report
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: linux-kernel, linux-ide, tytso, cassel, Anna Schumaker, dlemoal,
Chuck Lever, dri-devel, linux-modules, Jeff Layton, ojeda,
linux-rt-devel
In-Reply-To: <20260706061928.66713-24-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Data race on `dt->stage_m` leading to zeroed class keys and kernel warnings.
--
commit 02dbd1fb0ecda3493e1e312bcf532860fa1353de
Author: Byungchul Park <byungchul@sk.com>
dept: print staged wait's stacktrace on report
This commit modifies the dependency tracking (dept) system to capture and
print the stacktrace of the wait that an event wakes up. This provides more
context for interpreting dept's reports.
> diff --git a/kernel/dependency/dept.c b/kernel/dependency/dept.c
> index e425d3ab05e5a..5bf32633e1fbd 100644
> --- a/kernel/dependency/dept.c
> +++ b/kernel/dependency/dept.c
[ ... ]
> @@ -2561,6 +2580,14 @@ void dept_request_event_wait_commit(void)
>
> wg = next_wgen();
> WRITE_ONCE(dt->stage_m.wgen, wg);
> +
> + /*
> + * __schedule() can be hit multiple times between
> + * dept_stage_wait() and dept_clean_stage(). In that case,
> + * keep the first stacktrace only. That's enough.
> + */
> + if (!dt->stage_wait_stack)
> + dt->stage_wait_stack = get_current_stack();
> arch_spin_unlock(&dt->stage_lock);
>
> __dept_wait(&dt->stage_m, 1UL, ip, w_fn, 0, true, sched_map, timeout);
[Severity: High]
This isn't a bug introduced by this patch, but does accessing dt->stage_m
here, immediately after dropping the stage_lock, introduce a data race with
a concurrent wakeup?
If a task begins scheduling out, it calls dept_request_event_wait_commit()
from __schedule(). It acquires dt->stage_lock, updates fields, and unlocks
it. Before it can call __dept_wait(&dt->stage_m, ...), an interrupt or
another CPU could concurrently call try_to_wake_up() on the same task.
The waking CPU would acquire dt->stage_lock in dept_ttwu_stage_wait() and
zero out dt->stage_m via __dept_clean_stage():
kernel/dependency/dept.c:__dept_clean_stage() {
memset(&dt->stage_m, 0x0, sizeof(struct dept_map));
...
}
If __dept_wait(&dt->stage_m) executes concurrently on the original CPU, it
reads the zeroed structure. This would cause it to look up a class with key
0 and trigger DEPT_WARN_ON(!valid_class(c)), breaking dependency tracking
and potentially panicking the system if panic_on_warn is enabled.
Could this be prevented by either copying the required parts of dt->stage_m
to local variables while the lock is still held, or expanding the lock
coverage?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=23
^ permalink raw reply
* Re: [PATCH v19 22/40] dept: track PG_locked with dept
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: linux-kernel, dri-devel, linux-rt-devel, Anna Schumaker,
linux-ide, dlemoal, cassel, Chuck Lever, tytso, ojeda,
linux-modules, Jeff Layton
In-Reply-To: <20260706061928.66713-23-byungchul@sk.com>
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
^ permalink raw reply
* Re: [PATCH v19 21/40] dept: make dept able to work with an external wgen
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: tytso, dri-devel, Chuck Lever, linux-kernel, linux-ide, dlemoal,
linux-rt-devel, linux-modules, ojeda, Anna Schumaker, Jeff Layton,
cassel
In-Reply-To: <20260706061928.66713-22-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Unconditional zeroing of `wgen` in `dept_event` creates a race condition that systematically loses dependencies for shared locks, blinding the deadlock detector.
--
commit f7ae795e924d8a4b1154e57adaf0e878e810272c
Author: Byungchul Park <byungchul@sk.com>
dept: make dept able to work with an external wgen
This commit introduces an external wait generation structure, struct
dept_ext_wgen, to allow subsystems with memory constraints to use DEPT
without embedding a full struct dept_map. It modifies dept_request_event()
and dept_event() to optionally accept and operate on this external wgen
instead of the one embedded in struct dept_map.
> diff --git a/kernel/dependency/dept.c b/kernel/dependency/dept.c
> index 5ef85977e6314..e425d3ab05e5a 100644
> --- a/kernel/dependency/dept.c
> +++ b/kernel/dependency/dept.c
[ ... ]
> @@ -2816,24 +2826,26 @@ void dept_event(struct dept_map *m, unsigned long e_f,
> if (m->nocheck)
> return;
>
> + wg_p = ewg ? &ewg->wgen : &m->wgen;
> +
> if (dt->recursive) {
> /*
> * Dept won't work with this even though an event
> * context has been asked. Don't make it confused at
> * handling the event. Disable it until the next.
> */
> - WRITE_ONCE(m->wgen, 0U);
> + WRITE_ONCE(*wg_p, 0U);
> return;
> }
>
> flags = dept_enter();
>
> - __dept_event(m, m, e_f, ip, e_fn, false);
> + __dept_event(m, m, e_f, ip, e_fn, false, READ_ONCE(*wg_p));
>
> /*
> * Keep the map diabled until the next sleep.
> */
> - WRITE_ONCE(m->wgen, 0U);
> + WRITE_ONCE(*wg_p, 0U);
[Severity: High]
This is a pre-existing issue, but does unconditionally clearing the wgen here
in dept_event() create a race condition that loses dependencies for shared
locks?
When multiple threads hold a shared lock (e.g., a read-lock like
rw_semaphore), and a writer begins waiting, the writer calls
dept_request_event() which sets the lock's wgen. The first reader to unlock
calls dept_event(), which reads the wgen, processes the wait dependency for
its own context, and then unconditionally clears wgen to 0 here.
All subsequent readers unlocking will read 0 and fail to record the dependency
between the writer's wait and their own read-side critical sections, which
could blind the deadlock detector.
Since this issue affects the newly introduced ewg->wgen as well, could this
be addressed to ensure dependencies aren't lost when multiple threads share
the lock?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=21
^ permalink raw reply
* Re: [PATCH v19 19/40] dept: apply timeout consideration to hashed-waitqueue wait
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: ojeda, linux-ide, linux-kernel, Anna Schumaker, Chuck Lever,
dlemoal, linux-rt-devel, Jeff Layton, tytso, cassel,
linux-modules, dri-devel
In-Reply-To: <20260706061928.66713-20-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing `sdt_might_sleep_end()` cleanup in the error path of wait macros (e.g., `___wait_var_event`, `___wait_event`, `___swait_event`) bypasses `dept_clean_stage()`, leaking the staged wait event state and corrupting DEPT tracking.
--
commit cf04720896ab037609320947c88e8faa57dbddd7
Author: Byungchul Park <byungchul@sk.com>
dept: apply timeout consideration to hashed-waitqueue wait
Now that CONFIG_DEPT_AGGRESSIVE_TIMEOUT_WAIT was introduced, apply the
consideration to hashed-waitqueue wait, assuming an input 'ret' in
___wait_var_event() macro is used as a timeout value.
> diff --git a/include/linux/wait_bit.h b/include/linux/wait_bit.h
> index 179a616ad245a..9885ac4e1ded5 100644
> --- a/include/linux/wait_bit.h
> +++ b/include/linux/wait_bit.h
> @@ -258,7 +258,7 @@ extern wait_queue_head_t *__var_waitqueue(void *p);
> struct wait_bit_queue_entry __wbq_entry; \
> long __ret = ret; /* explicit shadow */ \
> \
> - sdt_might_sleep_start(NULL); \
> + sdt_might_sleep_start_timeout(NULL, __ret); \
[Severity: High]
Does this error path properly clean up the dept wait state?
Lower in the ___wait_var_event() macro, if a signal is received it jumps to
__out:
if (___wait_is_interruptible(state) && __int) { \
__ret = __int; \
goto __out; \
} \
...
sdt_might_sleep_end(); \
__out: __ret; \
Because the goto __out branch skips sdt_might_sleep_end(), does it leave a
dangling stage context in the dept subsystem's tracking graph?
If the stage context remains populated, could a subsequent wait on the same
task be incorrectly seen as a nested stage, potentially corrupting dept
tracking or leading to false deadlock reports?
(Note: A similar goto __out pattern skipping sdt_might_sleep_end() also
exists in ___wait_event() in include/linux/wait.h and ___swait_event() in
include/linux/swait.h).
> init_wait_var_entry(&__wbq_entry, var, \
> exclusive ? WQ_FLAG_EXCLUSIVE : 0); \
> for (;;) { \
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=19
^ permalink raw reply
* Re: [PATCH v19 18/40] dept: apply timeout consideration to waitqueue wait
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: cassel, tytso, ojeda, linux-kernel, dlemoal, Jeff Layton,
linux-rt-devel, Anna Schumaker, linux-modules, dri-devel,
Chuck Lever, linux-ide
In-Reply-To: <20260706061928.66713-19-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The timeout consideration logic fails for wait_event_hrtimeout(), hashed-waitqueues, and DMA fences, causing them to be falsely treated as infinite waits.
- [Medium] The ___wait_event() and ___wait_var_event() macros skip the DEPT sleep end annotation on the signal interruption error path, leaking the DEPT tracking stage context.
--
commit 25089e0217c82211740316928fec38c6d2b7e4fa
Author: Byungchul Park <byungchul@sk.com>
dept: apply timeout consideration to waitqueue wait
This commit applies timeout consideration to waitqueue waits by using the
ret parameter in ___wait_event() as a timeout value for DEPT tracking. It
leverages CONFIG_DEPT_AGGRESSIVE_TIMEOUT_WAIT to help reduce false positive
deadlock reports for waits that are meant to expire.
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index c8f8b44060fb9..a9524bc8630b7 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -306,7 +306,7 @@ extern void init_wait_entry(struct wait_queue_entry *wq_entry, int flags);
> struct wait_queue_entry __wq_entry; \
> long __ret = ret; /* explicit shadow */ \
> \
> - sdt_might_sleep_start(NULL); \
> + sdt_might_sleep_start_timeout(NULL, __ret); \
[Severity: Medium]
Does this logic also need to be applied to __wait_event_hrtimeout(),
hashed-waitqueues, and DMA fences?
For example, __wait_event_hrtimeout() passes 0 as the 'ret' parameter to
___wait_event(), causing sdt_might_sleep_start_timeout() to treat it as an
infinite wait.
Additionally, hashed-waitqueues (in ___wait_var_event) and DMA fences
(in dma_fence_default_wait and dma_fence_wait_any_timeout) still hardcode
sdt_might_sleep_start(NULL) even when taking a timeout. This seems to cause
DEPT to falsely report deadlocks for these bounded waits.
> init_wait_entry(&__wq_entry, exclusive ? WQ_FLAG_EXCLUSIVE : 0); \
> for (;;) { \
> long __int = prepare_to_wait_event(&wq_head, &__wq_entry, state);\
[Severity: Medium]
Can the DEPT sleep end annotation be skipped on the signal interruption
error path?
Looking further down in the ___wait_event() macro, if the wait is
interruptible and interrupted by a signal, it takes the goto __out branch:
if (___wait_is_interruptible(state) && __int) {
__ret = __int;
goto __out;
}
...
}
finish_wait(&wq_head, &__wq_entry);
sdt_might_sleep_end();
__out: __ret;
This skips over sdt_might_sleep_end(), which leaks the DEPT wait stage
context. As a result, DEPT will mistakenly treat subsequent wait events for
this task as nested waits.
The same issue appears to exist in ___wait_var_event() for
hashed-waitqueues.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=18
^ permalink raw reply
* Re: [PATCH v19 07/40] dept: distinguish each work from another
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: linux-rt-devel, cassel, linux-ide, tytso, linux-kernel,
Anna Schumaker, linux-modules, Chuck Lever, ojeda, dlemoal,
dri-devel, Jeff Layton
In-Reply-To: <20260706061928.66713-8-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] `dept_update_cxt()` hardcodes the process context ID, failing to track bottom-half (BH) work items in softirq context and corrupting the context of preempted tasks.
--
commit b8dda7d95e5377770d28c6712cf8fe7fafa98ade
Author: Byungchul Park <byungchul@sk.com>
dept: distinguish each work from another
This commit updates DEPT to assign a different context ID to each work item.
Since workqueues provide their own concurrency control, wait operations in a
work item do not prevent events in other works from executing.
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index c6ea96d5b7167..4a4075d0697c7 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
[ ... ]
> @@ -3204,6 +3205,8 @@ __acquires(&pool->lock)
>
> lockdep_copy_map(&lockdep_map, &work->lockdep_map);
> #endif
> + dept_update_cxt();
> +
[Severity: Medium]
Will this unconditionally modify the process context ID, even when tracking
bottom-half (BH) work items running in softirq context?
When a BH workqueue executes multiple work items in a single softirq batch,
process_one_work() executes them and calls dept_update_cxt(). However,
dept_update_cxt() is implemented as:
kernel/dependency/dept.c:dept_update_cxt() {
struct dept_task *dt = dept_task();
dt->cxt_id[DEPT_CXT_PROCESS] += 1UL << DEPT_CXTS_NR;
}
This hardcodes the increment to DEPT_CXT_PROCESS rather than checking
cur_cxt() to increment DEPT_CXT_SIRQ. Does this cause DEPT to fail in
distinguishing sequential BH work items and falsely report dependencies?
Also, can this corrupt the continuous dependency tracking context of
whatever task was preempted by the softirq?
> /* ensure we're on the correct CPU */
> WARN_ON_ONCE(!(pool->flags & POOL_DISASSOCIATED) &&
> raw_smp_processor_id() != pool->cpu);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=7
^ permalink raw reply
* Re: [PATCH v19 06/40] dept: distinguish each kernel context from another
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: cassel, dri-devel, Anna Schumaker, ojeda, tytso, linux-modules,
Jeff Layton, Chuck Lever, dlemoal, linux-ide, linux-kernel,
linux-rt-devel
In-Reply-To: <20260706061928.66713-7-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] cur_cxt() incorrectly identifies a hardirq interrupting a softirq as a softirq context, mixing dependencies between unrelated execution contexts.
--
commit 504ac1eb7d0a4081c645648037ec455b5ec741d7
Author: Byungchul Park <byungchul@sk.com>
dept: distinguish each kernel context from another
Each unique kernel context, in dept's point of view, should be
identified on every entrance to kernel mode e.g. system call or user
oriented fault. Otherwise, dept may track meaningless dependencies
across different kernel context.
> diff --git a/kernel/dependency/dept.c b/kernel/dependency/dept.c
> index fd091ae28ae27..1c4de1b55730a 100644
> --- a/kernel/dependency/dept.c
> +++ b/kernel/dependency/dept.c
[ ... ]
> -static int cur_irq(void)
> +static int cur_cxt(void)
> {
> if (lockdep_softirq_context(current))
> - return DEPT_SIRQ;
> + return DEPT_CXT_SIRQ;
> if (lockdep_hardirq_context())
> - return DEPT_HIRQ;
> - return DEPT_IRQS_NR;
> + return DEPT_CXT_HIRQ;
> + return DEPT_CXT_PROCESS;
> }
[Severity: High]
This is a pre-existing issue, but does cur_cxt() incorrectly prioritize softirq
over hardirq context here?
If a softirq is executing and is interrupted by a hardirq, both
lockdep_softirq_context(current) and lockdep_hardirq_context() evaluate to
true. Because the softirq check comes first, cur_cxt() will return
DEPT_CXT_SIRQ instead of DEPT_CXT_HIRQ.
Could this cause the waits and events triggered inside the interrupting hardirq
to be erroneously attributed to the interrupted softirq's context, potentially
mixing unrelated dependencies and leading to false positive circular dependency
reports?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=6
^ permalink raw reply
* Re: [PATCH v19 15/40] dept: track timeout waits separately with a new Kconfig
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: cassel, linux-rt-devel, ojeda, linux-ide, dlemoal, Chuck Lever,
Jeff Layton, linux-modules, Anna Schumaker, linux-kernel,
dri-devel, tytso
In-Reply-To: <20260706061928.66713-16-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Semantic collision in the timeoutval argument causes 0-timeout try-waits to be incorrectly tracked as hard blocking dependencies.
- [High] Enabling CONFIG_DEPT_AGGRESSIVE_TIMEOUT_WAIT masks real deadlocks because a timeout wait permanently poisons the dependency edge, causing subsequent strict (non-timeout) waits to be ignored.
--
commit 1b06fc68b17e6c7948f8187e55952d1767e48762
Author: Byungchul Park <byungchul@sk.com>
dept: track timeout waits separately with a new Kconfig
This patch modifies DEPT to track timeout waits separately to avoid
reporting false positive deadlocks for deliberate timeouts. It also
introduces the CONFIG_DEPT_AGGRESSIVE_TIMEOUT_WAIT option to toggle this
tracking behavior.
> diff --git a/kernel/dependency/dept.c b/kernel/dependency/dept.c
> index 3af360ba17d8e..5ef85977e6314 100644
> --- a/kernel/dependency/dept.c
> +++ b/kernel/dependency/dept.c
[ ... ]
> @@ -810,6 +812,24 @@ static void print_dep(struct dept_dep *d)
>
> static void save_current_stack(int skip);
>
> +static bool is_timeout_wait_circle(struct dept_class *c)
> +{
> + struct dept_class *fc = c->bfs_parent;
> + struct dept_class *tc = c;
> +
> + do {
> + struct dept_dep *d = lookup_dep(fc, tc);
> +
> + if (d->wait->timeout)
> + return true;
[Severity: High]
If CONFIG_DEPT_AGGRESSIVE_TIMEOUT_WAIT is enabled, does this mask genuine
deadlocks if a strict wait follows a timeout wait?
If a timeout wait between class A and class B occurs first, it creates a
dependency edge with wait->timeout set to true. If a strict (non-timeout)
wait subsequently occurs between the same classes, add_dep() finds the
existing edge and returns early:
kernel/dependency/dept.c:add_dep() {
if (lookup_dep(fc, tc))
return;
...
}
The edge is never upgraded to reflect the stricter dependency.
If a deadlock later happens involving this edge, would is_timeout_wait_circle()
see the stale timeout flag and incorrectly report it as an intended circular
dependency instead of a genuine deadlock?
> +
> + tc = fc;
> + fc = fc->bfs_parent;
> + } while (tc != c);
> +
> + return false;
> +}
[ ... ]
> @@ -2355,14 +2381,23 @@ static void __dept_event(struct dept_map *m, struct dept_map *real_m,
> }
>
> void dept_wait(struct dept_map *m, unsigned long w_f,
> - unsigned long ip, const char *w_fn, int sub_l)
> + unsigned long ip, const char *w_fn, int sub_l,
> + long timeoutval)
> {
> struct dept_task *dt = dept_task();
> unsigned long flags;
> + bool timeout;
>
> if (unlikely(!dept_working()))
> return;
>
> + timeout = timeoutval > 0 && timeoutval < MAX_SCHEDULE_TIMEOUT;
[Severity: High]
Does this logic correctly handle try-waits or polling?
When a caller invokes a polling wait like wait_for_completion_timeout(x, 0),
the timeoutval is 0. Because 0 > 0 evaluates to false, this sets timeout to
false. DEPT then tracks this non-blocking poll as an indefinite hard wait.
Can this lead to false positive deadlock reports when no actual blocking
occurs?
> +
> +#if !defined(CONFIG_DEPT_AGGRESSIVE_TIMEOUT_WAIT)
> + if (timeout)
> + return;
> +#endif
> +
> if (dt->recursive)
> return;
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=15
^ permalink raw reply
* Re: [PATCH v19 02/40] dept: add single event dependency tracker APIs
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: Chuck Lever, Anna Schumaker, ojeda, linux-modules, Jeff Layton,
tytso, cassel, linux-rt-devel, dri-devel, dlemoal, linux-ide,
linux-kernel
In-Reply-To: <20260706061928.66713-3-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Unsafe macro conversion of functions causes double-evaluation of arguments, leading to logic bugs and memory corruption.
--
commit e5795ffb367b1b68ef8574b0a67f0bf3654baf77
Author: Byungchul Park <byungchul@sk.com>
dept: add single event dependency tracker APIs
Wrapped the base APIs for easier annotation on wait and event. Start
with supporting waiters on each single event. More general support for
multiple events is a future work. Do more when the need arises.
> diff --git a/include/linux/dept_sdt.h b/include/linux/dept_sdt.h
> new file mode 100644
> index 0000000000000..0535f763b21be
> --- /dev/null
> +++ b/include/linux/dept_sdt.h
[ ... ]
> +#define sdt_map_init_key(m, k) dept_map_init(m, k, 0, #m)
> +
> +#define sdt_wait(m) \
> + do { \
> + dept_request_event(m); \
> + dept_wait(m, 1UL, _THIS_IP_, __func__, 0); \
> + } while (0)
[Severity: Critical]
Can this macro evaluate its argument multiple times, causing a regression
if the argument contains side effects?
Unlike sdt_might_sleep_start(m) later in this file which safely captures
its argument in a local variable __m, sdt_wait(m) evaluates its argument
twice.
If an expression with a post-increment is passed to a macro like this,
the side effect will execute multiple times. This is especially risky
if existing synchronization primitives like wait_for_completion(x) are
converted into similar macros to integrate with the tracker.
For example, looking at drivers/vdpa/mlx5/core/resources.c:
mlx5_vdpa_add_res() {
...
while (completed < issued)
wait_for_completion(&cmds[completed++].cmd_done);
...
}
If wait_for_completion() or a similar wrapper evaluates its argument twice,
completed++ increments twice per loop iteration. This would skip completion
entries and eventually access out-of-bounds memory past the end of the cmds
array.
Could the argument be evaluated once and captured in a local variable
within these macros to avoid this?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=2
^ permalink raw reply
* Re: [PATCH v19 17/40] dept: apply timeout consideration to swait
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: cassel, Jeff Layton, Anna Schumaker, ojeda, dri-devel,
linux-rt-devel, tytso, dlemoal, linux-modules, linux-kernel,
linux-ide, Chuck Lever
In-Reply-To: <20260706061928.66713-18-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] When a wait event is interrupted by a signal, it bypasses sdt_might_sleep_end(), permanently leaking the DEPT wait stage.
--
commit 264d829a8f72641205caea8b130ee3c25557563a
Author: Byungchul Park <byungchul@sk.com>
dept: apply timeout consideration to swait
Now that CONFIG_DEPT_AGGRESSIVE_TIMEOUT_WAIT was introduced, apply the
consideration to swait, assuming an input 'ret' in ___swait_event()
macro is used as a timeout value.
> diff --git a/include/linux/swait.h b/include/linux/swait.h
> index 277ac74f61c39..233acdf55e9bc 100644
> --- a/include/linux/swait.h
> +++ b/include/linux/swait.h
> @@ -162,7 +162,7 @@ extern void finish_swait(struct swait_queue_head *q, struct swait_queue *wait);
> struct swait_queue __wait; \
> long __ret = ret; \
> \
> - sdt_might_sleep_start(NULL); \
> + sdt_might_sleep_start_timeout(NULL, __ret); \
[Severity: High]
Is this a regression where a signal-interrupted wait event permanently leaks
the dept wait stage tracking state?
Looking at ___swait_event(), if the task receives a signal while waiting, it
evaluates ___wait_is_interruptible(state) && __int as true and takes the
goto __out path:
if (___wait_is_interruptible(state) && __int) {
__ret = __int;
goto __out;
}
This jumps completely over the cleanup call:
finish_swait(&wq, &__wait);
sdt_might_sleep_end();
__out: __ret;
Without calling sdt_might_sleep_end() and its underlying dept_clean_stage(),
the dept state isn't cleaned up for the current task. Subsequent waits by this
task will fail to register new dependency maps and instead repeatedly commit
the stale wait map during __schedule(). This irreversibly corrupts the lock
dependency graph and triggers false deadlock reports.
The exact same bypass flaw appears to be present in ___wait_event() in
include/linux/wait.h and ___wait_var_event() in include/linux/wait_bit.h.
> INIT_LIST_HEAD(&__wait.task_list); \
> for (;;) { \
> long __int = prepare_to_swait_event(&wq, &__wait, state);\
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=17
^ permalink raw reply
* Re: [PATCH v19 01/40] dept: implement DEPT(DEPendency Tracker)
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: ojeda, tytso, Anna Schumaker, cassel, dlemoal, Jeff Layton,
dri-devel, linux-ide, linux-rt-devel, linux-kernel, Chuck Lever,
linux-modules
In-Reply-To: <20260706061928.66713-2-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Data race and Use-After-Clear on `dt->stage_m` between `dept_request_event_wait_commit()` and `dept_ttwu_stage_wait()`
- [High] Use-after-free and RCU list corruption because `dept_class` objects are freed before the RCU grace period elapses
- [High] Deadlock on PREEMPT_RT due to `local_irq_disable()` being used with a raw spinlock (`dept_pool_spin`)
--
commit 1fd2695b58a470d54e232dcaaf112ad8a807718e
Author: Byungchul Park <byungchul@sk.com>
dept: implement DEPT(DEPendency Tracker)
This commit implements DEPT (Dependency Tracker) to monitor waits and
events for deadlock detection. It addresses Lockdep's limitations
regarding read locks and cross-event scenarios.
> diff --git a/kernel/dependency/dept.c b/kernel/dependency/dept.c
> new file mode 100644
> index 0000000000000..52b9669fe0179
> --- /dev/null
> +++ b/kernel/dependency/dept.c
> @@ -0,0 +1,2977 @@
[ ... ]
> +static void dept_wq_work_fn(struct work_struct *work)
> +{
> + int i;
> +
> + for (i = 0; i < OBJECT_NR; i++) {
> + struct dept_pool *p = dept_pool + i;
> + int sz = p->tot_nr * p->obj_sz;
> + void *rpool;
> + bool need;
> +
> + local_irq_disable();
> + arch_spin_lock(&dept_pool_spin);
[Severity: High]
Is this locking pattern safe on PREEMPT_RT?
By using local_irq_disable() with a raw spinlock (dept_pool_spin), hardware
interrupts aren't disabled on PREEMPT_RT. Since DEPT instruments hardirq
paths via dept_enter(), could a hardware interrupt fire here, attempt to
acquire the exact same lock in from_pool(), and cause a hard deadlock?
Should raw_local_irq_disable() or arch_local_irq_save() be used instead?
[ ... ]
> + list_for_each_entry_safe(c, n, &dept_classes, all_node) {
> + if (!within((void *)c->key, start, sz) &&
> + !within(c->name, start, sz))
> + continue;
> +
> + hash_del_class(c);
> + disconnect_class(c);
> + list_del(&c->all_node);
> + invalidate_class(c);
> +
> + /*
> + * Actual deletion will happen on the rcu callback
> + * that has been added in disconnect_class().
> + */
> + del_class(c);
> + }
> + dept_unlock();
> + dept_exit(flags);
> +
> + /*
> + * Wait until even lockless hash_lookup_class() for the class
> + * returns NULL.
> + */
> + might_sleep();
> + synchronize_rcu();
[Severity: High]
Does this sequence safely handle RCU lifetimes for the dept_class objects?
Although the comment states actual deletion happens in the RCU callback,
disconnect_class() only queues callbacks for dept_dep objects, not the
class itself. By calling del_class(c) directly before synchronize_rcu(),
the object is pushed to the freelist synchronously via to_pool().
If a concurrent lockless RCU reader like hash_lookup_class() traverses
the bucket and reads hash_node.next after the class is reallocated,
wouldn't this result in list corruption or use-after-free crashes?
[ ... ]
> +void dept_request_event_wait_commit(void)
> +{
> + struct dept_task *dt = dept_task();
> + unsigned long flags;
> + unsigned int wg;
> + unsigned long ip;
> + const char *w_fn;
> + bool sched_map;
[ ... ]
> + wg = next_wgen();
> + WRITE_ONCE(dt->stage_m.wgen, wg);
> + arch_spin_unlock(&dt->stage_lock);
> +
> + __dept_wait(&dt->stage_m, 1UL, ip, w_fn, 0, true, sched_map);
[Severity: High]
Can this sequence lead to a use-after-clear of dt->stage_m?
Once dt->stage_lock is dropped, a concurrent wakeup can execute
dept_ttwu_stage_wait() which acquires the same stage_lock and calls
__dept_clean_stage(), clearing the original stage_m using memset().
Since __dept_wait() is called here with a reference to &dt->stage_m
after the lock is released, could it read memory that is actively being
zeroed out, resulting in a NULL pointer dereference for fields like
keys or name in check_new_class()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=1
^ permalink raw reply
* Re: [PATCH v19 11/40] dept: apply sdt_might_sleep_{start,end}() to swait
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: linux-rt-devel, dlemoal, Chuck Lever, linux-kernel, linux-ide,
ojeda, tytso, dri-devel, cassel, linux-modules, Jeff Layton,
Anna Schumaker
In-Reply-To: <20260706061928.66713-12-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `___swait_event`, `___wait_event`, and `___wait_var_event` macros skip the `sdt_might_sleep_end()` cleanup call when returning early due to a pending signal, leading to persistent DEPT state corruption that permanently disables dependency tracking for the affected task.
--
commit 7785a2fe54ced42e856f39ece5776f11bca47be8
Author: Byungchul Park <byungchul@sk.com>
dept: apply sdt_might_sleep_{start,end}() to swait
Make dept able to track dependencies by swaits.
> diff --git a/include/linux/swait.h b/include/linux/swait.h
> index d324419482a0f..277ac74f61c39 100644
> --- a/include/linux/swait.h
> +++ b/include/linux/swait.h
[ ... ]
> @@ -161,6 +162,7 @@ extern void finish_swait(struct swait_queue_head *q, struct swait_queue *wait);
> struct swait_queue __wait; \
> long __ret = ret; \
> \
> + sdt_might_sleep_start(NULL); \
> INIT_LIST_HEAD(&__wait.task_list); \
> for (;;) { \
> long __int = prepare_to_swait_event(&wq, &__wait, state);\
> @@ -176,6 +178,7 @@ extern void finish_swait(struct swait_queue_head *q, struct swait_queue *wait);
> cmd; \
> } \
> finish_swait(&wq, &__wait); \
> + sdt_might_sleep_end(); \
> __out: __ret; \
> })
[Severity: High]
Does this code bypass the sdt_might_sleep_end() cleanup when returning early
due to a pending signal?
Looking at ___swait_event(), if a task waiting interruptibly receives a
signal, prepare_to_swait_event() returns a non-zero error code.
The wait loop evaluates ___wait_is_interruptible(state) && __int as true and
executes a goto __out; which jumps completely over this new cleanup call.
If sdt_might_sleep_end() is skipped, does the task's dependency staging map
remain persistently populated, effectively causing subsequent calls to
dept_stage_wait() to silently exit early and disable DEPT tracking for that
task?
It looks like this identical sequence might also occur in the generic
waitqueue macros (___wait_event in include/linux/wait.h) and the hashed
waitqueue macros (___wait_var_event in include/linux/wait_bit.h).
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=11
^ permalink raw reply
* Re: [PATCH v19 05/40] dept: add proc knobs to show stats and dependency graph
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: linux-modules, cassel, linux-kernel, tytso, dri-devel,
Anna Schumaker, Jeff Layton, Chuck Lever, linux-rt-devel, ojeda,
linux-ide, dlemoal
In-Reply-To: <20260706061928.66713-6-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Lockless traversal of `dept_classes` and `fc->dep_head` in procfs read operations causes kernel panic due to LIST_POISON pointer dereference and Use-After-Free.
--
commit e2e753e7050a1aa0365bc001321fe4e79158b5b4
Author: Byungchul Park <byungchul@sk.com>
dept: add proc knobs to show stats and dependency graph
This commit introduces procfs knobs to expose internal stats and the dependency
graph for the dependency tracker at runtime.
> diff --git a/kernel/dependency/dept_proc.c b/kernel/dependency/dept_proc.c
> new file mode 100644
> index 0000000000000..97beaf397715f
> --- /dev/null
> +++ b/kernel/dependency/dept_proc.c
[ ... ]
> +static void *l_next(struct seq_file *m, void *v, loff_t *pos)
> +{
> + /*
> + * XXX: Serialize list traversal if needed. The following might
> + * give a wrong information on contention.
> + */
> + return seq_list_next(v, &dept_classes, pos);
> +}
> +
> +static void *l_start(struct seq_file *m, loff_t *pos)
> +{
> + /*
> + * XXX: Serialize list traversal if needed. The following might
> + * give a wrong information on contention.
> + */
> + return seq_list_start_head(&dept_classes, *pos);
> +}
[Severity: High]
Is it safe to traverse dept_classes without locks here?
If a concurrent module unload occurs, dept_free_range() calls list_del() on the
class node:
kernel/dependency/dept.c:dept_free_range() {
...
list_del(&c->all_node);
...
}
Because list_del() poisons the removed node's next pointer with LIST_POISON1,
a concurrent lockless reader in seq_list_next() will dereference LIST_POISON1
and cause a kernel panic, rather than just returning wrong information.
> +
> +static void l_stop(struct seq_file *m, void *v)
> +{
> +}
> +
> +static int l_show(struct seq_file *m, void *v)
> +{
> + struct dept_class *fc = list_entry(v, struct dept_class, all_node);
> + struct dept_dep *d;
> + const char *prefix;
> +
> + if (v == &dept_classes) {
> + seq_puts(m, "All classes:\n\n");
> + return 0;
> + }
> +
> + prefix = fc->sched_map ? "<sched> " : "";
> + seq_printf(m, "[%p] %s%s\n", (void *)fc->key, prefix, fc->name);
> +
> + /*
> + * XXX: Serialize list traversal if needed. The following might
> + * give a wrong information on contention.
> + */
> + list_for_each_entry(d, &fc->dep_head, dep_node) {
[Severity: High]
Does traversing fc->dep_head without rcu_read_lock() risk a use-after-free?
Since the dependency objects in this list are RCU-managed, traversing them
without holding rcu_read_lock() might allow the objects to be freed to the
pool while they are still being iterated.
> + struct dept_class *tc = d->wait->class;
> +
> + prefix = tc->sched_map ? "<sched> " : "";
> + seq_printf(m, " -> [%p] %s%s\n", (void *)tc->key, prefix, tc->name);
> + }
> + seq_puts(m, "\n");
> +
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=5
^ permalink raw reply
* Re: [PATCH v19 04/40] dept: tie to lockdep and IRQ tracing
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: linux-rt-devel, linux-kernel, dlemoal, cassel, tytso, Chuck Lever,
Anna Schumaker, linux-modules, Jeff Layton, ojeda, dri-devel,
linux-ide
In-Reply-To: <20260706061928.66713-5-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] dept_key_destroy() synchronously recycles dept_class objects to the lockless pool before the RCU grace period completes, causing UAF and list traversal corruption for concurrent lockless readers.
- [High] rwsem_acquire_read and rwsem_acquire incorrectly use ldt_lock(), breaking reader-writer dependency tracking and causing false-positive deadlocks.
- [High] ldt_init() is called before the `key` parameter is validated, causing DEPT to dereference a garbage pointer if `key` is NULL.
- [Medium] dept_key_init() and dept_key_destroy() are called on static keys before validation, erroneously disabling DEPT or corrupting tracking state.
- [Low] STATIC_LOCKDEP_MAP_INIT creates double-quoted strings for map names due to `#n` stringification in DEPT_MAP_INITIALIZER.
--
commit 6c1a95ee3182a2a82a1d61196d75acd387d7f3e3
Author: Byungchul Park <byungchul@sk.com>
dept: tie to lockdep and IRQ tracing
This patch integrates DEPT (Dependency Tracker) into the lockdep framework
and IRQ tracing paths. It modifies various locking primitives and lockdep
interfaces to initialize and track DEPT maps alongside lockdep maps.
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 621566345406d..60b17a2ff555f 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
[ ... ]
> @@ -428,7 +431,8 @@ enum xhlock_context_t {
> * Note that _name must not be NULL.
> */
> #define STATIC_LOCKDEP_MAP_INIT(_name, _key) \
> - { .name = (_name), .key = (void *)(_key), }
> + { .name = (_name), .key = (void *)(_key), \
> + .dmap = DEPT_MAP_INITIALIZER(_name, _key) }
[Severity: Low]
Since _name is usually passed as a string literal to
STATIC_LOCKDEP_MAP_INIT, does passing it to DEPT_MAP_INITIALIZER, which
uses the stringification operator on its first argument, result in
double-quoted strings for map names in DEPT's debug output?
[ ... ]
> -#define rwsem_acquire(l, s, t, i) lock_acquire_exclusive(l, s, t, NULL, i)
> -#define rwsem_acquire_nest(l, s, t, n, i) lock_acquire_exclusive(l, s, t, n, i)
> -#define rwsem_acquire_read(l, s, t, i) lock_acquire_shared(l, s, t, NULL, i)
> -#define rwsem_release(l, i) lock_release(l, i)
[ ... ]
> +#define rwsem_acquire(l, s, t, i) \
> +do { \
> + ldt_lock(&(l)->dmap, s, t, NULL, i); \
> + lock_acquire_exclusive(l, s, t, NULL, i); \
> +} while (0)
> +#define rwsem_acquire_nest(l, s, t, n, i) \
> +do { \
> + ldt_lock(&(l)->dmap, s, t, n, i); \
> + lock_acquire_exclusive(l, s, t, n, i); \
> +} while (0)
> +#define rwsem_acquire_read(l, s, t, i) \
> +do { \
> + ldt_lock(&(l)->dmap, s, t, NULL, i); \
> + lock_acquire_shared(l, s, t, NULL, i); \
> +} while (0)
[Severity: High]
Is it intentional that rwsem_acquire_read() uses ldt_lock() instead of
ldt_rlock(), and rwsem_acquire() uses ldt_lock() instead of ldt_wlock()?
This appears to mistakenly model a shared lock as an exclusive lock. Does
this cause DEPT to incorrectly model concurrent read locks as mutually
exclusive, leading to false-positive circular dependency (deadlock) reports
when concurrent readers take an rwsem and interact with other locks?
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 2d4c5bab5af88..dc97f2753ef81 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -1224,6 +1224,8 @@ void lockdep_register_key(struct lock_class_key *key)
> struct hlist_head *hash_head;
> struct lock_class_key *k;
> unsigned long flags;
>
> + dept_key_init(&key->dkey);
> +
> if (WARN_ON_ONCE(static_obj(key)))
> return;
[Severity: Medium]
In lockdep_register_key(), dept_key_init() is called before the
static_obj() check. If a static key is improperly passed, lockdep gracefully
rejects it by warning and returning early.
However, processing a static key in dept_key_init() triggers a DEPT_STOP
abort. Should the DEPT initialization occur after validating the key?
[ ... ]
> @@ -4940,6 +4950,8 @@ void lockdep_init_map_type(struct lockdep_map *lock, const char *name,
> {
> int i;
>
> + ldt_init(&lock->dmap, &key->dkey, subclass, name);
> +
> for (i = 0; i < NR_LOCKDEP_CACHING_CLASSES; i++)
> lock->class_cache[i] = NULL;
[Severity: High]
In lockdep_init_map_type(), ldt_init() is called before the parameter
validation checks for a NULL key further down in the function.
If key is NULL, &key->dkey yields a non-NULL offset pointer. Does DEPT accept
this garbage pointer in valid_key(), causing a kernel panic when
check_new_class() dereferences it?
[ ... ]
> @@ -6588,6 +6608,8 @@ void lockdep_unregister_key(struct lock_class_key *key)
> bool found = false;
> bool need_callback = false;
>
> + dept_key_destroy(&key->dkey);
> +
> might_sleep();
>
> if (WARN_ON_ONCE(static_obj(key)))
> return;
[Severity: Medium]
Similarly to lockdep_register_key(), dept_key_destroy() is called here
unconditionally before the static_obj() check. If a driver incorrectly calls
dynamic lock destruction on a static key, it eagerly destroys its DEPT state
while lockdep retains it. Should this also be moved after the static_obj()
check?
[Severity: Critical]
Additionally, in dept_key_destroy() called here, the function removes a class
from the RCU hash table and drops its refcount to 0 via del_class(). This
recycles the object to the DEPT pool before synchronize_rcu() is called.
Can a concurrent lockless reader in hash_lookup_class() traversing the same
bucket via hlist_for_each_entry_rcu() access this recycled object, leading to
a use-after-free and hash list corruption?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=4
^ permalink raw reply
* Re: [PATCH v19 12/40] dept: apply sdt_might_sleep_{start, end}() to waitqueue wait
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: dri-devel, linux-rt-devel, cassel, Chuck Lever, linux-kernel,
dlemoal, linux-modules, linux-ide, ojeda, Jeff Layton, tytso,
Anna Schumaker
In-Reply-To: <20260706061928.66713-13-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `sdt_might_sleep_end()` cleanup macro is bypassed when wait loops (`___wait_event`, `___swait_event`) are interrupted by a signal, permanently leaking a DEPT dependency stage.
--
commit 1300cac9c94eeb3955744b6d40ed8dce8cfc059f
Author: Byungchul Park <byungchul@sk.com>
dept: apply sdt_might_sleep_{start,end}() to waitqueue wait
Make dept able to track dependencies by waitqueue waits.
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index dce055e6add39..c8f8b44060fb9 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
[ ... ]
> @@ -305,6 +306,7 @@ extern void init_wait_entry(struct wait_queue_entry *wq_entry, int flags);
> struct wait_queue_entry __wq_entry; \
> long __ret = ret; /* explicit shadow */ \
> \
> + sdt_might_sleep_start(NULL); \
> init_wait_entry(&__wq_entry, exclusive ? WQ_FLAG_EXCLUSIVE : 0); \
> for (;;) { \
> long __int = prepare_to_wait_event(&wq_head, &__wq_entry, state);\
> @@ -323,6 +325,7 @@ extern void init_wait_entry(struct wait_queue_entry *wq_entry, int flags);
> break; \
> } \
> finish_wait(&wq_head, &__wq_entry); \
> + sdt_might_sleep_end(); \
> __out: __ret; \
> })
[Severity: High]
Does this bypass sdt_might_sleep_end() when interrupted by a signal?
Looking at ___wait_event(), if ___wait_is_interruptible(state) and __int is
set, it will take the goto __out path:
if (___wait_is_interruptible(state) && __int) { \
__ret = __int; \
goto __out; \
} \
If that path is taken, sdt_might_sleep_end() is bypassed, which permanently
leaks a DEPT dependency stage. A stale wait context remains in the task's
dependency tracker state. When the task later sleeps for unrelated reasons,
the scheduler might attribute the sleep to the leaked context, leading to
false-positive lockup splats.
Also, does this same structural issue exist in ___swait_event() in
include/linux/swait.h for simple waitqueues?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=12
^ permalink raw reply
* Re: [PATCH v19 08/40] dept: add a mechanism to refill the internal memory pools on running out
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: Chuck Lever, tytso, dri-devel, linux-kernel, linux-modules,
linux-rt-devel, Jeff Layton, cassel, Anna Schumaker, dlemoal,
ojeda, linux-ide
In-Reply-To: <20260706061928.66713-9-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Per-CPU object pools (`lpool`) grow unboundedly because objects are never returned to the global pool, leading to endless `vmalloc()` allocations and potential OOM.
- [High] Un-ratelimited printk in the pool exhaustion path causes console flooding and soft lockups.
--
(Note: The format of this report is altered to use a free-form summary rather
than inline diff quotes due to recitation restrictions.)
commit 0f35ff40c325e46d2d4adab446907928b805d0e5
Author: Byungchul Park <byungchul@sk.com>
dept: add a mechanism to refill the internal memory pools on running out
This commit introduces a mechanism to dynamically refill internal memory
pools for the DEPT engine using an IRQ work and workqueue. It adds per-CPU
local pools and a vmalloc-based refill path when the static pools are
exhausted.
[Severity: High]
Could the per-CPU local pools (lpool) grow without bounds under certain
workloads?
Looking at to_pool() and from_pool() in kernel/dependency/dept.c, objects
are unconditionally added to the per-CPU lpool without any upper bound
check or reclaim logic. If a task allocates an object (like ecxt or wait)
on CPU A, migrates to CPU B, and frees it, the object goes into CPU B's
lpool.
Because objects are never returned to the global spool when the lpool grows
large, the global pool will eventually drain. When from_pool() sees an
empty global pool, it triggers request_rpool_refill(), which allocates new
memory via vmalloc() in dept_wq_work_fn().
Will continuous cross-CPU allocation and freeing indefinitely leak memory
into local pools and repeatedly trigger vmalloc allocations, potentially
leading to an out-of-memory condition?
[Severity: High]
Is it safe to use an un-ratelimited printk in the pool exhaustion path?
In kernel/dependency/dept.c:from_pool(), when a pool is exhausted, the
code calls DEPT_INFO() (which maps to pr_warn()) to notify the user. This
happens with hardware interrupts disabled via dept_enter().
Because the pool refill in dept_wq_work_fn() happens asynchronously in a
workqueue, the pool will remain empty until the workqueue runs. Every
subsequent lock acquisition during this window will synchronously print a
multi-line message to the console.
Could this flood the console and cause soft lockups or RCU stalls since
thousands of locks might be acquired by a single thread while waiting for
the async workqueue to execute?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=8
^ permalink raw reply
* Re: [PATCH v19 10/40] dept: apply sdt_might_sleep_{start, end}() to wait_for_completion()/complete()
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: Jeff Layton, linux-kernel, linux-ide, Anna Schumaker, tytso,
dlemoal, linux-modules, Chuck Lever, dri-devel, ojeda, cassel,
linux-rt-devel
In-Reply-To: <20260706061928.66713-11-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `init_completion_map()` macro ignores the provided lockdep map argument `m` and sets `dmap` to `NULL`. This forces all generic wrappers like `flush_work()` to share a single, monolithic dependency class generated at the `wait_for_completion` macro call site.
--
commit c25e7af1e00aa68cd5900db21d080eadb8fccd71
Author: Byungchul Park <byungchul@sk.com>
dept: apply sdt_might_sleep_{start,end}() to wait_for_completion()/complete()
This commit enables the DEPT (Dependency Tracker) to track dependencies
introduced by wait_for_completion() and complete() calls. It adds a
dept_map to the completion structure and applies the appropriate sleep
start and end annotations.
> diff --git a/include/linux/completion.h b/include/linux/completion.h
> index fb29156765743..bd2c207481d68 100644
> --- a/include/linux/completion.h
> +++ b/include/linux/completion.h
[ ... ]
> @@ -26,14 +27,33 @@
> struct completion {
> unsigned int done;
> struct swait_queue_head wait;
> + struct dept_map dmap;
> };
>
> +#define init_completion(x) \
> +do { \
> + sdt_map_init(&(x)->dmap); \
> + __init_completion(x); \
> +} while (0)
> +
> +/*
> + * XXX: No use cases for now. Fill the body when needed.
> + */
> #define init_completion_map(x, m) init_completion(x)
[Severity: High]
Does this macro cause independent dependency graphs to merge? By ignoring the
lockdep map argument m and calling init_completion(x) instead, it seems
generic wrappers like flush_work() will be forced to share a single
monolithic dependency class generated at the wait site.
Could this lead DEPT to report false-positive deadlocks during normal
operations? For example, if Work A flushes Work B, and Work B flushes
Work C, both use the same flush_work() completion class. DEPT might see
Work B both waiting on and satisfying this global class, erroneously
detecting a self-cycle.
> -static inline void complete_acquire(struct completion *x) {}
> -static inline void complete_release(struct completion *x) {}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=10
^ permalink raw reply
* Re: [PATCH v19 13/40] dept: apply sdt_might_sleep_{start, end}() to hashed-waitqueue wait
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: Chuck Lever, tytso, Anna Schumaker, linux-kernel, dlemoal,
dri-devel, Jeff Layton, linux-rt-devel, ojeda, linux-modules,
cassel, linux-ide
In-Reply-To: <20260706061928.66713-14-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `sdt_might_sleep_end()` macro is bypassed when `___wait_var_event` is interrupted, leaving the DEPT tracker in an inconsistent state.
- [Medium] The `sdt_might_sleep_end()` macro is bypassed when `___wait_event` is interrupted, leaving the DEPT tracker in an inconsistent state.
--
commit 0bd2d42866fa6b852ee5317edcaf6890a90c3c65
Author: Byungchul Park <byungchul@sk.com>
dept: apply sdt_might_sleep_{start,end}() to hashed-waitqueue wait
Make dept able to track dependencies by hashed-waitqueue waits.
> diff --git a/include/linux/wait_bit.h b/include/linux/wait_bit.h
> index 9e29d79fc790a..179a616ad245a 100644
> --- a/include/linux/wait_bit.h
> +++ b/include/linux/wait_bit.h
[ ... ]
> @@ -257,6 +258,7 @@ extern wait_queue_head_t *__var_waitqueue(void *p);
> struct wait_bit_queue_entry __wbq_entry; \
> long __ret = ret; /* explicit shadow */ \
> \
> + sdt_might_sleep_start(NULL); \
> init_wait_var_entry(&__wbq_entry, var, \
> exclusive ? WQ_FLAG_EXCLUSIVE : 0); \
> for (;;) { \
> @@ -274,6 +276,7 @@ extern wait_queue_head_t *__var_waitqueue(void *p);
> cmd; \
> } \
> finish_wait(__wq_head, &__wbq_entry.wq_entry); \
> + sdt_might_sleep_end(); \
> __out: __ret; \
> })
[Severity: Medium]
If this wait is interrupted by a signal, the loop executes a goto __out
which bypasses sdt_might_sleep_end(). Does this leave the dept tracker
in an inconsistent state where the task's stage remains permanently armed?
This could cause false positive dependency tracking splats on subsequent
waits, or fail to record new dependencies because the tracker is blinded by
the stale stage.
I noticed the exact same issue is present in ___wait_event():
include/linux/wait.h:___wait_event() {
...
if (___wait_is_interruptible(state) && __int) {
__ret = __int;
goto __out;
}
...
sdt_might_sleep_end();
__out: __ret;
})
[Severity: Medium]
Does ___wait_event() also skip sdt_might_sleep_end() when interrupted?
By missing the paired cleanup call, the task's stage could remain
permanently armed here as well.
Would it make sense to move the __out label above sdt_might_sleep_end(),
or otherwise ensure the state is cleaned up on the interrupted paths for
both of these macros?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=13
^ permalink raw reply
* Re: [PATCH v19 03/40] dept: add lock dependency tracker APIs
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: dri-devel, linux-modules, cassel, linux-ide, Anna Schumaker,
tytso, linux-rt-devel, dlemoal, ojeda, Chuck Lever, Jeff Layton,
linux-kernel
In-Reply-To: <20260706061928.66713-4-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] `ldt_set_class` silently drops the lock name parameter, causing newly assigned lock classes to be registered in DEPT with the old lock name.
--
commit ca6be2708025c25889ef609a2203d1465096983d
Author: Byungchul Park <byungchul@sk.com>
dept: add lock dependency tracker APIs
Wrap the base APIs for easier annotation on typical lock.
> diff --git a/include/linux/dept_ldt.h b/include/linux/dept_ldt.h
> new file mode 100644
> index 0000000000000..8047d0a531f10
> --- /dev/null
> +++ b/include/linux/dept_ldt.h
[ ... ]
> +#define ldt_downgrade(m, i) \
> + do { \
> + if (dept_ecxt_holding(m, LDT_EVT_W)) \
> + dept_map_ecxt_modify(m, LDT_EVT_W, NULL, LDT_EVT_R, i, "downgrade", "read_unlock", -1);\
> + } while (0)
> +
> +#define ldt_set_class(m, n, k, sl, i) dept_map_ecxt_modify(m, LDT_EVT_ALL, k, 0UL, i, "lock_set_class", "(any)unlock", sl)
[Severity: Medium]
Does this code inadvertently drop the lock name parameter `n`?
When a caller invokes `lock_set_class` on a held lock, it passes the new
name `n`. Because `n` is completely omitted from the replacement list here,
`dept_map_ecxt_modify` won't receive the new name. This could cause newly
assigned lock classes to be registered in DEPT with the old lock name, which
might lead to incorrect lock names in dependency deadlock reports.
Inside `dept_map_ecxt_modify()`, `dept_map_reinit()` is called with `NULL`
for the name, leaving `m->name` unchanged. `check_new_class()` is then
called, which registers the new class key using the old `m->name`. Lockdep
subsequently updates `m->name` via `ldt_init`, but it is too late as the
DEPT class was already created with the old name.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=3
^ permalink raw reply
* Re: [PATCH v19 22/40] dept: track PG_locked with dept
From: Byungchul Park @ 2026-07-07 2:35 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-kernel, max.byungchul.park, kernel_team, torvalds,
damien.lemoal, linux-ide, adilger.kernel, linux-ext4, mingo,
peterz, will, tglx, rostedt, joel, sashal, daniel.vetter,
duyuyang, johannes.berg, tj, tytso, david, amir73il, gregkh,
kernel-team, linux-mm, akpm, mhocko, minchan, hannes,
vdavydov.dev, sj, jglisse, dennis, cl, penberg, rientjes, vbabka,
ngupta, linux-block, josef, linux-fsdevel, jack, jlayton,
dan.j.williams, hch, djwong, dri-devel, rodrigosiqueiramelo,
melissa.srw, hamohammed.sa, harry.yoo, chris.p.wilson,
gwan-gyeong.mun, boqun.feng, longman, yunseong.kim, ysk,
yeoreum.yun, netdev, matthew.brost, her0gyugyu, corbet,
catalin.marinas, bp, x86, hpa, luto, sumit.semwal, gustavo,
christian.koenig, andi.shyti, arnd, lorenzo.stoakes, Liam.Howlett,
rppt, surenb, mcgrof, petr.pavlu, da.gomez, samitolvanen, paulmck,
frederic, neeraj.upadhyay, joelagnelf, josh, urezki,
mathieu.desnoyers, jiangshanlai, qiang.zhang, juri.lelli,
vincent.guittot, dietmar.eggemann, bsegall, mgorman, vschneid,
chuck.lever, neil, okorniev, Dai.Ngo, tom, trondmy, anna, kees,
bigeasy, clrkwllms, mark.rutland, ada.coupriediaz,
kristina.martsenko, wangkefeng.wang, broonie, kevin.brodsky, dwmw,
shakeel.butt, ast, ziy, yuzhao, baolin.wang, usamaarif642,
joel.granados, richard.weiyang, geert+renesas, tim.c.chen, linux,
alexander.shishkin, lillian, chenhuacai, francesco,
guoweikang.kernel, link, jpoimboe, masahiroy, brauner,
thomas.weissschuh, oleg, mjguzik, andrii, wangfushuai, linux-doc,
linux-arm-kernel, linux-media, linaro-mm-sig, linux-i2c,
linux-arch, linux-modules, rcu, linux-nfs, linux-rt-devel,
2407018371, dakr, miguel.ojeda.sandonis, neilb, bagasdotme,
wsa+renesas, dave.hansen, geert, ojeda, alex.gaynor, gary,
bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, rust-for-linux
In-Reply-To: <akvueAxPl8aoLvMR@casper.infradead.org>
On Mon, Jul 06, 2026 at 07:05:44PM +0100, Matthew Wilcox wrote:
> On Mon, Jul 06, 2026 at 03:19:10PM +0900, Byungchul Park wrote:
> > Makes dept able to track PG_locked waits and events, which will be
> > useful in practice. See the following link that shows dept worked with
> > PG_locked and detected real issues in practice:
> >
> > https://lore.kernel.org/lkml/1674268856-31807-1-git-send-email-byungchul.park@lge.com/
>
> > @@ -219,6 +220,7 @@ struct page {
> > struct page *kmsan_shadow;
> > struct page *kmsan_origin;
> > #endif
> > + struct dept_ext_wgen pg_locked_wgen;
> > } _struct_page_alignment;
>
> I may not understand this quite correctly, but I think that tracking
> PG_locked dependencies in the struct page has both false positive and
> false negative problems.
>
> Imagine we have a file mapping M1 containing folio F1 at index 0 and F2
> at index 1. It is correct locking order to lock F1 before locking F2
> (for example when doing writeback). Later, M1 has its folios reclaimed
> and returned to the free pool. Then each is added to mapping M2, this
> time with folio F2 at index 8 and F1 at index 9. Now the correct order
> to lock these folios in the order F2 followed by F1.
First of all, I appreciate your feedback. Thanks!
That case doesn't generate any dependency unless any other waits are
involved in. That should be handled in xxx_nested manner e.g.
folio_lock_nested() that I need to introduce. The work is in progress.
> I don't see a part of this patch where we clear pg_locked_wgen when the
> page is returned to the page allocator. Maybe I missed that.
You are right. pg_locked_wgen doesn't get cleared. However, DEPT works
this way:
folio_lock()
wait_for_pg_locked_cleared()
set_pg_locked() // (1) update pg_locked_wgen to the current wgen
... // there might be other waits
folio_unlock()
clear_pg_locked() // (2) check if there have been any waits since (1)
In other words, it's guranteed that pg_locked_wgen has been updated e.i.
(1) when DEPT refers to pg_locked_wgen e.i. (2). So I don't think it's
a problem.
> I think we should be tracking PG_locked dependencies in the owner
> of the folio. For files, that would be in the struct address_space.
> For anon memory, I think that's in the anon_vma, but if somebody told
> me it was in some other structure, I wouldn't argue with them.
I think it's a good point but it's a classification issue. folios owned
by struct address_space should be classified to e.g. address_space_class
and ones owned by struct anon_vma should be classified to e.g.
anon_vma_class. I will work on it to apply the insight you just gave
but better do it as follow-up patches since the initial patchset is
already too big to get reviewed.
> This requires slightly more complexity than lockdep currently has.
> We don't want to use a lockdep class for each folio, obviously. So we
> need something to say "I already have folio F1 locked, is it OK to lock
From DEPT's perspective, folio_lock(F1) and folio_lock(F2) are waits and
folio_unlock(F1) and folio_unlock(F2) are events. Since DEPT tracks
dependencies with specified classes between waits and events, DEPT's
interest in the following example is to detect a situation like:
< context X >
folio_lock(address_space_class'ed F1)
...
folio_lock(anon_vma_class'ed F2)
...
folio_unlock(anon_vma_class'ed F2)
...
folio_unlock(address_space_class'ed F1)
< context Y >
folio_lock(anon_vma_class'ed any folio)
...
folio_lock(address_space_class'ed any folio)
...
folio_unlock(address_space_class'ed any folio)
...
folio_unlock(anon_vma_class'ed any folio)
However, the following pattern should be manually annotated by
developers like using folio_lock_nested() or something. DEPT cannot
work with it automatically:
folio_lock(address_space_class'ed F1)
...
folio_lock(address_space_class'ed F2)
...
folio_unlock(address_space_class'ed F2)
...
folio_unlock(address_space_class'ed F1)
or
folio_lock(anon_vma_class'ed F1)
...
folio_lock(anon_vma_class'ed F2)
...
folio_unlock(anon_vma_class'ed F2)
...
folio_unlock(anon_vma_class'ed F1)
These should be explicitly annotated by developers if it's intended:
folio_lock(address_space_class'ed F1)
...
folio_lock_nested(address_space_class'ed F2)
...
folio_unlock(address_space_class'ed F2)
...
folio_unlock(address_space_class'ed F1)
or
folio_lock(anon_vma_class'ed F1)
...
folio_lock_nested(anon_vma_class'ed F2)
...
folio_unlock(anon_vma_class'ed F2)
...
folio_unlock(anon_vma_class'ed F1)
> folio F2?". Essentially figuring out how we can track all folios in a
> given mapping the same way, and making sure that we don't deadlock on
> folios in the same mapping.
At the moment, as I told you, DEPT cannot work with dependencies between
the same class'ed folios. However, it'd be much better if DEPT can work
with even those cases. Could you provide a scenario where a deadlock
happens between the same class'ed ones? Any idea how to detect for the
cases?
> If F1 and F2 are in different mappings, it's not a deadlock if F1 is in a
> filesystem mapping and F2 is in its backing dev. It's also not a deadlock
> if F1 and F2 are both filesystem folios and the inodes are both locked.
> See vfs_lock_two_folios() in fs/remap_range.c.
Yeah.. DEPT is a tracker to track dependencies between waits and events
even across different contexts, but not a magic unfortunately. That
lock ordering issue - with the same class'ed ones - should be resolved
in the manual manner as vfs_lock_two_folios() does.
> I have much less knowledge about anonymous memory locking order.
> Maybe it doesn't happen. Or about locking one anon and one file folio.
> For slab memory, we don't sleep on PG_locked (it's used as a spinlock bit).
> For other kinds of memory ... I don't know. Page migration is fun.
Anyway, the sophisticated classification you mentioned is necessary for
DEPT to be better especially for folio locking mechanism.
Thanks again!
Byungchul
^ permalink raw reply
* Re: [PATCH v19 22/40] dept: track PG_locked with dept
From: Matthew Wilcox @ 2026-07-06 18:05 UTC (permalink / raw)
To: Byungchul Park
Cc: linux-kernel, max.byungchul.park, kernel_team, torvalds,
damien.lemoal, linux-ide, adilger.kernel, linux-ext4, mingo,
peterz, will, tglx, rostedt, joel, sashal, daniel.vetter,
duyuyang, johannes.berg, tj, tytso, david, amir73il, gregkh,
kernel-team, linux-mm, akpm, mhocko, minchan, hannes,
vdavydov.dev, sj, jglisse, dennis, cl, penberg, rientjes, vbabka,
ngupta, linux-block, josef, linux-fsdevel, jack, jlayton,
dan.j.williams, hch, djwong, dri-devel, rodrigosiqueiramelo,
melissa.srw, hamohammed.sa, harry.yoo, chris.p.wilson,
gwan-gyeong.mun, boqun.feng, longman, yunseong.kim, ysk,
yeoreum.yun, netdev, matthew.brost, her0gyugyu, corbet,
catalin.marinas, bp, x86, hpa, luto, sumit.semwal, gustavo,
christian.koenig, andi.shyti, arnd, lorenzo.stoakes, Liam.Howlett,
rppt, surenb, mcgrof, petr.pavlu, da.gomez, samitolvanen, paulmck,
frederic, neeraj.upadhyay, joelagnelf, josh, urezki,
mathieu.desnoyers, jiangshanlai, qiang.zhang, juri.lelli,
vincent.guittot, dietmar.eggemann, bsegall, mgorman, vschneid,
chuck.lever, neil, okorniev, Dai.Ngo, tom, trondmy, anna, kees,
bigeasy, clrkwllms, mark.rutland, ada.coupriediaz,
kristina.martsenko, wangkefeng.wang, broonie, kevin.brodsky, dwmw,
shakeel.butt, ast, ziy, yuzhao, baolin.wang, usamaarif642,
joel.granados, richard.weiyang, geert+renesas, tim.c.chen, linux,
alexander.shishkin, lillian, chenhuacai, francesco,
guoweikang.kernel, link, jpoimboe, masahiroy, brauner,
thomas.weissschuh, oleg, mjguzik, andrii, wangfushuai, linux-doc,
linux-arm-kernel, linux-media, linaro-mm-sig, linux-i2c,
linux-arch, linux-modules, rcu, linux-nfs, linux-rt-devel,
2407018371, dakr, miguel.ojeda.sandonis, neilb, bagasdotme,
wsa+renesas, dave.hansen, geert, ojeda, alex.gaynor, gary,
bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, rust-for-linux
In-Reply-To: <20260706061928.66713-23-byungchul@sk.com>
On Mon, Jul 06, 2026 at 03:19:10PM +0900, Byungchul Park wrote:
> Makes dept able to track PG_locked waits and events, which will be
> useful in practice. See the following link that shows dept worked with
> PG_locked and detected real issues in practice:
>
> https://lore.kernel.org/lkml/1674268856-31807-1-git-send-email-byungchul.park@lge.com/
> @@ -219,6 +220,7 @@ struct page {
> struct page *kmsan_shadow;
> struct page *kmsan_origin;
> #endif
> + struct dept_ext_wgen pg_locked_wgen;
> } _struct_page_alignment;
I may not understand this quite correctly, but I think that tracking
PG_locked dependencies in the struct page has both false positive and
false negative problems.
Imagine we have a file mapping M1 containing folio F1 at index 0 and F2
at index 1. It is correct locking order to lock F1 before locking F2
(for example when doing writeback). Later, M1 has its folios reclaimed
and returned to the free pool. Then each is added to mapping M2, this
time with folio F2 at index 8 and F1 at index 9. Now the correct order
to lock these folios in the order F2 followed by F1.
I don't see a part of this patch where we clear pg_locked_wgen when the
page is returned to the page allocator. Maybe I missed that.
I think we should be tracking PG_locked dependencies in the owner
of the folio. For files, that would be in the struct address_space.
For anon memory, I think that's in the anon_vma, but if somebody told
me it was in some other structure, I wouldn't argue with them.
This requires slightly more complexity than lockdep currently has.
We don't want to use a lockdep class for each folio, obviously. So we
need something to say "I already have folio F1 locked, is it OK to lock
folio F2?". Essentially figuring out how we can track all folios in a
given mapping the same way, and making sure that we don't deadlock on
folios in the same mapping.
If F1 and F2 are in different mappings, it's not a deadlock if F1 is in a
filesystem mapping and F2 is in its backing dev. It's also not a deadlock
if F1 and F2 are both filesystem folios and the inodes are both locked.
See vfs_lock_two_folios() in fs/remap_range.c.
I have much less knowledge about anonymous memory locking order.
Maybe it doesn't happen. Or about locking one anon and one file folio.
For slab memory, we don't sleep on PG_locked (it's used as a spinlock bit).
For other kinds of memory ... I don't know. Page migration is fun.
^ permalink raw reply
* Re: [PATCH v6 4/4] kallsyms: add KUnit tests for lineinfo feature
From: sashiko-bot @ 2026-07-06 14:45 UTC (permalink / raw)
To: Sasha Levin; +Cc: linux-modules
In-Reply-To: <20260706142914.2398993-5-sashal@kernel.org>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Test failures on architectures using function descriptors
- [Medium] Silently failing tests due to missing Kconfig dependency for module lineinfo
- [Medium] Test flakiness due to incorrect assumption about compiler optimization and function size
- [Medium] Missing NULL pointer check for test buffer allocation leading to potential kernel panic
--
commit 799168a56f108698328a395b2e257d42cad0b5d4
Author: Sasha Levin <sashal@kernel.org>
kallsyms: add KUnit tests for lineinfo feature
Add a KUnit test module (CONFIG_LINEINFO_KUNIT_TEST) that verifies the
kallsyms lineinfo feature produces correct source file:line annotations
in stack traces.
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 1244dcac2294a..257b5ca442f32 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
[ ... ]
> @@ -3092,6 +3092,16 @@ config LONGEST_SYM_KUNIT_TEST
>
> If unsure, say N.
>
> +config LINEINFO_KUNIT_TEST
> + tristate "KUnit tests for kallsyms lineinfo" if !KUNIT_ALL_TESTS
> + depends on KUNIT && KALLSYMS_LINEINFO
[Severity: Medium]
If this test is built as a loadable module and CONFIG_KALLSYMS_LINEINFO_MODULES
is disabled, will the module lineinfo tests fail at runtime? Should this Kconfig
also depend on KALLSYMS_LINEINFO_MODULES when built as a module, or should the
test suite gracefully skip module-specific tests if that configuration is not
enabled?
> diff --git a/lib/tests/lineinfo_kunit.c b/lib/tests/lineinfo_kunit.c
> new file mode 100644
> index 0000000000000..285d798cb6a3c
> --- /dev/null
> +++ b/lib/tests/lineinfo_kunit.c
[ ... ]
> +static char *alloc_sym_buf(struct kunit *test)
> +{
> + return kunit_kzalloc(test, KSYM_SYMBOL_LEN, GFP_KERNEL);
> +}
[Severity: Medium]
Can kunit_kzalloc() return NULL here and cause a kernel panic when callers
unconditionally pass the buffer to sprint_with_lineinfo()? Would it be better
to add a KUNIT_ASSERT_NOT_NULL() check here or validate the return value in
the callers?
[ ... ]
> +static noinline int lineinfo_target_many_lines(void)
> +{
> + int a = 0;
> +
> + barrier();
> + a += 1;
> + a += 2;
[ ... ]
> + a += 10;
> + barrier();
> + return a;
> +}
[Severity: Medium]
Is it possible that standard compiler optimizations will fold the arithmetic
in this function away, causing it to compile to fewer than 8 bytes?
If that happens, wouldn't the test_many_lines_mid_function() test below fail
because it adds 8 bytes to the start address and ends up looking outside the
function bounds? Should WRITE_ONCE() or inline assembly be used here to
prevent the arithmetic from being optimized out?
[ ... ]
> +static void test_normal_function(struct kunit *test)
> +{
> + char *buf = alloc_sym_buf(test);
> + unsigned long addr = (unsigned long)lineinfo_target_normal;
[Severity: Medium]
On architectures that use function descriptors (like PowerPC ELFv1 or ia64),
will casting lineinfo_target_normal directly to unsigned long yield a data
descriptor address instead of the instruction pointer?
Could this cause sprint_backtrace() to fail to resolve the symbol, breaking
the tests unconditionally on those architectures? Does this require using
dereference_symbol_descriptor() to get the actual instruction pointer?
[ ... ]
> +static void test_pBb_format(struct kunit *test)
> +{
> + char *buf = alloc_sym_buf(test);
> + /*
> + * %pBb uses sprint_backtrace_build_id which subtracts 1 from the
> + * address, so pass addr+1 to resolve back to the function.
> + */
> + void *addr = (void *)((unsigned long)lineinfo_target_normal + 1);
[Severity: Medium]
Similarly, adding 1 to a function descriptor pointer instead of an instruction
pointer creates a corrupt address. Will this cause snprintf() with %pBb to
fail to format the backtrace correctly on architectures using function
descriptors?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706142914.2398993-1-sashal@kernel.org?part=4
^ permalink raw reply
* Re: [PATCH v6 1/4] kallsyms: embed source file:line info in kernel stack traces
From: sashiko-bot @ 2026-07-06 14:44 UTC (permalink / raw)
To: Sasha Levin; +Cc: linux-modules
In-Reply-To: <20260706142914.2398993-2-sashal@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Assembly routines and other un-annotated symbols will be falsely annotated with the file and line number of the preceding C function because the binary search does not restrict lookups to the boundaries of the resolved symbol.
- [Low] Unchecked `strdup` return value causes a segmentation fault on memory allocation failure in the `gen_lineinfo` host tool.
--
commit 8ad0e66e84394fea4bf24efbedda7eb50bbe8269
Author: Sasha Levin <sashal@kernel.org>
kallsyms: embed source file:line info in kernel stack traces
This commit adds CONFIG_KALLSYMS_LINEINFO to embed an address-to-line
lookup table in the kernel image. A new host tool extracts DWARF data
at build time, allowing stack traces to directly include source file and
line number information.
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index aec2f06858afd..1e3f527b13988 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -497,6 +544,23 @@ static int __sprint_symbol(char *buffer, unsigned long address,
> len += sprintf(buffer + len, "]");
> }
>
> + /*
> + * Append "(file:line)" only for stack-backtrace consumers. Plain
> + * sprint_symbol() backs %ps, and many existing format strings tack
> + * literal "()" after %ps to indicate a function call ("foo()
> + * replaced with bar()"); appending lineinfo there would produce a
> + * confusing "foo (file:line)()".
> + */
> + if (add_lineinfo && IS_ENABLED(CONFIG_KALLSYMS_LINEINFO) && !modname) {
> + const char *li_file;
> + unsigned int li_line;
> +
> + if (kallsyms_lookup_lineinfo(address,
> + &li_file, &li_line))
[Severity: Medium]
Will this falsely annotate assembly routines or un-annotated symbols with
the file and line number of the preceding C function?
The caller has access to the resolved symbol's size and offset but fails to
pass this bounding information to kallsyms_lookup_lineinfo(). The binary
search in kallsyms_lookup_lineinfo() does not restrict lookups to the
boundaries of the resolved symbol, finding the closest preceding entry.
Since gen_lineinfo explicitly ignores DW_LNE_end_sequence markers, could
this regression cause the search to bleed across symbol boundaries?
> + len += snprintf(buffer + len, KSYM_SYMBOL_LEN - len,
> + " (%s:%u)", li_file, li_line);
> + }
> +
> return len;
> }
> diff --git a/scripts/gen_lineinfo.c b/scripts/gen_lineinfo.c
> new file mode 100644
> index 0000000000000..699e760178f09
> --- /dev/null
> +++ b/scripts/gen_lineinfo.c
> @@ -0,0 +1,557 @@
[ ... ]
> + if (num_files >= files_capacity) {
> + files_capacity = files_capacity ? files_capacity * 2 : 4096;
> + files = realloc(files, files_capacity * sizeof(*files));
> + if (!files) {
> + fprintf(stderr, "out of memory\n");
> + exit(1);
> + }
> + }
> + files[num_files].name = strdup(name);
[Severity: Low]
Does this code handle memory allocation failures?
strdup() can return NULL on failure, which would be assigned directly to the
files array here and could cause a segmentation fault when later dereferenced
via strlen().
> + files[num_files].id = num_files;
> +
> + /* Insert into hash table (points to files[] entry) */
> + file_hash[h].name = files[num_files].name;
> + file_hash[h].id = num_files;
> +
> + num_files++;
> + return num_files - 1;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706142914.2398993-1-sashal@kernel.org?part=1
^ permalink raw reply
* Re: [PATCH v6 2/4] kallsyms: extend lineinfo to loadable modules
From: sashiko-bot @ 2026-07-06 14:41 UTC (permalink / raw)
To: Sasha Levin; +Cc: linux-modules
In-Reply-To: <20260706142914.2398993-3-sashal@kernel.org>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] `apply_debug_line_relocations()` ignores `.rel.debug_line` (SHT_REL), breaking module lineinfo on 32-bit architectures like i386 and ARM.
- [High] Misattribution of DWARF line entries for unhandled text sections pollutes `.text` lineinfo.
- [High] Hardcoded assembly padding creates an ABI mismatch with the C struct layout on architectures where `alignof(u64) == 4` (e.g., 32-bit x86).
- [Medium] An integer overflow in relocation offset validation allows an out-of-bounds write in the `gen_lineinfo` build tool.
--
commit a6a5ac22ccec63c8ea45f4ded1db6418cd100063
Author: Sasha Levin <sashal@kernel.org>
Subject: kallsyms: extend lineinfo to loadable modules
This commit extends the CONFIG_KALLSYMS_LINEINFO feature to loadable kernel
modules. It post-processes modules at build time to generate per-section
.mod_lineinfo binary tables mapping addresses to source lines. The tables
are embedded in the module and searched during stack traces.
> diff --git a/scripts/gen_lineinfo.c b/scripts/gen_lineinfo.c
> index 699e760178f09..e1e08469b4f2f 100644
> --- a/scripts/gen_lineinfo.c
> +++ b/scripts/gen_lineinfo.c
[ ... ]
> + /* Find the relevant sections */
> + while ((scn = elf_nextscn(elf, scn)) != NULL) {
> + const char *name;
> +
> + if (!gelf_getshdr(scn, &shdr))
> + continue;
> + name = elf_strptr(elf, shstrndx, shdr.sh_name);
> + if (!name)
> + continue;
> +
> + if (!strcmp(name, ".debug_line"))
> + debug_line_scn = scn;
> + else if (!strcmp(name, ".rela.debug_line"))
> + rela_debug_line_scn = scn;
> + else if (shdr.sh_type == SHT_SYMTAB)
> + symtab_scn = scn;
> + }
> +
> + if (!debug_line_scn || !rela_debug_line_scn || !symtab_scn)
> + return;
[Severity: High]
Does apply_debug_line_relocations() skip .rel.debug_line sections?
On 32-bit architectures like i386 and ARM, relocations use SHT_REL and
generate a .rel.debug_line section rather than .rela.debug_line.
If the function bails out here, synthetic biases are never added and
DW_FORM_line_strp offsets remain un-relocated, which feeds zeros to libdw.
Could this break the module lineinfo feature entirely on these architectures?
[ ... ]
> + if (module_mode) {
> + struct covered_section *cs;
> +
> + cs = section_by_index(all_sections, ALL_SECTIONS,
> + sym.st_shndx);
> + if (cs)
> + value += cs->bias;
> + }
[Severity: High]
Could unbiased text sections pollute the .text lineinfo?
Sections not listed in all_sections[], such as .noinstr.text, do not
receive a synthetic bias here, leaving their relocated DWARF PCs starting at 0.
When classify_address() processes these entries:
classify_address() {
...
if (addr < sections[i].bias)
continue;
if (addr >= sections[i].bias + sections[i].size)
continue;
...
}
An address smaller than the size of .text will fall into the .text bucket,
since .text has a bias of 0. Would this incorrectly merge line entries and
corrupt the binary search invariant?
[ ... ]
> + if (!is_abs64)
> + value &= 0xffffffffULL;
> +
> + if (rela.r_offset + width <= dl_data->d_size)
> + elf_write_uint((unsigned char *)dl_data->d_buf +
> + rela.r_offset,
> + value, width, target_le);
> + }
[Severity: Medium]
Is there a risk of an integer overflow in this bounds check?
If a maliciously crafted ELF file provides a huge r_offset value from the
relocation section (such as UINT64_MAX), the addition of width could wrap
around to a small positive integer and bypass the check. This might lead to
an out-of-bounds write in elf_write_uint().
[ ... ]
> +static void emit_blob(const char *output_section,
> + const char *blob_tag,
> + enum mod_lineinfo_blob blob)
> +{
[ ... ]
> + printf("\t.section %s, \"a\"\n\n", output_section);
> +
> + printf("\t.balign 8\n");
> + printf(".Lroot_%s:\n", blob_tag);
> + printf("\t.long %u\t\t/* num_sections */\n", active);
> + /* Pad to align the u64 anchor in sections[0] to 8 bytes. */
> + printf("\t.balign 8\n");
[Severity: High]
Does this hardcoded padding create an ABI mismatch with the mod_lineinfo_root
C struct on some architectures?
The assembly forces an 8-byte alignment immediately following the 4-byte
num_sections field. However, on 32-bit x86 architectures, the System V ABI
sets alignof(u64) to 4.
Looking at include/linux/mod_lineinfo.h:
struct mod_lineinfo_root {
u32 num_sections;
struct mod_lineinfo_section sections[];
};
The C compiler will align the struct to 4 bytes, meaning the sections[]
flexible array starts at offset 4, not offset 8. If the kernel reads this
blob, could it parse the assembly padding as the start of the anchor field
and corrupt the section descriptors?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706142914.2398993-1-sashal@kernel.org?part=2
^ permalink raw reply
* Re: [PATCH v6 3/4] kallsyms: delta-compress lineinfo tables for ~2.7x size reduction
From: sashiko-bot @ 2026-07-06 14:40 UTC (permalink / raw)
To: Sasha Levin; +Cc: linux-modules
In-Reply-To: <20260706142914.2398993-4-sashal@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] lineinfo_read_sleb128 contains a signed left shift that can overflow on malformed or large data, triggering a UBSAN splat in NMI/panic contexts.
--
commit 3756e632dbdf4d6db79f0e2c8a39f6103ac6e8ad
Author: Sasha Levin <sashal@kernel.org>
kallsyms: delta-compress lineinfo tables for ~2.7x size reduction
This patch replaces the flat uncompressed parallel arrays for lineinfo with a
block-indexed, delta-encoded, ULEB128/SLEB128 varint compressed format to
significantly reduce size. Entries are grouped into blocks of 64 and decoded
entirely on the stack to ensure safety during NMI or panic contexts.
> diff --git a/include/linux/mod_lineinfo.h b/include/linux/mod_lineinfo.h
> index 9cda3263a0784..a3c7143433020 100644
> --- a/include/linux/mod_lineinfo.h
> +++ b/include/linux/mod_lineinfo.h
[ ... ]
> +/* Read an SLEB128 varint. Same safety guarantees as above. */
> +static inline int32_t lineinfo_read_sleb128(const u8 *data, u32 *pos, u32 end)
> +{
> + int32_t result = 0;
> + unsigned int shift = 0;
> + u8 byte = 0;
> +
> + while (*pos < end) {
> + byte = data[*pos];
> + (*pos)++;
> + result |= (int32_t)(byte & 0x7f) << shift;
[Severity: High]
Can this signed left shift trigger a UBSAN splat on malformed or large data?
For the 5th byte of an SLEB128 varint, shift will be 28. If the decoded byte
has a value greater than 7 (e.g. 0x7F), 127 << 28 evaluates to 3409130496,
which exceeds INT32_MAX.
Since left-shifting into or past the sign bit of a signed integer is
undefined behavior in C, a corrupted or maliciously crafted .mod_lineinfo
section could trigger a CONFIG_UBSAN_SHIFT splat.
If this function is called by lineinfo_search() during an NMI or panic
context, would the resulting UBSAN splat attempt to take locks and call
printk, potentially causing a deadlock or nested panic?
Perhaps the shift should be performed as unsigned:
result |= (int32_t)((u32)(byte & 0x7f) << shift);
> + shift += 7;
> + if (!(byte & 0x80))
> + break;
> + if (shift >= 32) {
> + while (*pos < end && (data[*pos] & 0x80))
> + (*pos)++;
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706142914.2398993-1-sashal@kernel.org?part=3
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox