* Re: [PATCH 3/4] tpm_crb_ffa: Include linux/slab.h for kzalloc_obj() and kfree()
From: sashiko-bot @ 2026-07-07 15:43 UTC (permalink / raw)
To: Petr Pavlu
Cc: kvm, Heiko Carstens, linux-modules, Alexander Gordeev, linux-s390,
Vasily Gorbik
In-Reply-To: <20260707153819.4172541-4-petr.pavlu@suse.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The commit message incorrectly claims that the file uses `kzalloc_obj()` instead of `kzalloc()`.
--
--- Patch [3]: [PATCH 3/4] tpm_crb_ffa: Include linux/slab.h for kzalloc_obj() and kfree() ---
commit a56e4cf89267d12126bd70f7981a05c846959016
Author: Petr Pavlu <petr.pavlu@suse.com>
tpm_crb_ffa: Include linux/slab.h for kzalloc_obj() and kfree()
This commit adds the missing linux/slab.h inclusion to tpm_crb_ffa.c.
It removes the reliance on indirect inclusion through linux/module.h.
This prepares for the eventual removal of linux/elf.h from linux/module.h.
> tpm_crb_ffa: Include linux/slab.h for kzalloc_obj() and kfree()
[Severity: Low]
This isn't a bug, but there appears to be a typo in the commit subject.
Did you mean kzalloc() instead of kzalloc_obj()? Looking at
tpm_crb_ffa_probe() in the driver, it correctly calls kzalloc().
> The drivers/char/tpm/tpm_crb_ffa.c file uses kzalloc_obj() and kfree(),
[Severity: Low]
This isn't a bug, but the same typo is also present here in the commit
description.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260707153819.4172541-1-petr.pavlu@suse.com?part=3
^ permalink raw reply
* [PATCH 4/4] module: Limit ELF includes in linux/module.h to uapi/linux/elf.h
From: Petr Pavlu @ 2026-07-07 15:37 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Peter Huewe,
Jarkko Sakkinen, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen
Cc: Xin Li, H. Peter Anvin, Andy Lutomirski, Christian Borntraeger,
Sven Schnelle, Janosch Frank, Claudio Imbrenda, David Hildenbrand,
Jason Gunthorpe, Aaron Tomlin, linux-s390, kvm, linux-integrity,
linux-modules, linux-kernel
In-Reply-To: <20260707153819.4172541-1-petr.pavlu@suse.com>
The linux/module.h header is included by every *.mod.c file to provide
`struct module` and other related definitions. This makes it a "hot"
header, so it should avoid pulling in unnecessary definitions.
The header currently includes linux/elf.h but this is largely unnecessary
because it requires only the base ELF structures, such as Elf_Ehdr and
Elf_Sym, which are declared in uapi/linux/elf.h.
Replace the linux/elf.h include with uapi/linux/elf.h. On x86_64_defconfig,
including linux/module.h alone results in 2.2 MB of preprocessed data. This
change reduces that to 1.5 MB.
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
include/linux/module.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 7566815fabbe..7661faadbedd 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -17,7 +17,6 @@
#include <linux/cleanup.h>
#include <linux/kmod.h>
#include <linux/init.h>
-#include <linux/elf.h>
#include <linux/stringify.h>
#include <linux/kobject.h>
#include <linux/moduleparam.h>
@@ -29,8 +28,9 @@
#include <linux/srcu.h>
#include <linux/static_call_types.h>
#include <linux/dynamic_debug.h>
-
#include <linux/percpu.h>
+
+#include <uapi/linux/elf.h>
#include <asm/module.h>
#define MODULE_NAME_LEN __MODULE_NAME_LEN
--
2.54.0
^ permalink raw reply related
* [PATCH 3/4] tpm_crb_ffa: Include linux/slab.h for kzalloc_obj() and kfree()
From: Petr Pavlu @ 2026-07-07 15:37 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Peter Huewe,
Jarkko Sakkinen, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen
Cc: Xin Li, H. Peter Anvin, Andy Lutomirski, Christian Borntraeger,
Sven Schnelle, Janosch Frank, Claudio Imbrenda, David Hildenbrand,
Jason Gunthorpe, Aaron Tomlin, linux-s390, kvm, linux-integrity,
linux-modules, linux-kernel
In-Reply-To: <20260707153819.4172541-1-petr.pavlu@suse.com>
The drivers/char/tpm/tpm_crb_ffa.c file uses kzalloc_obj() and kfree(),
which are declared in linux/slab.h. It currently relies on this header
being included indirectly through linux/module.h -> linux/elf.h. Add the
missing include in preparation for removing the linux/elf.h include from
linux/module.h.
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
drivers/char/tpm/tpm_crb_ffa.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
index 025c4d4b17ca..af4466cdc297 100644
--- a/drivers/char/tpm/tpm_crb_ffa.c
+++ b/drivers/char/tpm/tpm_crb_ffa.c
@@ -12,6 +12,7 @@
#include <linux/arm_ffa.h>
#include <linux/delay.h>
#include <linux/moduleparam.h>
+#include <linux/slab.h>
#include "tpm_crb_ffa.h"
static unsigned int busy_timeout_ms = 2000;
--
2.54.0
^ permalink raw reply related
* [PATCH 2/4] s390: Avoid indirect includes through linux/module.h -> linux/elf.h
From: Petr Pavlu @ 2026-07-07 15:37 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Peter Huewe,
Jarkko Sakkinen, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen
Cc: Xin Li, H. Peter Anvin, Andy Lutomirski, Christian Borntraeger,
Sven Schnelle, Janosch Frank, Claudio Imbrenda, David Hildenbrand,
Jason Gunthorpe, Aaron Tomlin, linux-s390, kvm, linux-integrity,
linux-modules, linux-kernel
In-Reply-To: <20260707153819.4172541-1-petr.pavlu@suse.com>
Several s390 files use declarations that are provided indirectly through
linux/module.h -> linux/elf.h:
* arch/s390/kernel/nospec-branch.c: s390_kernel_write() -> linux/uaccess.h,
* drivers/s390/char/uvdevice.c: copy_to_user(), copy_from_user(),
put_user(), get_user() -> linux/uaccess.h; file_operations -> linux/fs.h.
Add the missing includes to these files in preparation for removing the
linux/elf.h include from linux/module.h.
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
arch/s390/kernel/nospec-branch.c | 1 +
drivers/s390/char/uvdevice.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/arch/s390/kernel/nospec-branch.c b/arch/s390/kernel/nospec-branch.c
index e11ec15960a1..b62da9e81e79 100644
--- a/arch/s390/kernel/nospec-branch.c
+++ b/arch/s390/kernel/nospec-branch.c
@@ -2,6 +2,7 @@
#include <linux/module.h>
#include <linux/device.h>
#include <linux/cpu.h>
+#include <linux/uaccess.h>
#include <asm/nospec-branch.h>
int nobp = IS_ENABLED(CONFIG_KERNEL_NOBP);
diff --git a/drivers/s390/char/uvdevice.c b/drivers/s390/char/uvdevice.c
index e6a264c996ce..4fe3ead4a091 100644
--- a/drivers/s390/char/uvdevice.c
+++ b/drivers/s390/char/uvdevice.c
@@ -28,6 +28,8 @@
#include <linux/vmalloc.h>
#include <linux/slab.h>
#include <linux/cpufeature.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
#include <asm/uvdevice.h>
#include <asm/uv.h>
--
2.54.0
^ permalink raw reply related
* [PATCH 1/4] x86: Avoid indirect includes through linux/module.h -> linux/elf.h
From: Petr Pavlu @ 2026-07-07 15:37 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Peter Huewe,
Jarkko Sakkinen, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen
Cc: Xin Li, H. Peter Anvin, Andy Lutomirski, Christian Borntraeger,
Sven Schnelle, Janosch Frank, Claudio Imbrenda, David Hildenbrand,
Jason Gunthorpe, Aaron Tomlin, linux-s390, kvm, linux-integrity,
linux-modules, linux-kernel
In-Reply-To: <20260707153819.4172541-1-petr.pavlu@suse.com>
Several x86 files use ELF-related declarations and currently rely on them
being provided indirectly through linux/module.h -> linux/elf.h:
* arch/x86/entry/entry_fred.c: ia32_enabled() -> asm/ia32.h,
* arch/x86/entry/syscall_32.c: vdso32_image -> asm/vdso.h,
* arch/x86/kernel/cpu/intel.c: ELF_HWCAP2 -> asm/elf.h,
* arch/x86/kernel/fpu/xstate.c: elf_coredump_extra_notes_write(),
elf_coredump_extra_notes_size(), NT_X86_XSAVE_LAYOUT, elf_note ->
linux/elf.h,
* arch/x86/kernel/process.c: mmap_is_ia32() -> asm/elf.h,
* arch/x86/kernel/signal.c: get_sigframe_size() -> asm/elf.h.
Add the missing includes to these files in preparation for removing the
linux/elf.h include from linux/module.h.
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
arch/x86/entry/entry_fred.c | 1 +
arch/x86/entry/syscall_32.c | 1 +
arch/x86/kernel/cpu/intel.c | 1 +
arch/x86/kernel/fpu/xstate.c | 2 +-
arch/x86/kernel/process.c | 1 +
arch/x86/kernel/signal.c | 1 +
6 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/x86/entry/entry_fred.c b/arch/x86/entry/entry_fred.c
index fb3594ddf731..ea3bb4298065 100644
--- a/arch/x86/entry/entry_fred.c
+++ b/arch/x86/entry/entry_fred.c
@@ -9,6 +9,7 @@
#include <asm/desc.h>
#include <asm/fred.h>
+#include <asm/ia32.h>
#include <asm/idtentry.h>
#include <asm/syscall.h>
#include <asm/trapnr.h>
diff --git a/arch/x86/entry/syscall_32.c b/arch/x86/entry/syscall_32.c
index 31b9492fe851..12fceffc99ef 100644
--- a/arch/x86/entry/syscall_32.c
+++ b/arch/x86/entry/syscall_32.c
@@ -12,6 +12,7 @@
#include <asm/traps.h>
#include <asm/cpufeature.h>
#include <asm/syscall.h>
+#include <asm/vdso.h>
#ifdef CONFIG_IA32_EMULATION
#define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, compat)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index abb3984336eb..18cf40f08557 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -17,6 +17,7 @@
#include <asm/cpufeature.h>
#include <asm/cpu.h>
#include <asm/cpuid/api.h>
+#include <asm/elf.h>
#include <asm/hwcap2.h>
#include <asm/intel-family.h>
#include <asm/microcode.h>
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index a7b6524a9dea..8b90bf86a742 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -7,6 +7,7 @@
#include <linux/bitops.h>
#include <linux/compat.h>
#include <linux/cpu.h>
+#include <linux/elf.h>
#include <linux/mman.h>
#include <linux/kvm_types.h>
#include <linux/nospec.h>
@@ -26,7 +27,6 @@
#include <asm/msr.h>
#include <asm/tlbflush.h>
#include <asm/prctl.h>
-#include <asm/elf.h>
#include <uapi/asm/elf.h>
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 85435044e33c..e40f7f3d3489 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -54,6 +54,7 @@
#include <asm/mmu_context.h>
#include <asm/msr.h>
#include <asm/shstk.h>
+#include <asm/elf.h>
#include "process.h"
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 2404233336ab..0fe2d7a6b1af 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -29,6 +29,7 @@
#include <linux/syscalls.h>
#include <linux/rseq.h>
+#include <asm/elf.h>
#include <asm/processor.h>
#include <asm/ucontext.h>
#include <asm/fpu/signal.h>
--
2.54.0
^ permalink raw reply related
* [PATCH 0/4] module: Limit ELF includes in linux/module.h to uapi/linux/elf.h
From: Petr Pavlu @ 2026-07-07 15:37 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Peter Huewe,
Jarkko Sakkinen, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen
Cc: Xin Li, H. Peter Anvin, Andy Lutomirski, Christian Borntraeger,
Sven Schnelle, Janosch Frank, Claudio Imbrenda, David Hildenbrand,
Jason Gunthorpe, Aaron Tomlin, linux-s390, kvm, linux-integrity,
linux-modules, linux-kernel
The linux/module.h header is included by every *.mod.c file to provide
`struct module` and other related definitions. This makes it a "hot"
header, so it should avoid pulling in unnecessary definitions.
The header currently includes linux/elf.h but this is largely unnecessary
because it requires only the base ELF structures, such as Elf_Ehdr and
Elf_Sym, which are declared in uapi/linux/elf.h.
Replace the linux/elf.h include with uapi/linux/elf.h. On x86_64_defconfig,
including linux/module.h alone results in 2.2 MB of preprocessed data. This
change reduces that to 1.5 MB.
The first three patches are cleanups to avoid relying on declarations being
indirectly provided through linux/module.h -> linux/elf.h. These patches
are all independent and can go through their respective trees. The final
patch depends on these changes.
It is hard to identify all files that indirectly reference something only
through linux/module.h -> linux/elf.h. The changes were build-tested with
allmodconfig on arm, arm64, m68k, mips, powerpc64, riscv, s390, x86_64.
Petr Pavlu (4):
x86: Avoid indirect includes through linux/module.h -> linux/elf.h
s390: Avoid indirect includes through linux/module.h -> linux/elf.h
tpm_crb_ffa: Include linux/slab.h for kzalloc_obj() and kfree()
module: Limit ELF includes in linux/module.h to uapi/linux/elf.h
arch/s390/kernel/nospec-branch.c | 1 +
arch/x86/entry/entry_fred.c | 1 +
arch/x86/entry/syscall_32.c | 1 +
arch/x86/kernel/cpu/intel.c | 1 +
arch/x86/kernel/fpu/xstate.c | 2 +-
arch/x86/kernel/process.c | 1 +
arch/x86/kernel/signal.c | 1 +
drivers/char/tpm/tpm_crb_ffa.c | 1 +
drivers/s390/char/uvdevice.c | 2 ++
include/linux/module.h | 4 ++--
10 files changed, 12 insertions(+), 3 deletions(-)
--
2.54.0
^ permalink raw reply
* Re: [PATCH net v2] ppp: defer channel free to an RCU grace period to fix pppol2tp RX UAF
From: Petr Pavlu @ 2026-07-07 15:32 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Qingfang Deng, Breno Leitao, Norbert Szetei, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Taegu Ha, Kees Cook, linux-ppp, linux-kernel, Guillaume Nault,
netdev, Luis Chamberlain, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, linux-modules, Paul E. McKenney
In-Reply-To: <20260706092926.PIlrhRKz@linutronix.de>
On 7/6/26 11:29 AM, Sebastian Andrzej Siewior wrote:
> + MODULE maintainer
+ Paul E. McKenney
>
> On 2026-07-05 10:57:44 [+0800], Qingfang Deng wrote:
>> On 7/4/2026 at 12:32 AM, Breno Leitao wrote:
>>> On Fri, Jul 03, 2026 at 03:27:00PM +0800, Qingfang Deng wrote:
>>>> AI-review found an issue: https://sashiko.dev/#/patchset/D9C0245B-608B-4884-8A09-F55BA4A9F948%40doyensec.com
>>>>
>>>> An rcu_barrier() call is needed at the end of ppp_cleanup().
>>>
>>> I was initially unclear why rcu_barrier() would be necessary on a kfree path,
>>> but it appears to be required during module unload to ensure that
>>> ppp_release_channel_free() completes before the module's struct rcu_head is
>>> destroyed. Is that the correct understanding?
>>
>> It's required to ensure that all ppp_release_channel_free() callback
>> complete before the text segment of the module is unloaded.
>
> So either a rcu_barrier() in ppp's module_exit() callback or a
> synchronize_rcu() instead of the call_rcu(). And all this because the
> module RCU callbacks pending which can be invoked after the module has
> been removed. There is a synchronize_rcu() during module exit but this
> is after the module code is gone.
>
> I'm curious how many modules have a call_rcu() within their code but
> don't have anything to enforce its completion before module removal is
> complete? Wouldn't something like
>
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 46dd8d25a6058..8eae1ea2d6eb4 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -858,6 +858,9 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> goto out;
>
> mutex_unlock(&module_mutex);
> +
> + /* Ensure all rcu callbacks issued by the module have completed */
> + rcu_barrier();
> /* Final destruction now no one is using it. */
> if (mod->exit != NULL)
> mod->exit();
>
> make sense?
This is discussed in Documentation/RCU/rcubarrier.rst and
Documentation/RCU/Design/Requirements/Requirements.rst. The latter
contains:
| Loadable Modules
| ~~~~~~~~~~~~~~~~
|
| The Linux kernel has loadable modules, and these modules can also be
| unloaded. After a given module has been unloaded, any attempt to call
| one of its functions results in a segmentation fault. The module-unload
| functions must therefore cancel any delayed calls to loadable-module
| functions, for example, any outstanding mod_timer() must be dealt
| with via timer_shutdown_sync() or similar.
|
| Unfortunately, there is no way to cancel an RCU callback; once you
| invoke call_rcu(), the callback function is eventually going to be
| invoked, unless the system goes down first. Because it is normally
| considered socially irresponsible to crash the system in response to a
| module unload request, we need some other way to deal with in-flight RCU
| callbacks.
|
| RCU therefore provides rcu_barrier(), which waits until all
| in-flight RCU callbacks have been invoked. If a module uses
| call_rcu(), its exit function should therefore prevent any future
| invocation of call_rcu(), then invoke rcu_barrier(). In theory,
| the underlying module-unload code could invoke rcu_barrier()
| unconditionally, but in practice this would incur unacceptable
| latencies.
I don't know if the last part about unacceptable latencies is still
relevant. I haven't done any measurements myself.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH v19 31/40] dept: assign unique dept_key to each distinct wait_for_completion() caller
From: Gary Guo @ 2026-07-07 14:18 UTC (permalink / raw)
To: Byungchul Park, linux-kernel
Cc: 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, willy, 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-32-byungchul@sk.com>
On Mon Jul 6, 2026 at 7:19 AM BST, Byungchul Park wrote:
> wait_for_completion() can be used at various points in the code and it's
> very hard to distinguish wait_for_completion()s between different usages.
> Using a single dept_key for all the wait_for_completion()s could trigger
> false positive reports.
>
> Assign unique dept_key to each distinct wait_for_completion() caller to
> avoid false positive reports.
>
> While at it, add a rust helper for wait_for_completion() to avoid build
> errors.
This will cause Rust code to share the same dept_key, so it will have all the
false positives that the change is trying to avoid.
In general it is easy to create Rust bindings for static inline C functions
because it'll be just some computation, while creating bindings for C
function-like macros that define additional statics can be challenging.
Is dept_key similar to lock_class_key, where only the address matters? If so,
the approach that I use in
https://lore.kernel.org/rust-for-linux/DJP0CDOR98N5.29BK8PUFRWRUK@garyguo.net
could be used for dept_key as well, then we can keep Rust `wait_for_completion`
still a function; otherwise we have to turn it into a macro too on the Rust side
to create such statics, which isn't ideal.
Best,
Gary
>
> Signed-off-by: Byungchul Park <byungchul@sk.com>
> ---
> include/linux/completion.h | 100 +++++++++++++++++++++++++++++++------
> kernel/sched/completion.c | 60 +++++++++++-----------
> rust/helpers/completion.c | 5 ++
> 3 files changed, 120 insertions(+), 45 deletions(-)
^ permalink raw reply
* Re: [PATCH v19 40/40] dept: implement a basic unit test for dept
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: cassel, dri-devel, Jeff Layton, dlemoal, tytso, ojeda,
linux-kernel, linux-modules, Anna Schumaker, linux-ide,
Chuck Lever, linux-rt-devel
In-Reply-To: <20260706061928.66713-41-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Data race on lockless global `dept_ut_results` test counters.
--
commit ed16879949bb8cfa840ac89b2502567868a42cfe
Author: Byungchul Park <byungchul@sk.com>
dept: implement a basic unit test for dept
This commit introduces CONFIG_DEPT_UNIT_TEST and a corresponding kernel
module that runs basic unit tests for the Dependency Tracker (DEPT).
> diff --git a/include/linux/dept_unit_test.h b/include/linux/dept_unit_test.h
> new file mode 100644
> index 0000000000000..753ac9ac727c6
> --- /dev/null
> +++ b/include/linux/dept_unit_test.h
[ ... ]
> +static inline void dept_ut_ecxt_stack_account(bool valid)
> +{
> + dept_ut_results.ecxt_stack_total_cnt++;
[Severity: Medium]
Since this global counter is incremented non-atomically, can it race with
concurrent DEPT evaluations (e.g., from print_dep()) on other CPUs? A standard
data race here might lead to corrupted test counters or trigger KCSAN warnings.
[ ... ]
> diff --git a/kernel/dependency/dept_unit_test.c b/kernel/dependency/dept_unit_test.c
> new file mode 100644
> index 0000000000000..e8dada2e3dfba
> --- /dev/null
> +++ b/kernel/dependency/dept_unit_test.c
[ ... ]
> +static DEFINE_SPINLOCK(s1);
> +static DEFINE_SPINLOCK(s2);
> +static bool test_spin_lock_deadlock(void)
> +{
> + dept_ut_results.circle_detected = false;
[Severity: Medium]
Is it safe to reset this flag without protection? If a genuine dependency
issue triggers DEPT on another CPU and calls print_circle() concurrently,
could it set circle_detected to true and interfere with this test's result?
[ ... ]
> +static int __init dept_ut_init(void)
> +{
> + int i;
> +
> + lockdep_off();
> +
> + dept_ut_results.ecxt_stack_valid_cnt = 0;
> + dept_ut_results.ecxt_stack_total_cnt = 0;
> + dept_ut_results.wait_stack_valid_cnt = 0;
> + dept_ut_results.wait_stack_total_cnt = 0;
> + dept_ut_results.evnt_stack_valid_cnt = 0;
> + dept_ut_results.evnt_stack_total_cnt = 0;
[Severity: Medium]
Could resetting these global counters locklessly and without accessors like
WRITE_ONCE cause issues? It appears this setup could race with concurrent
DEPT reports modifying them simultaneously in print_dep().
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=40
^ permalink raw reply
* Re: [PATCH v19 38/40] mm: percpu: increase PERCPU_DYNAMIC_SIZE_SHIFT on DEPT and large PAGE_SIZE
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: Jeff Layton, ojeda, tytso, Chuck Lever, Anna Schumaker,
linux-kernel, cassel, linux-rt-devel, dri-devel, dlemoal,
linux-ide, linux-modules
In-Reply-To: <20260706061928.66713-39-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch addresses a compile-time BUILD_BUG_ON failure that no longer exists, referencing a deleted structure (`struct kmem_cache_cpu`) and function (`alloc_kmem_cache_cpus()`). Both have been removed from the upstream kernel. The only remaining BUILD_BUG_ON checking `PERCPU_DYNAMIC_EARLY_SIZE` in SLUB is in `alloc_kmem_cache_stats()`, which evaluates `sizeof(struct kmem_cache_stats)`. Because `struct kmem_cache_stats` consists solely of an integer array, its size is entirely unaffected by `CONFIG_DEPT` or `local_lock_t`. Thus, this patch needlessly increases the dynamically reserved per-CPU memory based on an obsolete and misleading justification.
--
commit 279fb8fb2919a407c18ab625ec67d2a3430b7b4d
Author: Byungchul Park <byungchul@sk.com>
mm: percpu: increase PERCPU_DYNAMIC_SIZE_SHIFT on DEPT and large PAGE_SIZE
This commit increases PERCPU_DYNAMIC_SIZE_SHIFT to 11 when both CONFIG_DEPT
and a PAGE_SIZE larger than 4KiB are enabled. It aims to prevent a build
failure related to the size of struct kmem_cache_cpu.
> Yunseong reported a build failure due to the BUILD_BUG_ON() statement in
> alloc_kmem_cache_cpus(). In the following test:
>
> PERCPU_DYNAMIC_EARLY_SIZE < NR_KMALLOC_TYPES * KMALLOC_SHIFT_HIGH * sizeof(struct kmem_cache_cpu)
[Severity: Medium]
Are we referencing structures and functions that have been removed from the
upstream kernel?
Both alloc_kmem_cache_cpus() and struct kmem_cache_cpu no longer exist
upstream. The only remaining BUILD_BUG_ON() checking PERCPU_DYNAMIC_EARLY_SIZE
in SLUB is in alloc_kmem_cache_stats():
mm/slub.c:alloc_kmem_cache_stats() {
BUILD_BUG_ON(PERCPU_DYNAMIC_EARLY_SIZE <
NR_KMALLOC_TYPES * KMALLOC_SHIFT_HIGH *
sizeof(struct kmem_cache_stats));
}
Since struct kmem_cache_stats consists solely of an integer array, its size
is entirely unaffected by CONFIG_DEPT or local_lock_t. Does this mean the
increase in dynamically reserved per-CPU memory is no longer needed?
> diff --git a/include/linux/percpu.h b/include/linux/percpu.h
> index 85bf8dd9f0874..dd74321d4bbd0 100644
> --- a/include/linux/percpu.h
> +++ b/include/linux/percpu.h
> @@ -43,7 +43,11 @@
> # define PERCPU_DYNAMIC_SIZE_SHIFT 12
> #endif /* LOCKDEP and PAGE_SIZE > 4KiB */
> #else
> +#if defined(CONFIG_DEPT) && !defined(CONFIG_PAGE_SIZE_4KB)
> +#define PERCPU_DYNAMIC_SIZE_SHIFT 11
> +#else
> #define PERCPU_DYNAMIC_SIZE_SHIFT 10
> +#endif /* DEPT and PAGE_SIZE > 4KiB */
> #endif
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=38
^ permalink raw reply
* Re: [PATCH v19 36/40] dept: track PG_writeback with dept
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: Jeff Layton, Anna Schumaker, linux-rt-devel, cassel, Chuck Lever,
ojeda, dlemoal, tytso, dri-devel, linux-modules, linux-ide,
linux-kernel
In-Reply-To: <20260706061928.66713-37-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] False lock acquisition registered in DEPT for SHARED waits on PG_writeback, permanently corrupting the task's dependency graph.
--
commit 9dc48dc95eded487307805ef6bce49ac459667ad
Author: Byungchul Park <byungchul@sk.com>
dept: track PG_writeback with dept
This commit adds DEPT tracking for PG_writeback to track writeback waits
and events. It extends the existing page bit tracking machinery to cover
PG_writeback in addition to PG_locked.
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 0b0655354b084..ec736811a2c66 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
[ ... ]
> @@ -292,6 +293,8 @@ static inline void dept_page_set_bit(struct page *p, int bit_nr)
>
> if (bit_nr == PG_locked)
> dept_request_event(&pg_locked_map, &p->pg_locked_wgen);
> + else if (bit_nr == PG_writeback)
> + dept_request_event(&pg_writeback_map, &p->pg_writeback_wgen);
[Severity: High]
Does wiring PG_writeback into this tracking logic cause false lock
acquisitions for shared waits?
In the wait queue logic, tasks can wait for writeback to complete
without actually acquiring the bit (a SHARED wait). However, when a task
waits on PG_writeback and the bit is already clear on the fast path,
folio_trylock_flag() sets WQ_FLAG_DONE:
mm/filemap.c:folio_trylock_flag() {
...
} else if (test_bit(bit_nr, &folio->flags.f))
return false;
wait->flags |= WQ_FLAG_WOKEN | WQ_FLAG_DONE;
return true;
}
Then in folio_wait_bit_common(), it unconditionally registers an acquisition
if WQ_FLAG_DONE is set:
mm/filemap.c:folio_wait_bit_common() {
...
if (wait->flags & WQ_FLAG_DONE)
dept_page_set_bit(&folio->page, bit_nr);
...
}
Because the task never actually acquired the bit, it will never clear it,
leaving DEPT recording the lock as permanently held. Could this cause
DEPT resource leaks and false positive deadlock reports?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=36
^ permalink raw reply
* Re: [PATCH v19 35/40] dept: introduce APIs to set page usage and use subclasses_evt for the usage
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: linux-kernel, Chuck Lever, ojeda, dri-devel, linux-modules,
cassel, tytso, linux-rt-devel, linux-ide, dlemoal, Jeff Layton,
Anna Schumaker
In-Reply-To: <20260706061928.66713-36-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Page usage state (`usage.type`) is not cleared upon page free, leaking stale DEPT classes.
- [High] Premature usage type update in waiters breaks the DEPT_PAGE_DEFAULT fallback mechanism.
- [Medium] The newly introduced APIs for setting page usage are never called.
- [Low] Passing unsigned int* to atomic_try_cmpxchg violates pointer compatibility.
- [Low] Redundant bitwise operations in dept_set_page_usage.
- [Low] Missing dummy implementations for dept_set_folio_usage and dept_reset_folio_usage.
--
commit ded2d968cb8933d3c60792b69d4f3c8618c64ffa
Author: Byungchul Park <byungchul@sk.com>
dept: introduce APIs to set page usage and use subclasses_evt for the usage
False positive reports have been observed since dept assumes that all
the pages have the same dept class, but the class should be split since
the call paths are different depending on what the page is used for.
This commit introduces APIs to track these usages via struct page to
avoid false positive DEPT reports.
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 5b3f54ee0d381..e25d09f3dfa9b 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -220,6 +220,7 @@ struct page {
> struct page *kmsan_shadow;
> struct page *kmsan_origin;
> #endif
> + struct dept_page_usage usage;
[Severity: High]
Does this leak stale DEPT classes when pages are freed and reallocated?
The new usage field is added to struct page, but it doesn't appear to be
cleared during page free. If a page with a specific usage type is freed and
later reallocated for a different purpose, it might retain the old usage
type, leading to graph pollution and false positives.
Should this state be scrubbed in mm/page_alloc.c:__free_pages_prepare()?
> struct dept_ext_wgen pg_locked_wgen;
> } _struct_page_alignment;
>
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 8ab39823ea313..0b0655354b084 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
[ ... ]
> @@ -204,6 +204,80 @@ enum pageflags {
>
> extern struct dept_map pg_locked_map;
>
> +static inline void dept_set_page_usage(struct page *p,
> + unsigned int new_type)
> +{
[Severity: Medium]
This isn't a bug, but are there missing callers for these newly introduced
APIs?
The commit message mentions splitting usages for block and file caches,
but dept_set_page_usage() and dept_set_folio_usage() do not seem to be
invoked anywhere in this patch.
> + /*
> + * Consider the page as DEPT_PAGE_DEFAULT until the next use of
> + * PG flags e.g. folio_lock().
> + */
> + unsigned int type = DEPT_PAGE_DEFAULT;
> +
> + if (WARN_ON_ONCE(new_type >= DEPT_PAGE_USAGE_NR))
> + return;
> +
> + new_type <<= DEPT_PAGE_USAGE_SHIFT;
> + new_type |= type & DEPT_PAGE_USAGE_MASK;
[Severity: Low]
Does this bitwise OR operation have any effect?
Since type is unconditionally initialized to DEPT_PAGE_DEFAULT (which is 0)
just above, masking and ORing it with new_type seems redundant.
> + atomic_set(&p->usage.type, new_type);
> +}
[ ... ]
> +static inline void dept_update_page_usage(struct page *p)
> +{
> + unsigned int type = atomic_read(&p->usage.type);
> + unsigned int new_type;
> +
> +retry:
> + new_type = type & DEPT_PAGE_USAGE_PENDING_MASK;
> + new_type >>= DEPT_PAGE_USAGE_SHIFT;
> + new_type |= type & DEPT_PAGE_USAGE_PENDING_MASK;
> +
> + /*
> + * Already updated by others.
> + */
> + if (type == new_type)
> + return;
> +
> + if (!atomic_try_cmpxchg(&p->usage.type, &type, new_type))
[Severity: Low]
Can this trigger a -Wpointer-sign build warning?
The type variable is declared as unsigned int, but atomic_try_cmpxchg()
expects an int * for its second argument.
> + goto retry;
> +}
[ ... ]
> static inline void dept_page_set_bit(struct page *p, int bit_nr)
> {
> + dept_update_page_usage(p);
> +
> if (bit_nr == PG_locked)
> dept_request_event(&pg_locked_map, &p->pg_locked_wgen);
> }
[ ... ]
> static inline void dept_page_wait_on_bit(struct page *p, int bit_nr)
> {
> + unsigned long evt_f;
> +
> + dept_update_page_usage(p);
[Severity: High]
Does calling dept_update_page_usage() in these flag update/wait functions
prematurely overwrite the page usage, breaking the DEPT_PAGE_DEFAULT fallback
mechanism?
If waiters or setters trigger dept_update_page_usage() unconditionally, they
could clobber the current state with the pending state before the lock is
released. When the lock is subsequently released, it might emit the wrong
event type, which could cause early DEFAULT waiters to hang indefinitely
and trigger false positive reports.
> + evt_f = dept_event_flags(p, true);
> +
> if (bit_nr == PG_locked)
> - dept_wait(&pg_locked_map, 1UL, _RET_IP_, __func__, 0, -1L);
> + dept_wait(&pg_locked_map, evt_f, _RET_IP_, __func__, 0, -1L);
> }
[ ... ]
> @@ -245,6 +328,8 @@ static inline void dept_folio_wait_on_bit(struct folio *f, int bit_nr)
> dept_page_wait_on_bit(&f->page, bit_nr);
> }
> #else
> +#define dept_set_page_usage(p, t) do { } while (0)
> +#define dept_reset_page_usage(p) do { } while (0)
[Severity: Low]
Will this break the build for users of dept_set_folio_usage() and
dept_reset_folio_usage() when CONFIG_DEPT is disabled?
The #else block provides dummy macros for page variants, but seems to be
missing the folio equivalents.
> #define dept_page_set_bit(p, bit_nr) do { } while (0)
> #define dept_page_clear_bit(p, bit_nr) do { } while (0)
> #define dept_page_wait_on_bit(p, bit_nr) do { } while (0)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=35
^ permalink raw reply
* Re: [PATCH v19 34/40] rcu/update: fix same dept key collision between various types of RCU
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: dri-devel, linux-ide, linux-kernel, dlemoal, cassel,
Anna Schumaker, ojeda, tytso, Chuck Lever, Jeff Layton,
linux-rt-devel, linux-modules
In-Reply-To: <20260706061928.66713-35-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Unused struct member `dkey` inflates stack footprint
--
commit de6aa9797881496b4a8d7d66626b1bfd19cfdd54
Author: Yunseong Kim <ysk@kzalloc.com>
rcu/update: fix same dept key collision between various types of RCU
This commit addresses a false positive circular dependency report in dept
by assigning separate dept keys and maps to each struct rcu_synchronize
execution context.
> diff --git a/include/linux/rcupdate_wait.h b/include/linux/rcupdate_wait.h
> --- a/include/linux/rcupdate_wait.h
> +++ b/include/linux/rcupdate_wait.h
> @@ -19,17 +19,20 @@ struct rcu_synchronize {
>
> /* This is for debugging. */
> struct rcu_gp_oldstate oldstate;
> + struct dept_map dmap;
> + struct dept_key dkey;
[Severity: Low]
Does this code ever use the dkey member of struct rcu_synchronize?
The _wait_rcu_gp() macro defines its own static struct dept_key __key,
which it passes as an argument to __wait_rcu_gp(). This leaves the dkey
member of struct rcu_synchronize completely unused and uninitialized, while
needlessly increasing the stack footprint of the rs_array allocated in the
macro.
> };
> void wakeme_after_rcu(struct rcu_head *head);
>
> void __wait_rcu_gp(bool checktiny, unsigned int state, int n, call_rcu_func_t *crcu_array,
> - struct rcu_synchronize *rs_array);
> + struct rcu_synchronize *rs_array, struct dept_key *dkey);
>
> #define _wait_rcu_gp(checktiny, state, ...) \
> -do { \
> - call_rcu_func_t __crcu_array[] = { __VA_ARGS__ }; \
> - struct rcu_synchronize __rs_array[ARRAY_SIZE(__crcu_array)]; \
> - __wait_rcu_gp(checktiny, state, ARRAY_SIZE(__crcu_array), __crcu_array, __rs_array); \
> +do { \
> + call_rcu_func_t __crcu_array[] = { __VA_ARGS__ }; \
> + static struct dept_key __key; \
> + struct rcu_synchronize __rs_array[ARRAY_SIZE(__crcu_array)]; \
> + __wait_rcu_gp(checktiny, state, ARRAY_SIZE(__crcu_array), __crcu_array, __rs_array, &__key); \
> } while (0)
[ ... ]
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
[ ... ]
> @@ -426,7 +426,8 @@ void __wait_rcu_gp(bool checktiny, unsigned int state, int n, call_rcu_func_t *c
> break;
> if (j == i) {
> init_rcu_head_on_stack(&rs_array[i].head);
> - init_completion(&rs_array[i].completion);
> + sdt_map_init_key(&rs_array[i].dmap, dkey);
> + init_completion_dmap(&rs_array[i].completion, &rs_array[i].dmap);
> (crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
> }
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=34
^ permalink raw reply
* Re: [PATCH v19 33/40] dept: call dept_hardirqs_off() in local_irq_*() regardless of irq state
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: linux-modules, linux-kernel, Jeff Layton, linux-ide, cassel,
Chuck Lever, Anna Schumaker, dri-devel, tytso, linux-rt-devel,
dlemoal, ojeda
In-Reply-To: <20260706061928.66713-34-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hides missing assembly trace annotations, causing DEPT and Lockdep states to diverge and allowing NMIs to silently revert the workaround.
- [Medium] Exporting `dept_hardirqs_off` as GPL-only breaks the build/loading of non-GPL modules that use `local_irq_save()` or `local_irq_disable()`.
--
commit 3f12c8d53e9de3037ef421690b95c2ca6667e3a1
Author: Byungchul Park <byungchul@sk.com>
dept: call dept_hardirqs_off() in local_irq_*() regardless of irq state
For dept to function properly, dept_task()->hardirqs_enabled must be set
correctly. If it fails to set this value to false, for example, dept
may mistakenly think irq is still enabled even when it's not.
> diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
> index d8b9cf093f83d..586f5bad4da78 100644
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> @@ -214,6 +214,13 @@ extern void warn_bogus_irq_restore(void);
> raw_local_irq_disable(); \
> if (!was_disabled) \
> trace_hardirqs_off(); \
> + /* \
> + * Just in case that C code has missed \
> + * trace_hardirqs_off() at the first \
> + * place e.g. disabling irq at asm code.\
> + */ \
> + else \
> + dept_hardirqs_off(); \
> } while (0)
[Severity: High]
Can NMIs silently revert this workaround?
If assembly code disables IRQs but misses the trace_hardirqs_off() annotation,
and then C code calls local_irq_disable(), this skips trace_hardirqs_off()
but executes the new dept_hardirqs_off() branch. Lockdep still thinks IRQs
are enabled, while DEPT thinks they are disabled.
If an NMI occurs while in this state, irqentry_nmi_enter() saves the Lockdep
state (which is incorrectly 'enabled'). Upon NMI exit, irqentry_nmi_exit()
restores the saved Lockdep state by calling lockdep_hardirqs_on(), which
unconditionally calls dept_hardirqs_on().
Will this force DEPT back into an incorrect 'enabled' state while hardware
IRQs are actually disabled, potentially leading to false-positive circular
dependency reports?
[ ... ]
> diff --git a/kernel/dependency/dept.c b/kernel/dependency/dept.c
> index 007e1bc7d201e..35a3667ac8b3c 100644
> --- a/kernel/dependency/dept.c
> +++ b/kernel/dependency/dept.c
> @@ -2095,6 +2095,7 @@ void noinstr dept_hardirqs_off(void)
> */
> dept_task()->hardirqs_enabled = false;
> }
> +EXPORT_SYMBOL_GPL(dept_hardirqs_off);
[Severity: Medium]
Will this break the build or loading of non-GPL modules?
Because dept_hardirqs_off() is now called directly from universal inline
macros like local_irq_disable() and local_irq_save(), any out-of-tree
module marked with a non-GPL license that invokes these macros will fail
to link or load when CONFIG_DEPT is enabled.
Standard trace_hardirqs_off() avoids this issue by being exported via
EXPORT_SYMBOL.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=33
^ permalink raw reply
* Re: [PATCH v19 32/40] completion, dept: introduce init_completion_dmap() API
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: dlemoal, Jeff Layton, linux-modules, dri-devel, linux-rt-devel,
cassel, linux-kernel, tytso, Chuck Lever, linux-ide,
Anna Schumaker, ojeda
In-Reply-To: <20260706061928.66713-33-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Double evaluation of arguments in wait_for_completion macros leads to skipped array indices and memory corruption.
- [Medium] Unconditional struct growth of a widely used primitive for a debug feature.
- [Medium] Wait event macros skip calling sdt_might_sleep_end() on signal interruptions, leaking DEPT tracking state and stacks.
--
commit 5b626a37ee940081b6d37187cde51c2f272e370c
Author: Byungchul Park <byungchul@sk.com>
completion, dept: introduce init_completion_dmap() API
Currently, dept uses dept's map embedded in task_struct to track
dependencies related to wait_for_completion() and its family. So it
doesn't need an explicit map basically.
However, for those who want to set the maps with customized class or
key, introduce a new API to use external maps.
> diff --git a/include/linux/completion.h b/include/linux/completion.h
> index 4d8fb1d95c0a6..e50f7d9b4b974 100644
> --- a/include/linux/completion.h
> +++ b/include/linux/completion.h
> @@ -27,17 +27,15 @@
> struct completion {
> unsigned int done;
> struct swait_queue_head wait;
> + struct dept_map *dmap;
> };
[Severity: Medium]
Since struct completion is heavily embedded in many kernel data structures,
does unconditionally adding the dmap pointer increase the global memory
footprint even on production non-debug kernels? Should this new field be
guarded by an #ifdef?
[ ... ]
> @@ -136,13 +140,13 @@ extern void complete_all(struct completion *);
>
> #define wait_for_completion(x) \
> ({ \
> - sdt_might_sleep_start_timeout(NULL, -1L); \
> + sdt_might_sleep_start_timeout((x)->dmap, -1L); \
> __wait_for_completion(x); \
> sdt_might_sleep_end(); \
> })
[Severity: High]
Does evaluating the argument x multiple times in this macro lead to issues
if callers pass an expression with side effects?
For example, if a caller like mlx5_vdpa_exec_async_cmds() does:
while (completed < issued)
wait_for_completion(&cmds[completed++].cmd_done);
Would completed++ be executed twice per iteration, skipping array elements
and potentially causing out-of-bounds memory accesses?
[ ... ]
[Severity: Medium]
Do wait event macros such as ___wait_event() in include/linux/wait.h skip
calling sdt_might_sleep_end() when interrupted by a signal?
In include/linux/wait.h:___wait_event():
if (___wait_is_interruptible(state) && __int) { \
__ret = __int; \
goto __out; \
}
...
sdt_might_sleep_end(); \
__out: __ret; \
})
If the wait is interrupted by a signal, it escapes via goto __out. Because
__out is placed after sdt_might_sleep_end(), it seems the cleanup is
skipped.
Does this permanently leak the tracking state and its underlying stack
objects?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=32
^ permalink raw reply
* Re: [PATCH v19 31/40] dept: assign unique dept_key to each distinct wait_for_completion() caller
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: Anna Schumaker, linux-rt-devel, dri-devel, linux-modules, dlemoal,
Jeff Layton, ojeda, Chuck Lever, cassel, linux-kernel, linux-ide,
tytso
In-Reply-To: <20260706061928.66713-32-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `wait_for_completion_*_timeout` macros evaluate their timeout argument `t` multiple times, creating a side-effect vulnerability.
- [High] The `wait_for_completion_*` wrapper macros declare local variables (`__ret` and `__m`) that shadow variables in the caller's scope, leading to uninitialized memory usage.
- [High] The `wait_for_completion` macros now implicitly depend on GPL-only exported symbols, breaking non-GPL module compilation when CONFIG_DEPT is enabled.
- [Medium] The C helper function `rust_helper_wait_for_completion` defeats the primary goal of the patch for the Rust subsystem by mapping all Rust callers to a single `dept_key`.
--
commit 18fd6796eb79fb0d0201e71c6791aa80955604b2
Author: Byungchul Park <byungchul@sk.com>
dept: assign unique dept_key to each distinct wait_for_completion() caller
This patch assigns a unique dept_key to each distinct wait_for_completion()
caller to avoid false positive reports from DEPT. It also adds a C helper
for the Rust subsystem to avoid build errors.
> diff --git a/include/linux/completion.h b/include/linux/completion.h
> --- a/include/linux/completion.h
> +++ b/include/linux/completion.h
[ ... ]
> +#define wait_for_completion(x) \
> +({ \
> + sdt_might_sleep_start_timeout(NULL, -1L); \
> + __wait_for_completion(x); \
> + sdt_might_sleep_end(); \
> +})
[Severity: High]
Will these macros implicitly depend on GPL-only exported symbols, breaking
non-GPL module compilation when CONFIG_DEPT is enabled?
The original wait_for_completion() functions are exported via EXPORT_SYMBOL,
making them available to non-GPL modules.
The new macros wrap these primitives with calls to dept_stage_wait() and
dept_clean_stage() via sdt_might_sleep_start_timeout().
Since those tracker functions are exported via EXPORT_SYMBOL_GPL, could this
cause linkage failures for non-GPL modules using the macro?
[ ... ]
> +#define wait_for_completion_timeout(x, t) \
> +({ \
> + unsigned long __ret; \
> + \
> + sdt_might_sleep_start_timeout(NULL, t); \
> + __ret = __wait_for_completion_timeout(x, t); \
> + sdt_might_sleep_end(); \
> + __ret; \
> +})
[Severity: High]
Does this macro evaluate the timeout argument t twice?
If a caller passes an expression with side effects (like a stateful
function call or a pre/post-increment) as the timeout argument, the
expression will be executed multiple times.
Would this lead to incorrect timeouts or potential kernel crashes if the
expression modifies sensitive state?
[Severity: High]
Could the internal __ret variable shadow a variable in the caller's scope?
If a caller passes an expression for the timeout t or the completion x
that relies on a variable named __ret in their own scope, the macro's
inner uninitialized __ret might shadow the caller's variable.
Would this result in the underlying wait functions receiving uninitialized
stack memory instead of the intended parameter?
[ ... ]
> diff --git a/rust/helpers/completion.c b/rust/helpers/completion.c
> --- a/rust/helpers/completion.c
> +++ b/rust/helpers/completion.c
[ ... ]
> +void rust_helper_wait_for_completion(struct completion *x)
> +{
> + wait_for_completion(x);
> +}
[Severity: Medium]
Does this C helper function defeat the primary goal of the patch for the
Rust subsystem?
The patch explicitly changes wait_for_completion() to a macro so that a
unique static dept_key is instantiated at each distinct call site, avoiding
false positive dependency reports.
However, by wrapping the macro into a single out-of-line C function,
wouldn't all Rust callers of completions share the exact same dept_key,
reintroducing the false positive reports?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=31
^ permalink raw reply
* Re: [PATCH v19 30/40] dept: make dept stop from working on debug_locks_off()
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: tytso, linux-kernel, Jeff Layton, linux-rt-devel, linux-ide,
cassel, ojeda, Anna Schumaker, dlemoal, dri-devel, linux-modules,
Chuck Lever
In-Reply-To: <20260706061928.66713-31-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] DEPT fails to stop when lock debugging is globally disabled via kernel tainting.
- [Low] dept_stop_emerg() is unnecessarily exported via EXPORT_SYMBOL_GPL despite having no module callers.
- [Low] Implicit reliance on transitive header inclusion for dept_stop_emerg().
--
commit 3772379470ad3a639343d641bfb17cbd105fc9a5
Author: Byungchul Park <byungchul@sk.com>
dept: make dept stop from working on debug_locks_off()
The commit adds a dept_stop_emerg() call inside debug_locks_off() to
stop the DEPT feature when panic or other lock debugging suspensions occur.
> diff --git a/kernel/dependency/dept.c b/kernel/dependency/dept.c
> index 048348ea64d29..007e1bc7d201e 100644
> --- a/kernel/dependency/dept.c
> +++ b/kernel/dependency/dept.c
[ ... ]
> @@ -187,6 +187,12 @@ static void dept_unlock(void)
> arch_spin_unlock(&dept_spin);
> }
>
> +void dept_stop_emerg(void)
> +{
> + WRITE_ONCE(dept_stop, 1);
> +}
> +EXPORT_SYMBOL_GPL(dept_stop_emerg);
[Severity: Low]
Does this function need to be exported to loadable modules?
CONFIG_DEPT is a boolean configuration, so kernel/dependency/dept.c is
always built-in. Its only caller in lib/debug_locks.c is also always
built-in, making this export unnecessary.
> +
> enum bfs_ret {
> BFS_CONTINUE,
> BFS_DONE,
> diff --git a/lib/debug_locks.c b/lib/debug_locks.c
> index a75ee30b77cb8..14a965914a8fb 100644
> --- a/lib/debug_locks.c
> +++ b/lib/debug_locks.c
[ ... ]
> @@ -38,6 +38,8 @@ EXPORT_SYMBOL_GPL(debug_locks_silent);
> */
> int debug_locks_off(void)
> {
> + dept_stop_emerg();
[Severity: Low]
Is it possible to explicitly include <linux/dept.h> in lib/debug_locks.c?
Relying on implicit transitive header inclusion for dept_stop_emerg()
could break the build if inner header structures like spinlock.h or
lockdep.h change in the future.
> +
> if (debug_locks && __debug_locks_off()) {
[Severity: Medium]
Will this miss DEPT shutdown when the system is tainted?
For example, when a system fault occurs, add_taint() disables lockdep via
__debug_locks_off():
kernel/panic.c:add_taint() {
...
if (lockdep_ok == LOCKDEP_NOW_UNRELIABLE && __debug_locks_off())
pr_warn("Disabling lock debugging due to kernel taint\n");
...
}
Because this bypasses debug_locks_off(), DEPT remains enabled when the
system lock state is already unreliable, potentially leading to false
positive dependency splats or further crashes.
> if (!debug_locks_silent) {
> console_verbose();
> return 1;
> }
> }
> return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=30
^ permalink raw reply
* Re: [PATCH v19 29/40] dept: make dept aware of lockdep_set_lock_cmp_fn() annotation
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: linux-modules, Chuck Lever, Anna Schumaker, dri-devel,
linux-kernel, linux-rt-devel, dlemoal, cassel, tytso, Jeff Layton,
ojeda, linux-ide
In-Reply-To: <20260706061928.66713-30-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] DEPT falsely reports a deadlock when a task sequentially acquires three or more locks of the same class (or interleaves them with other locks), because the `lockdep_cmp_fn` bypass is improperly restricted to only the immediately preceding lock.
--
commit d03c5b926a945b89f5a90fd98986386e113df5af
Author: Byungchul Park <byungchul@sk.com>
dept: make dept aware of lockdep_set_lock_cmp_fn() annotation
This commit updates the DEPT (Dependency Tracker) subsystem to respect the
lockdep_set_lock_cmp_fn() annotation. It allows DEPT to avoid false positive
deadlock reports by using the custom comparison function when evaluating
dependencies between locks of the same class.
> diff --git a/kernel/dependency/dept.c b/kernel/dependency/dept.c
> --- a/kernel/dependency/dept.c
> +++ b/kernel/dependency/dept.c
[ ... ]
> @@ -1658,8 +1688,13 @@ static void add_wait(struct dept_class *c, unsigned long ip,
> if (!eh->ecxt)
> continue;
>
> - if (eh->ecxt->class != c || eh->sub_l == sub_l)
> - add_dep(eh->ecxt, w);
> + if (eh->ecxt->class == c && eh->sub_l != sub_l)
> + continue;
> +
> + if (i == dt->ecxt_held_pos - 1 && lockdep_cmp_fn(eh->map, m))
> + continue;
[Severity: Medium]
Will this condition incorrectly bypass the comparison for earlier locks in
the chain?
By restricting the lockdep_cmp_fn() check to only the most recently
acquired lock (i == dt->ecxt_held_pos - 1), it seems we might miss valid
comparisons for older contexts.
For example, if a task acquires lock A1, then lock B, and then lock A2
(where A1 and A2 are the same class), the check against A1 will be skipped
because A1 is no longer at dt->ecxt_held_pos - 1.
This would result in a call to add_dep(A1, A2), creating a
self-referential class dependency that triggers a false positive deadlock
report.
Should lockdep_cmp_fn() be allowed to evaluate against any previously held
lock in the chain, rather than just the immediate predecessor?
> +
> + add_dep(eh->ecxt, w);
> }
>
> wg = next_wgen();
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=29
^ permalink raw reply
* Re: [PATCH v19 28/40] dept: assign unique dept_key to each distinct dma fence caller
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: Chuck Lever, dlemoal, Jeff Layton, linux-rt-devel, linux-modules,
dri-devel, linux-ide, tytso, cassel, ojeda, Anna Schumaker,
linux-kernel
In-Reply-To: <20260706061928.66713-29-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The newly introduced statement expression macros for DMA fence waiting evaluate their timeout argument (`t`) multiple times.
- [Medium] Local variable shadowing in the statement expression macros due to the unhygienic `__ret` variable declaration.
--
commit 4d4caad8d173eededcba1493c060f5ddecbd6b70
Author: Byungchul Park <byungchul@sk.com>
dept: assign unique dept_key to each distinct dma fence caller
dma fence can be used at various points in the code and it's very hard
to distinguish dma fences between different usages. Using a single
dept_key for all the dma fences could trigger false positive reports.
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index d4c92fd350924..3732849a30b7e 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
[ ... ]
> @@ -370,8 +370,22 @@ bool dma_fence_check_and_signal_locked(struct dma_fence *fence);
> void dma_fence_signal_locked(struct dma_fence *fence);
> void dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp);
> void dma_fence_signal_timestamp_locked(struct dma_fence *fence, ktime_t timestamp);
> -signed long dma_fence_default_wait(struct dma_fence *fence,
> +signed long __dma_fence_default_wait(struct dma_fence *fence,
> bool intr, signed long timeout);
> +
> +/*
> + * Associate every caller with its own dept map.
> + */
> +#define dma_fence_default_wait(f, intr, t) \
> +({ \
> + signed long __ret; \
[Severity: Medium]
Could this local variable declaration lead to shadowing issues? If a caller
passes arguments that reference a variable named __ret in their own scope,
this uninitialized local variable will shadow the caller's variable, causing
undefined behavior.
> + \
> + sdt_might_sleep_start_timeout(NULL, t); \
> + __ret = __dma_fence_default_wait(f, intr, t); \
[Severity: Medium]
Is it safe to evaluate the timeout argument t multiple times here?
If a caller passes an expression with side effects, such as timeout--, it
would be evaluated twice, which could lead to incorrect timeout logic.
> + sdt_might_sleep_end(); \
> + __ret; \
> +})
> +
[ ... ]
> @@ -628,12 +642,37 @@ static inline ktime_t dma_fence_timestamp(struct dma_fence *fence)
> return fence->timestamp;
> }
>
> -signed long dma_fence_wait_timeout(struct dma_fence *,
> +signed long __dma_fence_wait_timeout(struct dma_fence *,
> bool intr, signed long timeout);
> -signed long dma_fence_wait_any_timeout(struct dma_fence **fences,
> +signed long __dma_fence_wait_any_timeout(struct dma_fence **fences,
> uint32_t count,
> bool intr, signed long timeout,
> uint32_t *idx);
> +/*
> + * Associate every caller with its own dept map.
> + */
> +#define dma_fence_wait_timeout(f, intr, t) \
> +({ \
> + signed long __ret; \
> + \
> + sdt_might_sleep_start_timeout(NULL, t); \
> + __ret = __dma_fence_wait_timeout(f, intr, t); \
[Severity: Medium]
Does this code also suffer from the same macro hygiene issues with shadowing
__ret and evaluating the t argument twice?
> + sdt_might_sleep_end(); \
> + __ret; \
> +})
> +
> +/*
> + * Associate every caller with its own dept map.
> + */
> +#define dma_fence_wait_any_timeout(fpp, count, intr, t, idx) \
> +({ \
> + signed long __ret; \
> + \
> + sdt_might_sleep_start_timeout(NULL, t); \
> + __ret = __dma_fence_wait_any_timeout(fpp, count, intr, t, idx); \
[Severity: Medium]
Would it be safer to use a more unique variable name instead of __ret, and
evaluate t only once before passing it to sdt_might_sleep_start_timeout()
and __dma_fence_wait_any_timeout(), similar to the other macros?
> + sdt_might_sleep_end(); \
> + __ret; \
> +})
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=28
^ permalink raw reply
* Re: [PATCH v19 27/40] dept: assign dept map to mmu notifier invalidation synchronization
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: linux-modules, cassel, linux-kernel, tytso, Chuck Lever,
linux-rt-devel, dri-devel, Jeff Layton, Anna Schumaker, ojeda,
linux-ide, dlemoal
In-Reply-To: <20260706061928.66713-28-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unbalanced DEPT tracking context leak in mmu_notifier_invalidate_range_start_nonblock()
--
commit 10f75b8cdecfe9a5cbc6804a1d8ae1770fc9fe67
Author: Byungchul Park <byungchul@sk.com>
dept: assign dept map to mmu notifier invalidation synchronization
This commit introduces explicit DEPT tracking contexts to resolve false
positive deadlock reports in mmu_notifier invalidation synchronization.
It annotates the start and end of the event contexts delimited by
mmu_notifier_invalidate_range_start() and _end().
> diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h
> --- a/include/linux/mmu_notifier.h
> +++ b/include/linux/mmu_notifier.h
[ ... ]
> @@ -460,6 +474,12 @@ mmu_notifier_invalidate_range_start_nonblock(struct mmu_notifier_range *range)
> ret = __mmu_notifier_invalidate_range_start(range);
> }
> lock_map_release(&__mmu_notifier_invalidate_range_start_map);
> +
> + /*
> + * From now on, waiters could be there by this start until
> + * mmu_notifier_invalidate_range_end().
> + */
> + mmu_notifier_invalidate_dept_ecxt_start(range);
> return ret;
> }
[Severity: Medium]
Does this unconditionally start the DEPT event context even if the
non-blocking start fails?
If mmu_notifier_invalidate_range_start_nonblock() returns an error, callers
typically abort and skip calling mmu_notifier_invalidate_range_end(). For
example, in mm/oom_kill.c:__oom_reap_task_mm():
if (mmu_notifier_invalidate_range_start_nonblock(&range)) {
tlb_finish_mmu(&tlb);
ret = false;
continue;
}
Could this lead to an unbalanced DEPT context being leaked, as the
corresponding mmu_notifier_invalidate_dept_ecxt_end() will never be called
on the error path?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=27
^ permalink raw reply
* Re: [PATCH v19 24/40] locking/lockdep: prevent various lockdep assertions when lockdep_off()'ed
From: sashiko-bot @ 2026-07-07 7:33 UTC (permalink / raw)
To: Byungchul Park
Cc: dri-devel, dlemoal, ojeda, tytso, linux-rt-devel, Jeff Layton,
linux-modules, cassel, linux-ide, linux-kernel, Chuck Lever,
Anna Schumaker
In-Reply-To: <20260706061928.66713-25-byungchul@sk.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch introduces a performance regression on fast paths when CONFIG_PROVE_LOCKING is enabled by adding an out-of-line function call to a macro that is unconditionally evaluated before checking if the assertion actually fails.
--
commit a2a5c1f9c5f68f19a3e991808b6e812dde3fa53a
Author: Byungchul Park <byungchul@sk.com>
locking/lockdep: prevent various lockdep assertions when lockdep_off()'ed
This commit adds a check for lockdep_recursing(current) to the
__lockdep_enabled macro. To avoid header dependency issues, it introduces
an out-of-line function lockdep_recursing_current().
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 60b17a2ff555f..5113b7053b621 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
[ ... ]
> @@ -630,7 +631,7 @@ DECLARE_PER_CPU(int, hardirqs_enabled);
> DECLARE_PER_CPU(int, hardirq_context);
> DECLARE_PER_CPU(unsigned int, lockdep_recursion);
>
> -#define __lockdep_enabled (debug_locks && !this_cpu_read(lockdep_recursion))
> +#define __lockdep_enabled (debug_locks && !this_cpu_read(lockdep_recursion) && !lockdep_recursing_current())
>
> #define lockdep_assert_irqs_enabled() \
> do { \
[Severity: Medium]
Will this out-of-line call introduce measurable overhead on fast paths?
Since debug_locks and !this_cpu_read(lockdep_recursion) are almost always
true in healthy systems, lockdep_recursing_current() will be evaluated every
time __lockdep_enabled is checked.
For example, in lockdep_assert_irqs_enabled():
WARN_ON_ONCE(__lockdep_enabled && !this_cpu_read(hardirqs_enabled));
Because __lockdep_enabled is the first operand, the out-of-line call executes
before checking if the assertion actually fails.
Could we move __lockdep_enabled to the end of the && chain in these
assertions?
WARN_ON_ONCE(!this_cpu_read(hardirqs_enabled) && __lockdep_enabled);
Placing the failure condition first would ensure the out-of-line function is
only evaluated on the slow path when the assertion is already failing, avoiding
the function call overhead on hot paths entirely.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=24
^ permalink raw reply
* 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
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