* Re: [syzbot] [kernel?] INFO: task hung in restrict_one_thread_callback
From: Frederic Weisbecker @ 2026-02-23 13:40 UTC (permalink / raw)
To: syzbot, Mickaël Salaün, Günther Noack, Paul Moore,
James Morris, Serge E. Hallyn, linux-security-module
Cc: anna-maria, linux-kernel, syzkaller-bugs, tglx
In-Reply-To: <69984159.050a0220.21cd75.01bb.GAE@google.com>
Le Fri, Feb 20, 2026 at 03:11:21AM -0800, syzbot a écrit :
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 635c467cc14e Add linux-next specific files for 20260213
> git tree: linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=1452f6e6580000
> kernel config: https://syzkaller.appspot.com/x/.config?x=61690c38d1398936
> dashboard link: https://syzkaller.appspot.com/bug?extid=7ea2f5e9dfd468201817
> compiler: Debian clang version 21.1.8 (++20251221033036+2078da43e25a-1~exp1~20251221153213.50), Debian LLD 21.1.8
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16e41c02580000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15813652580000
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/78b3d15ca8e6/disk-635c467c.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/a95f3d108ef4/vmlinux-635c467c.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/e58086838b24/bzImage-635c467c.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+7ea2f5e9dfd468201817@syzkaller.appspotmail.com
>
> INFO: task syz.0.2812:14643 blocked for more than 143 seconds.
> Not tainted syzkaller #0
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:syz.0.2812 state:D stack:25600 pid:14643 tgid:14643 ppid:13375 task_flags:0x400040 flags:0x00080002
> Call Trace:
> <TASK>
> context_switch kernel/sched/core.c:5295 [inline]
> __schedule+0x1585/0x5340 kernel/sched/core.c:6907
> __schedule_loop kernel/sched/core.c:6989 [inline]
> schedule+0x164/0x360 kernel/sched/core.c:7004
> schedule_timeout+0xc3/0x2c0 kernel/time/sleep_timeout.c:75
> do_wait_for_common kernel/sched/completion.c:100 [inline]
> __wait_for_common kernel/sched/completion.c:121 [inline]
> wait_for_common kernel/sched/completion.c:132 [inline]
> wait_for_completion+0x2cc/0x5e0 kernel/sched/completion.c:153
> restrict_one_thread security/landlock/tsync.c:128 [inline]
> restrict_one_thread_callback+0x320/0x570 security/landlock/tsync.c:162
Seems to be related to landlock security module.
Cc'ing maintainers for awareness.
Thanks.
> task_work_run+0x1d9/0x270 kernel/task_work.c:233
> get_signal+0x11eb/0x1330 kernel/signal.c:2807
> arch_do_signal_or_restart+0xbc/0x830 arch/x86/kernel/signal.c:337
> __exit_to_user_mode_loop kernel/entry/common.c:64 [inline]
> exit_to_user_mode_loop+0x86/0x480 kernel/entry/common.c:98
> __exit_to_user_mode_prepare include/linux/irq-entry-common.h:226 [inline]
> syscall_exit_to_user_mode_prepare include/linux/irq-entry-common.h:256 [inline]
> syscall_exit_to_user_mode include/linux/entry-common.h:325 [inline]
> do_syscall_64+0x32d/0xf80 arch/x86/entry/syscall_64.c:100
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f8d7f19bf79
> RSP: 002b:00007ffe0b192a38 EFLAGS: 00000246 ORIG_RAX: 00000000000000db
> RAX: fffffffffffffdfc RBX: 00000000000389f1 RCX: 00007f8d7f19bf79
> RDX: 0000000000000000 RSI: 0000000000000080 RDI: 00007f8d7f41618c
> RBP: 0000000000000032 R08: 3fffffffffffffff R09: 0000000000000000
> R10: 00007ffe0b192b40 R11: 0000000000000246 R12: 00007ffe0b192b60
> R13: 00007f8d7f41618c R14: 0000000000038a23 R15: 00007ffe0b192b40
> </TASK>
> INFO: task syz.0.2812:14644 blocked for more than 143 seconds.
> Not tainted syzkaller #0
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:syz.0.2812 state:D stack:28216 pid:14644 tgid:14643 ppid:13375 task_flags:0x400040 flags:0x00080002
> Call Trace:
> <TASK>
> context_switch kernel/sched/core.c:5295 [inline]
> __schedule+0x1585/0x5340 kernel/sched/core.c:6907
> __schedule_loop kernel/sched/core.c:6989 [inline]
> schedule+0x164/0x360 kernel/sched/core.c:7004
> schedule_timeout+0xc3/0x2c0 kernel/time/sleep_timeout.c:75
> do_wait_for_common kernel/sched/completion.c:100 [inline]
> __wait_for_common kernel/sched/completion.c:121 [inline]
> wait_for_common kernel/sched/completion.c:132 [inline]
> wait_for_completion+0x2cc/0x5e0 kernel/sched/completion.c:153
> restrict_one_thread security/landlock/tsync.c:128 [inline]
> restrict_one_thread_callback+0x320/0x570 security/landlock/tsync.c:162
> task_work_run+0x1d9/0x270 kernel/task_work.c:233
> get_signal+0x11eb/0x1330 kernel/signal.c:2807
> arch_do_signal_or_restart+0xbc/0x830 arch/x86/kernel/signal.c:337
> __exit_to_user_mode_loop kernel/entry/common.c:64 [inline]
> exit_to_user_mode_loop+0x86/0x480 kernel/entry/common.c:98
> __exit_to_user_mode_prepare include/linux/irq-entry-common.h:226 [inline]
> syscall_exit_to_user_mode_prepare include/linux/irq-entry-common.h:256 [inline]
> syscall_exit_to_user_mode include/linux/entry-common.h:325 [inline]
> do_syscall_64+0x32d/0xf80 arch/x86/entry/syscall_64.c:100
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f8d7f19bf79
> RSP: 002b:00007f8d8007c0e8 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca
> RAX: fffffffffffffe00 RBX: 00007f8d7f415fa8 RCX: 00007f8d7f19bf79
> RDX: 0000000000000000 RSI: 0000000000000080 RDI: 00007f8d7f415fa8
> RBP: 00007f8d7f415fa0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007f8d7f416038 R14: 00007ffe0b1927f0 R15: 00007ffe0b1928d8
> </TASK>
> INFO: task syz.0.2812:14645 blocked for more than 143 seconds.
> Not tainted syzkaller #0
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:syz.0.2812 state:D stack:28648 pid:14645 tgid:14643 ppid:13375 task_flags:0x400140 flags:0x00080006
> Call Trace:
> <TASK>
> context_switch kernel/sched/core.c:5295 [inline]
> __schedule+0x1585/0x5340 kernel/sched/core.c:6907
> __schedule_loop kernel/sched/core.c:6989 [inline]
> schedule+0x164/0x360 kernel/sched/core.c:7004
> schedule_timeout+0xc3/0x2c0 kernel/time/sleep_timeout.c:75
> do_wait_for_common kernel/sched/completion.c:100 [inline]
> __wait_for_common kernel/sched/completion.c:121 [inline]
> wait_for_common kernel/sched/completion.c:132 [inline]
> wait_for_completion+0x2cc/0x5e0 kernel/sched/completion.c:153
> landlock_restrict_sibling_threads+0xe9c/0x11f0 security/landlock/tsync.c:539
> __do_sys_landlock_restrict_self security/landlock/syscalls.c:574 [inline]
> __se_sys_landlock_restrict_self+0x540/0x810 security/landlock/syscalls.c:482
> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> do_syscall_64+0x14d/0xf80 arch/x86/entry/syscall_64.c:94
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f8d7f19bf79
> RSP: 002b:00007f8d8005b028 EFLAGS: 00000246 ORIG_RAX: 00000000000001be
> RAX: ffffffffffffffda RBX: 00007f8d7f416090 RCX: 00007f8d7f19bf79
> RDX: 0000000000000000 RSI: 000000000000000e RDI: 0000000000000003
> RBP: 00007f8d7f2327e0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007f8d7f416128 R14: 00007f8d7f416090 R15: 00007ffe0b1928d8
> </TASK>
> INFO: task syz.0.2812:14646 blocked for more than 144 seconds.
> Not tainted syzkaller #0
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:syz.0.2812 state:D stack:28832 pid:14646 tgid:14643 ppid:13375 task_flags:0x400140 flags:0x00080006
> Call Trace:
> <TASK>
> context_switch kernel/sched/core.c:5295 [inline]
> __schedule+0x1585/0x5340 kernel/sched/core.c:6907
> __schedule_loop kernel/sched/core.c:6989 [inline]
> schedule+0x164/0x360 kernel/sched/core.c:7004
> schedule_timeout+0xc3/0x2c0 kernel/time/sleep_timeout.c:75
> do_wait_for_common kernel/sched/completion.c:100 [inline]
> __wait_for_common kernel/sched/completion.c:121 [inline]
> wait_for_common kernel/sched/completion.c:132 [inline]
> wait_for_completion+0x2cc/0x5e0 kernel/sched/completion.c:153
> landlock_restrict_sibling_threads+0xe9c/0x11f0 security/landlock/tsync.c:539
> __do_sys_landlock_restrict_self security/landlock/syscalls.c:574 [inline]
> __se_sys_landlock_restrict_self+0x540/0x810 security/landlock/syscalls.c:482
> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> do_syscall_64+0x14d/0xf80 arch/x86/entry/syscall_64.c:94
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f8d7f19bf79
> RSP: 002b:00007f8d8003a028 EFLAGS: 00000246 ORIG_RAX: 00000000000001be
> RAX: ffffffffffffffda RBX: 00007f8d7f416180 RCX: 00007f8d7f19bf79
> RDX: 0000000000000000 RSI: 000000000000000e RDI: 0000000000000003
> RBP: 00007f8d7f2327e0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007f8d7f416218 R14: 00007f8d7f416180 R15: 00007ffe0b1928d8
> </TASK>
>
> Showing all locks held in the system:
> 1 lock held by khungtaskd/31:
> #0: ffffffff8e9602e0 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire include/linux/rcupdate.h:312 [inline]
> #0: ffffffff8e9602e0 (rcu_read_lock){....}-{1:3}, at: rcu_read_lock include/linux/rcupdate.h:850 [inline]
> #0: ffffffff8e9602e0 (rcu_read_lock){....}-{1:3}, at: debug_show_all_locks+0x2e/0x180 kernel/locking/lockdep.c:6775
> 2 locks held by getty/5581:
> #0: ffff8880328890a0 (&tty->ldisc_sem){++++}-{0:0}, at: tty_ldisc_ref_wait+0x25/0x70 drivers/tty/tty_ldisc.c:243
> #1: ffffc9000332b2f0 (&ldata->atomic_read_lock){+.+.}-{4:4}, at: n_tty_read+0x45c/0x13c0 drivers/tty/n_tty.c:2211
>
> =============================================
>
> NMI backtrace for cpu 0
> CPU: 0 UID: 0 PID: 31 Comm: khungtaskd Not tainted syzkaller #0 PREEMPT(full)
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/12/2026
> Call Trace:
> <TASK>
> dump_stack_lvl+0xe8/0x150 lib/dump_stack.c:120
> nmi_cpu_backtrace+0x274/0x2d0 lib/nmi_backtrace.c:113
> nmi_trigger_cpumask_backtrace+0x17a/0x300 lib/nmi_backtrace.c:62
> trigger_all_cpu_backtrace include/linux/nmi.h:161 [inline]
> __sys_info lib/sys_info.c:157 [inline]
> sys_info+0x135/0x170 lib/sys_info.c:165
> check_hung_uninterruptible_tasks kernel/hung_task.c:346 [inline]
> watchdog+0xfd9/0x1030 kernel/hung_task.c:515
> kthread+0x388/0x470 kernel/kthread.c:467
> ret_from_fork+0x51e/0xb90 arch/x86/kernel/process.c:158
> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
> </TASK>
> Sending NMI from CPU 0 to CPUs 1:
> NMI backtrace for cpu 1
> CPU: 1 UID: 0 PID: 86 Comm: kworker/u8:5 Not tainted syzkaller #0 PREEMPT(full)
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/12/2026
> Workqueue: events_unbound nsim_dev_trap_report_work
> RIP: 0010:native_save_fl arch/x86/include/asm/irqflags.h:26 [inline]
> RIP: 0010:arch_local_save_flags arch/x86/include/asm/irqflags.h:109 [inline]
> RIP: 0010:arch_local_irq_save arch/x86/include/asm/irqflags.h:127 [inline]
> RIP: 0010:lock_acquire+0xab/0x2e0 kernel/locking/lockdep.c:5864
> Code: 84 c1 00 00 00 65 8b 05 73 b8 9f 11 85 c0 0f 85 b2 00 00 00 65 48 8b 05 bb 72 9f 11 83 b8 14 0b 00 00 00 0f 85 9d 00 00 00 9c <5b> fa 48 c7 c7 8f a1 02 8e e8 57 40 17 0a 65 ff 05 40 b8 9f 11 45
> RSP: 0018:ffffc9000260f498 EFLAGS: 00000246
> RAX: ffff88801df81e40 RBX: ffffffff818f9166 RCX: 0000000080000002
> RDX: 0000000000000000 RSI: ffffffff8176da62 RDI: 1ffffffff1d2c05c
> RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> R10: ffffc9000260f638 R11: ffffffff81b11580 R12: 0000000000000002
> R13: ffffffff8e9602e0 R14: 0000000000000000 R15: 0000000000000000
> FS: 0000000000000000(0000) GS:ffff88812510b000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fe09b2c1ff8 CR3: 000000000e74c000 CR4: 00000000003526f0
> Call Trace:
> <TASK>
> rcu_lock_acquire include/linux/rcupdate.h:312 [inline]
> rcu_read_lock include/linux/rcupdate.h:850 [inline]
> class_rcu_constructor include/linux/rcupdate.h:1193 [inline]
> unwind_next_frame+0xc2/0x23c0 arch/x86/kernel/unwind_orc.c:495
> arch_stack_walk+0x11b/0x150 arch/x86/kernel/stacktrace.c:25
> stack_trace_save+0xa9/0x100 kernel/stacktrace.c:122
> kasan_save_stack mm/kasan/common.c:57 [inline]
> kasan_save_track+0x3e/0x80 mm/kasan/common.c:78
> unpoison_slab_object mm/kasan/common.c:340 [inline]
> __kasan_slab_alloc+0x6c/0x80 mm/kasan/common.c:366
> kasan_slab_alloc include/linux/kasan.h:253 [inline]
> slab_post_alloc_hook mm/slub.c:4501 [inline]
> slab_alloc_node mm/slub.c:4830 [inline]
> kmem_cache_alloc_node_noprof+0x384/0x690 mm/slub.c:4882
> __alloc_skb+0x1d0/0x7d0 net/core/skbuff.c:702
> alloc_skb include/linux/skbuff.h:1383 [inline]
> nsim_dev_trap_skb_build drivers/net/netdevsim/dev.c:819 [inline]
> nsim_dev_trap_report drivers/net/netdevsim/dev.c:876 [inline]
> nsim_dev_trap_report_work+0x29a/0xb80 drivers/net/netdevsim/dev.c:922
> process_one_work+0x949/0x1650 kernel/workqueue.c:3279
> process_scheduled_works kernel/workqueue.c:3362 [inline]
> worker_thread+0xb46/0x1140 kernel/workqueue.c:3443
> kthread+0x388/0x470 kernel/kthread.c:467
> ret_from_fork+0x51e/0xb90 arch/x86/kernel/process.c:158
> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
> </TASK>
>
>
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
>
> If the report is already addressed, let syzbot know by replying with:
> #syz fix: exact-commit-title
>
> If you want syzbot to run the reproducer, reply with:
> #syz test: git://repo/address.git branch-or-commit-hash
> If you attach or paste a git patch, syzbot will apply it before testing.
>
> If you want to overwrite report's subsystems, reply with:
> #syz set subsystems: new-subsystem
> (See the list of subsystem names on the web dashboard)
>
> If the report is a duplicate of another one, reply with:
> #syz dup: exact-subject-of-another-report
>
> If you want to undo deduplication, reply with:
> #syz undup
--
Frederic Weisbecker
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 06/15] selinux: Use simple_start_creating() / simple_done_creating()
From: Chris Mason @ 2026-02-23 13:24 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong, linux-kernel, netfs,
linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-7-neilb@ownmail.net>
NeilBrown <neilb@ownmail.net> wrote:
> From: NeilBrown <neil@brown.name>
>
> Instead of explicitly locking the parent and performing a lookup in
> selinux, use simple_start_creating(), and then use
> simple_done_creating() to unlock.
>
Hi everyone,
Amir suggested I run these through, and this commit was flagged:
commit bc981629767980bbaee2ca3c7348b5a648665a9d
Author: NeilBrown <neil@brown.name>
selinux: Use simple_start_creating() / simple_done_creating()
Instead of explicitly locking the parent and performing a lookup in
selinux, use simple_start_creating(), and then use
simple_done_creating() to unlock.
This extends the region that the directory is locked for, and also
performs a lookup. The lock extension is of no real consequence.
The lookup uses simple_lookup() and so always succeeds. Thus when
d_make_persistent() is called the dentry will already be hashed.
d_make_persistent() handles this case.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 3245cc531555b..7d4f90e5b12a0 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1931,15 +1931,16 @@ static const struct inode_operations swapover_dir_inode_operations = {
> static struct dentry *sel_make_swapover_dir(struct super_block *sb,
> unsigned long *ino)
> {
> - struct dentry *dentry = d_alloc_name(sb->s_root, ".swapover");
> + struct dentry *dentry;
> struct inode *inode;
>
> - if (!dentry)
> + inode = sel_make_inode(sb, S_IFDIR);
> + if (!inode)
> return ERR_PTR(-ENOMEM);
>
> - inode = sel_make_inode(sb, S_IFDIR);
> - if (!inode) {
> - dput(dentry);
> + dentry = simple_start_creating(sb->s_root, ".swapover");
> + if (!dentry) {
> + iput(inode);
> return ERR_PTR(-ENOMEM);
> }
Can this cause a kernel crash when simple_start_creating() fails?
The function simple_start_creating() returns ERR_PTR on failure, not
NULL. The check `if (!dentry)` only catches NULL values. When
simple_start_creating() fails and returns an ERR_PTR value like
ERR_PTR(-ENOMEM), this value is non-NULL and the error check is bypassed.
Execution would continue to d_make_persistent(dentry, inode), which
attempts to lock dentry->d_lock, dereferencing the ERR_PTR value and
causing a kernel crash.
The old code used d_alloc_name() which returns NULL on error. The new
code uses simple_start_creating() which returns ERR_PTR on error, but
the error checking pattern wasn't updated to match.
Should the check be `if (IS_ERR(dentry))` instead, and return dentry
rather than ERR_PTR(-ENOMEM) to preserve the actual error code?
>
> @@ -1947,11 +1948,9 @@ static struct dentry *sel_make_swapover_dir(struct super_block *sb,
> inode->i_ino = ++(*ino);
> /* directory inodes start off with i_nlink == 2 (for "." entry) */
> inc_nlink(inode);
> - inode_lock(sb->s_root->d_inode);
> d_make_persistent(dentry, inode);
> inc_nlink(sb->s_root->d_inode);
> - inode_unlock(sb->s_root->d_inode);
> - dput(dentry);
> + simple_done_creating(dentry);
> return dentry; // borrowed
> }
^ permalink raw reply
* Re: [PATCH v2 12/15] ovl: change ovl_create_real() to get a new lock when re-opening created file.
From: Chris Mason @ 2026-02-23 13:23 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong, linux-kernel, netfs,
linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-13-neilb@ownmail.net>
NeilBrown <neilb@ownmail.net> wrote:
> From: NeilBrown <neil@brown.name>
>
> When ovl_create_real() is used to create a file on the upper filesystem
> it needs to return the resulting dentry - positive and hashed.
> It is usually the case the that dentry passed to the create function
> (e.g. vfs_create()) will be suitable but this is not guaranteed. The
> filesystem may unhash that dentry forcing a repeat lookup next time the
> name is wanted.
>
Hi everyone,
Amir suggested I run these through, and this commit was flagged:
commit 62d49d1e44667e4f93bec415faabec5526992ac0
Author: NeilBrown <neil@brown.name>
ovl: change ovl_create_real() to get a new lock when re-opening created file.
This commit changes ovl_create_real() to drop the directory lock and
reacquire a new lock for lookup when the created dentry is unhashed. It
also removes ovl_lookup_upper() which is no longer used.
Signed-off-by: NeilBrown <neil@brown.name>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index c4feb89ad1e39..6285069ccc590 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -159,7 +159,8 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> }
>
> struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> - struct dentry *newdentry, struct ovl_cattr *attr)
> + struct dentry *newdentry, struct qstr *qname,
> + struct ovl_cattr *attr)
> {
> struct inode *dir = parent->d_inode;
> int err;
> @@ -221,19 +222,29 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> struct dentry *d;
> /*
> * Some filesystems (i.e. casefolded) may return an unhashed
> - * negative dentry from the ovl_lookup_upper() call before
> + * negative dentry from the ovl_start_creating_upper() call before
> * ovl_create_real().
> * In that case, lookup again after making the newdentry
> * positive, so ovl_create_upper() always returns a hashed
> - * positive dentry.
> + * positive dentry. We lookup using qname which should be
> + * the same name as newentry, but is certain not to change.
> + * As we have to drop the lock before the lookup a race
> + * could result in a lookup failure. In that case we return
> + * an error.
> */
> - d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> - newdentry->d_name.len);
> - dput(newdentry);
> - if (IS_ERR_OR_NULL(d))
> + end_creating_keep(newdentry);
> + d = ovl_start_creating_upper(ofs, parent, qname);
> +
> + if (IS_ERR_OR_NULL(d)) {
> err = d ? PTR_ERR(d) : -ENOENT;
> + } else if (d->d_inode != newdentry->d_inode) {
> + err = -EIO;
> + dput(newdentry);
> + } else {
> + dput(newdentry);
> return d;
> + }
> + return ERR_PTR(err);
> }
Does this leak the newdentry reference when IS_ERR_OR_NULL(d) is true?
The code now calls end_creating_keep(newdentry) which keeps the reference,
then calls ovl_start_creating_upper(). If that returns an error or NULL,
the function returns ERR_PTR(err) without cleaning up newdentry:
end_creating_keep(newdentry);
d = ovl_start_creating_upper(ofs, parent, qname);
if (IS_ERR_OR_NULL(d)) {
err = d ? PTR_ERR(d) : -ENOENT;
} ...
return ERR_PTR(err);
The old code always went through the 'out:' label which called
end_creating(newdentry) on error. The new code bypasses that cleanup.
Does this leak both the parent lock and the dentry 'd' when the lookup
returns a different inode?
When d->d_inode != newdentry->d_inode, the code sets err = -EIO and calls
dput(newdentry), but doesn't clean up 'd':
} else if (d->d_inode != newdentry->d_inode) {
err = -EIO;
dput(newdentry);
} ...
return ERR_PTR(err);
The dentry 'd' was returned by ovl_start_creating_upper() with the parent
directory lock held. The code needs to call end_creating(d) to release the
lock via end_dirop()->inode_unlock() and release the dentry reference via
end_dirop()->dput().
^ permalink raw reply
* Re: [PATCH v2 09/15] ovl: Simplify ovl_lookup_real_one()
From: Chris Mason @ 2026-02-23 13:13 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong, linux-kernel, netfs,
linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-10-neilb@ownmail.net>
NeilBrown <neilb@ownmail.net> wrote:
> From: NeilBrown <neil@brown.name>
>
> The primary purpose of this patch is to remove the locking from
> ovl_lookup_real_one() as part of centralising all locking of directories
> for name operations.
>
> The locking here isn't needed. By performing consistency tests after
> the lookup we can be sure that the result of the lookup was valid at
> least for a moment, which is all the original code promised.
>
> lookup_noperm_unlocked() is used for the lookup and it will take the
> lock if needed only where it is needed.
>
> Also:
> - don't take a reference to real->d_parent. The parent is
> only use for a pointer comparison, and no reference is needed for
> that.
> - Several "if" statements have a "goto" followed by "else" - the
> else isn't needed: the following statement can directly follow
> the "if" as a new statement
> - Use a consistent pattern of setting "err" before performing a test
> and possibly going to "fail".
> - remove the "out" label (now that we don't need to dput(parent) or
> unlock) and simply return from fail:.
Hi everyone,
Amir asked me to run these through, and this commit was flagged. I recently
changed the prompts to include some vfs knowledge:
https://github.com/masoncl/review-prompts/blob/main/kernel/subsystem/vfs.md
And I also put in some words to make it prefer reporting problems that might
not be bugs, but that do violate some rule in the .md files. This might
be a false positive from that new shift toward blindly trusting the guides,
but I'll let everyone here decide:
commit d1bfcef048896cdc9e4c57e3249b6ad344d64c02
Author: NeilBrown <neil@brown.name>
ovl: Simplify ovl_lookup_real_one()
This patch removes locking from ovl_lookup_real_one() and reorganizes the
control flow. The function now uses lookup_noperm_unlocked() instead of
lookup_noperm() and performs consistency validation after the lookup rather
than holding a lock during the operation.
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 83f80fdb15674..b448fc9424b64 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -349,69 +349,64 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
> return NULL;
> }
>
> -/*
> - * Lookup a child overlay dentry to get a connected overlay dentry whose real
> - * dentry is @real. If @real is on upper layer, we lookup a child overlay
> - * dentry with the same name as the real dentry. Otherwise, we need to consult
> - * index for lookup.
> +/**
> + * ovl_lookup_real_one - Lookup a child overlay dentry to get an overlay dentry whose real dentry is given
> + * @connected: parent overlay dentry
> + * @real: given child real dentry
> + * @layer: layer in which @real exists
> + *
> + *
> + * Lookup a child overlay dentry in @connected with the same name as the @real
> + * dentry. Then check that the parent of the result is the real dentry for
> + * @connected, and @real is the real dentry for the result.
> + *
> + * Returns:
> + * %-ECHILD if the parent of @real is no longer the real dentry for @connected.
> + * %-ESTALE if @real is no the real dentry of the found dentry.
^^
There's a typo here: "is no the" should be "is not the".
> + * Otherwise the found dentry is returned.
> */
> static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> struct dentry *real,
> const struct ovl_layer *layer)
> {
> - struct inode *dir = d_inode(connected);
> - struct dentry *this, *parent = NULL;
> + struct dentry *this;
> struct name_snapshot name;
> int err;
>
> /*
> - * Lookup child overlay dentry by real name. The dir mutex protects us
> - * from racing with overlay rename. If the overlay dentry that is above
> - * real has already been moved to a parent that is not under the
> - * connected overlay dir, we return -ECHILD and restart the lookup of
> - * connected real path from the top.
> - */
> - inode_lock_nested(dir, I_MUTEX_PARENT);
> - err = -ECHILD;
> - parent = dget_parent(real);
> - if (ovl_dentry_real_at(connected, layer->idx) != parent)
> - goto fail;
> -
> - /*
> - * We also need to take a snapshot of real dentry name to protect us
> + * We need to take a snapshot of real dentry name to protect us
> * from racing with underlying layer rename. In this case, we don't
> * care about returning ESTALE, only from dereferencing a free name
> * pointer because we hold no lock on the real dentry.
> */
> take_dentry_name_snapshot(&name, real);
> - /*
> - * No idmap handling here: it's an internal lookup.
> - */
> - this = lookup_noperm(&name.name, connected);
> + this = lookup_noperm_unlocked(&name.name, connected);
> release_dentry_name_snapshot(&name);
> +
> + err = -ECHILD;
> + if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> + goto fail;
> +
A subsystem pattern flags this as potentially concerning: the VFS subsystem
guide indicates that TOCTOU validation checking d_parent should also check
d_unhashed() because a dentry can be unhashed (removed from dcache by
concurrent operations) while its d_parent pointer remains unchanged.
The old code held inode_lock_nested(dir, I_MUTEX_PARENT) during the entire
lookup and validation, which serialized with operations that could unhash the
dentry. The new lockless pattern creates a window where:
1. lookup_noperm_unlocked() completes and returns dentry `this`
2. A concurrent unlink/remove operation unhashes `this`
3. The d_parent validation passes (parent pointer unchanged)
4. The function returns an unhashed dentry
Other overlayfs code uses the complete pattern. For example, in
ovl_lock_rename_workdir() at fs/overlayfs/util.c:
if (work->d_parent != workdir || d_unhashed(work)) {
...
}
Could this validation miss a race where the dentry is unhashed but d_parent
still matches? The subsequent checks (!this->d_inode for -ENOENT,
wrong real dentry for -ESTALE) might catch some cases, but is that
sufficient for all the scenarios where an unhashed dentry could cause
problems?
> err = PTR_ERR(this);
> - if (IS_ERR(this)) {
> + if (IS_ERR(this))
> goto fail;
> - } else if (!this || !this->d_inode) {
> - dput(this);
> - err = -ENOENT;
> +
> + err = -ENOENT;
> + if (!this || !this->d_inode)
> goto fail;
> - } else if (ovl_dentry_real_at(this, layer->idx) != real) {
> - dput(this);
> - err = -ESTALE;
> +
> + err = -ESTALE;
> + if (ovl_dentry_real_at(this, layer->idx) != real)
> goto fail;
> - }
>
> -out:
> - dput(parent);
> - inode_unlock(dir);
> return this;
>
> fail:
> pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
> real, layer->idx, connected, err);
> - this = ERR_PTR(err);
> - goto out;
> + if (!IS_ERR(this))
> + dput(this);
> + return ERR_PTR(err);
> }
>
> static struct dentry *ovl_lookup_real(struct super_block *sb,
^ permalink raw reply
* Re: [syzbot] [kernel?] INFO: task hung in restrict_one_thread_callback
From: Ding Yihan @ 2026-02-23 11:29 UTC (permalink / raw)
To: Günther Noack
Cc: syzbot, Mickaël Salaün, linux-security-module,
Jann Horn, Paul Moore
In-Reply-To: <20260223.52c45aed20f8@gnoack.org>
Hi Günther,
Thank you for the detailed analysis and the clear breakdown.
Apologies for the delayed response. I spent the last couple of days
thoroughly reading through the previous mailing list discussions. I
was trying hard to see if there was any viable pure lockless design
that could solve this concurrency issue while preserving the original
architecture.
However, after looking at the complexities you outlined, I completely
agree with your conclusion: serializing the TSYNC operations is indeed
the most robust and reasonable path forward to prevent the deadlock.
Regarding the lock choice, since 'cred_guard_mutex' is explicitly
marked as deprecated for new code in the kernel,maybe we can use its
modern replacement: 'exec_update_lock' (using down_write_trylock /
up_write on current->signal). This aligns with the current subsystem
standards and was also briefly touched upon by Jann in the older
discussions.
I fully understand the requirement for the two-part patch series:
1. Cleaning up the cancellation logic and comments.
2. Introducing the serialization lock for TSYNC.
I will take some time to draft and test this patch series properly.
I also plan to discuss this with my kernel colleagues here at
UnionTech to see if they have any additional suggestions on the
implementation details before I submit it.
I will send out the v1 patch series to the list as soon as it is
ready. Thanks again for your guidance and the great discussion!
Best regards,
Yihan Ding
在 2026/2/23 17:42, Günther Noack 写道:
> On Sat, Feb 21, 2026 at 02:19:53PM +0100, Günther Noack wrote:
>> OK, I think I understand now. Our existing recovery code for this
>> conflict is this:
>>
>> /*
>> * Decrement num_preparing for current, to undo that we initialized it
>> * to 1 a few lines above.
>> */
>> if (atomic_dec_return(&shared_ctx.num_preparing) > 0) {
>> if (wait_for_completion_interruptible(
>> &shared_ctx.all_prepared)) {
>> /* In case of interruption, we need to retry the system call. */
>> atomic_set(&shared_ctx.preparation_error,
>> -ERESTARTNOINTR);
>>
>> /*
>> * Cancel task works for tasks that did not start running yet,
>> * and decrement all_prepared and num_unfinished accordingly.
>> */
>> cancel_tsync_works(&works, &shared_ctx);
>>
>> /*
>> * The remaining task works have started running, so waiting for
>> * their completion will finish.
>> */
>> wait_for_completion(&shared_ctx.all_prepared);
>> }
>> }
>>
>> When I wrote this, I assumed, as the last comment states, that the
>> task works which we could not cancel, are already running.
>>
>> I was wrong there, because I had misunderstood task_work_run(). When
>> the task works get run there, it first *atomically dequeues the entire
>> queue of scheduled task works*, and then runs them sequentially.
>>
>> That is why, if we have one task work that belongs to the first
>> landlock_restrict_self() call and one which belongs to the other, the
>> task work which is scheduled later can (a) not be dequeued with
>> cancel_tsync_works() any more, and (b) also has not started running
>> yet.
>>
>> Now the only thing that is necessary to produce the deadlock is that
>> we have a pair of threads where the task works for the restriction
>> calls have been scheduled in different order. When the two
>> landlock_restrict_self() calls end up in the recovery path quoted
>> above, they will wait for one of their task works to run which is
>> blocked from running by another task work that is scheduled before and
>> does not finish either.
>>
>> (Just pasting a brain dump here to save you some time hunting for the
>> root cause. I don't know the best solution yet either.)
>
> Let me propose the following fixes:
>
> 1. Immediate fix for that specific issue
> ----------------------------------------
>
> Proposal:
> * Remove the wait_for_completion(&shared_ctx.all_prepared)
> call in the code snippet above.
> * Rewrite surrounding comments: Be clear about the fact that
> cancel_tsync_works() is an opportunistic improvement, but we don't
> have a guarantee at all that it cancels any of the enqueued task
> works (because task_work_run might already have popped them off).
>
> This removes the hold-and-wait dependency circle between the threads,
> which produces the observed deadlock. The way that we shut down now
> is that we exit the main loop (happens already without it, but we
> might also "break" to be explicit).
>
> I think that this fix or an equivalent one is needed here, because in
> either way, our assumptions in the quoted code above were wrong.
>
>
> 2. Can we reason constructively about correctness?
> --------------------------------------------------
>
> The remaining question: If on the shutdown path, we can not actually
> remove all the enqueued task works, under what circumstances are we
> even able to interrupt and return from the landlock_restrict_self()
> system call?
>
> 2.1 For n competing restrict_self calls, n-1 of them need to get interrupted
> ----------------------------------------------------------------------------
>
> To answer this, consider a multithreaded process with threads named
> "red", "green" and "blue" and many additional threads: When "red",
> "green" and "blue" enforce landlock_restrict_self() concurrently, due
> to differing iteration order, we might end up enqueueing the task
> works on other threads in all of the following combinations:
>
> t0: R G B <- front of queue
> t1: R B G
> t2: G R B
> t3: G B R
> t4: B R G
> t5: B G R
>
> In this configuration, for any of the landlock_restrict_self() system
> calls to even return (successfully or unsuccessfully), at least two
> threads must receive an interrupt and therefore remove their enqueued
> task works from the front of the queue. Assuming those are green and
> blue, we get:
>
> t0: R <- front of queue
> t1: R
> t2: G R
> t3: G B R
> t4: B R
> t5: B G R
>
> (This works because after the patch above, all of the enqueued G and B
> works finish even if there are remaining G and B works that are still
> blocked by an "R" entry.)
>
> Now, "R" is in the front of the queue, and the
> landlock_restrict_self() call for the red thread can finish normally,
> even without it being interrupted.
>
> Once the "R" task works are done as well, the remaining G and B works
> can run and finish as well.
>
> This scheme generalizes: If we have n competing
> landlock_restrict_self() calls, then in worst case, at least n-1 of
> these system calls need to be interrupted so that they can all
> terminate.
>
> 2.2 Can we guarantee that two system calls get interrupted?
> -----------------------------------------------------------
>
> In case of competing landlock_restrict_self() calls, I think it is
> possible that not all relevant system calls get seen. The scenario is
> one where we have a "red" and "green" thread calling
> landlock_restrict_self().
>
> (a set of additional threads)
> t0: task_works: R G
> t1: task_works: G R
> tR: red thread
> tG: green thread
>
> In the red thread, the following happens:
> * Under RCU, count the number of total threads => get a low number
> * Allocate space for that number of task_works
> * Under RCU
> * Enqueue "R" into t0 and t1
> * Enqueue "R" for some of the "additional threads"
> * But we do not have enough pre-allocated space to enqueue "R" for
> the green thread tG.
>
> The same thing happens in the green thread as well.
>
> The result is that we still have a deadlock between t0 and t1, but
> neither the red nor the green thread get interrupted so that they can
> resolve it.
>
> (FWIW, you could resolve it from the outside by sending a signal to
> the red or green thread manually, but it is not guaranteed to happen
> on its own.)
>
> Caveat: I am making pessimistic assumptions about the iteration order
> of the task list here, and I am assuming that the number of
> "additional threads" is swinging up and down during the competing
> enforcement, so that the enforcing threads are mis-approximating the
> required space for memory pre-allocation.
>
> 2.3 Possible resolutions
> ------------------------
>
> * We could try to interrupt all sibling threads during the teardown,
> to fix the issue discussed in 2.2. (Downside: Complicated, more
> expensive)
> * The reason why landlock_restrict_self() can't return is because it
> needs to wait until all task works are done before it can free the
> memory. Alternatively, we could make the task works take ownership
> of these memory structures (refcounting the shared_ctx). (Downside:
> The used memory is not linear to the number of threads any more.)
>
> Side remark: In testing, I had the impression that the
> landlock_restrict_self() calls can go into a retry loop for a while
> where all competing threads get interrupted all the time; in a debug
> build, when the Syzkaller test prints out a line for each attempt,
> sometimes it was hanging for seconds and *then* resolving itself
> again.
>
> 3 Conclusion
> ---------------
>
> I would prefer if the final solution would not require deadlock
> reasoning at that level and we could do it in simpler way. I
> therefore propose to do what Ding Yihan suggested, and what we had
> also discussed previously in the code review:
>
> * Let's serialize the landlock_restrict_self()-with-TSYNC operations
> through the cred_guard_mutex.
>
> This will resolve the issue where competing landlock_restrict_self()
> calls with TSYNC can deadlock. It will also remove the jittery
> behavior for that worst case where the conflict is resolved through
> retry.
>
>
> So in my mind, we need both patches:
>
> * The fix to the cleanup path from 1. above, to make interruption
> work more reliably and to correct the misunderstandings in the
> comments.
> * cred_guard_mutex to serialize the TSYNC invocations.
>
> Please let me know what you think.
>
> Thanks,
> –Günther
>
^ permalink raw reply
* Re: [PATCH v2 09/15] ovl: Simplify ovl_lookup_real_one()
From: Amir Goldstein @ 2026-02-23 9:49 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
Darrick J. Wong, linux-kernel, netfs, linux-fsdevel, linux-nfs,
linux-unionfs, apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-10-neilb@ownmail.net>
On Mon, Feb 23, 2026 at 2:13 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> The primary purpose of this patch is to remove the locking from
> ovl_lookup_real_one() as part of centralising all locking of directories
> for name operations.
>
> The locking here isn't needed. By performing consistency tests after
> the lookup we can be sure that the result of the lookup was valid at
> least for a moment, which is all the original code promised.
>
> lookup_noperm_unlocked() is used for the lookup and it will take the
> lock if needed only where it is needed.
>
> Also:
> - don't take a reference to real->d_parent. The parent is
> only use for a pointer comparison, and no reference is needed for
> that.
> - Several "if" statements have a "goto" followed by "else" - the
> else isn't needed: the following statement can directly follow
> the "if" as a new statement
> - Use a consistent pattern of setting "err" before performing a test
> and possibly going to "fail".
> - remove the "out" label (now that we don't need to dput(parent) or
> unlock) and simply return from fail:.
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/export.c | 71 ++++++++++++++++++++-----------------------
> 1 file changed, 33 insertions(+), 38 deletions(-)
>
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 83f80fdb1567..b448fc9424b6 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -349,69 +349,64 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
> return NULL;
> }
>
> -/*
> - * Lookup a child overlay dentry to get a connected overlay dentry whose real
> - * dentry is @real. If @real is on upper layer, we lookup a child overlay
> - * dentry with the same name as the real dentry. Otherwise, we need to consult
> - * index for lookup.
> +/**
> + * ovl_lookup_real_one - Lookup a child overlay dentry to get an overlay dentry whose real dentry is given
> + * @connected: parent overlay dentry
> + * @real: given child real dentry
> + * @layer: layer in which @real exists
> + *
> + *
> + * Lookup a child overlay dentry in @connected with the same name as the @real
> + * dentry. Then check that the parent of the result is the real dentry for
> + * @connected, and @real is the real dentry for the result.
> + *
> + * Returns:
> + * %-ECHILD if the parent of @real is no longer the real dentry for @connected.
> + * %-ESTALE if @real is no the real dentry of the found dentry.
> + * Otherwise the found dentry is returned.
> */
> static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> struct dentry *real,
> const struct ovl_layer *layer)
> {
> - struct inode *dir = d_inode(connected);
> - struct dentry *this, *parent = NULL;
> + struct dentry *this;
> struct name_snapshot name;
> int err;
>
> /*
> - * Lookup child overlay dentry by real name. The dir mutex protects us
> - * from racing with overlay rename. If the overlay dentry that is above
> - * real has already been moved to a parent that is not under the
> - * connected overlay dir, we return -ECHILD and restart the lookup of
> - * connected real path from the top.
> - */
> - inode_lock_nested(dir, I_MUTEX_PARENT);
> - err = -ECHILD;
> - parent = dget_parent(real);
> - if (ovl_dentry_real_at(connected, layer->idx) != parent)
> - goto fail;
> -
> - /*
> - * We also need to take a snapshot of real dentry name to protect us
> + * We need to take a snapshot of real dentry name to protect us
> * from racing with underlying layer rename. In this case, we don't
> * care about returning ESTALE, only from dereferencing a free name
> * pointer because we hold no lock on the real dentry.
> */
> take_dentry_name_snapshot(&name, real);
> - /*
> - * No idmap handling here: it's an internal lookup.
> - */
> - this = lookup_noperm(&name.name, connected);
> + this = lookup_noperm_unlocked(&name.name, connected);
> release_dentry_name_snapshot(&name);
> +
> + err = -ECHILD;
> + if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> + goto fail;
> +
> err = PTR_ERR(this);
> - if (IS_ERR(this)) {
> + if (IS_ERR(this))
> goto fail;
> - } else if (!this || !this->d_inode) {
> - dput(this);
> - err = -ENOENT;
> +
> + err = -ENOENT;
> + if (!this || !this->d_inode)
> goto fail;
> - } else if (ovl_dentry_real_at(this, layer->idx) != real) {
> - dput(this);
> - err = -ESTALE;
> +
> + err = -ESTALE;
> + if (ovl_dentry_real_at(this, layer->idx) != real)
> goto fail;
> - }
>
> -out:
> - dput(parent);
> - inode_unlock(dir);
> return this;
>
> fail:
> pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
> real, layer->idx, connected, err);
> - this = ERR_PTR(err);
> - goto out;
> + if (!IS_ERR(this))
> + dput(this);
> + return ERR_PTR(err);
> }
>
> static struct dentry *ovl_lookup_real(struct super_block *sb,
> --
> 2.50.0.107.gf914562f5916.dirty
>
^ permalink raw reply
* Re: [syzbot] [kernel?] INFO: task hung in restrict_one_thread_callback
From: Günther Noack @ 2026-02-23 9:42 UTC (permalink / raw)
To: Ding Yihan
Cc: syzbot, Mickaël Salaün, linux-security-module,
Jann Horn, Paul Moore
In-Reply-To: <20260221.3ff0e30e4010@gnoack.org>
On Sat, Feb 21, 2026 at 02:19:53PM +0100, Günther Noack wrote:
> OK, I think I understand now. Our existing recovery code for this
> conflict is this:
>
> /*
> * Decrement num_preparing for current, to undo that we initialized it
> * to 1 a few lines above.
> */
> if (atomic_dec_return(&shared_ctx.num_preparing) > 0) {
> if (wait_for_completion_interruptible(
> &shared_ctx.all_prepared)) {
> /* In case of interruption, we need to retry the system call. */
> atomic_set(&shared_ctx.preparation_error,
> -ERESTARTNOINTR);
>
> /*
> * Cancel task works for tasks that did not start running yet,
> * and decrement all_prepared and num_unfinished accordingly.
> */
> cancel_tsync_works(&works, &shared_ctx);
>
> /*
> * The remaining task works have started running, so waiting for
> * their completion will finish.
> */
> wait_for_completion(&shared_ctx.all_prepared);
> }
> }
>
> When I wrote this, I assumed, as the last comment states, that the
> task works which we could not cancel, are already running.
>
> I was wrong there, because I had misunderstood task_work_run(). When
> the task works get run there, it first *atomically dequeues the entire
> queue of scheduled task works*, and then runs them sequentially.
>
> That is why, if we have one task work that belongs to the first
> landlock_restrict_self() call and one which belongs to the other, the
> task work which is scheduled later can (a) not be dequeued with
> cancel_tsync_works() any more, and (b) also has not started running
> yet.
>
> Now the only thing that is necessary to produce the deadlock is that
> we have a pair of threads where the task works for the restriction
> calls have been scheduled in different order. When the two
> landlock_restrict_self() calls end up in the recovery path quoted
> above, they will wait for one of their task works to run which is
> blocked from running by another task work that is scheduled before and
> does not finish either.
>
> (Just pasting a brain dump here to save you some time hunting for the
> root cause. I don't know the best solution yet either.)
Let me propose the following fixes:
1. Immediate fix for that specific issue
----------------------------------------
Proposal:
* Remove the wait_for_completion(&shared_ctx.all_prepared)
call in the code snippet above.
* Rewrite surrounding comments: Be clear about the fact that
cancel_tsync_works() is an opportunistic improvement, but we don't
have a guarantee at all that it cancels any of the enqueued task
works (because task_work_run might already have popped them off).
This removes the hold-and-wait dependency circle between the threads,
which produces the observed deadlock. The way that we shut down now
is that we exit the main loop (happens already without it, but we
might also "break" to be explicit).
I think that this fix or an equivalent one is needed here, because in
either way, our assumptions in the quoted code above were wrong.
2. Can we reason constructively about correctness?
--------------------------------------------------
The remaining question: If on the shutdown path, we can not actually
remove all the enqueued task works, under what circumstances are we
even able to interrupt and return from the landlock_restrict_self()
system call?
2.1 For n competing restrict_self calls, n-1 of them need to get interrupted
----------------------------------------------------------------------------
To answer this, consider a multithreaded process with threads named
"red", "green" and "blue" and many additional threads: When "red",
"green" and "blue" enforce landlock_restrict_self() concurrently, due
to differing iteration order, we might end up enqueueing the task
works on other threads in all of the following combinations:
t0: R G B <- front of queue
t1: R B G
t2: G R B
t3: G B R
t4: B R G
t5: B G R
In this configuration, for any of the landlock_restrict_self() system
calls to even return (successfully or unsuccessfully), at least two
threads must receive an interrupt and therefore remove their enqueued
task works from the front of the queue. Assuming those are green and
blue, we get:
t0: R <- front of queue
t1: R
t2: G R
t3: G B R
t4: B R
t5: B G R
(This works because after the patch above, all of the enqueued G and B
works finish even if there are remaining G and B works that are still
blocked by an "R" entry.)
Now, "R" is in the front of the queue, and the
landlock_restrict_self() call for the red thread can finish normally,
even without it being interrupted.
Once the "R" task works are done as well, the remaining G and B works
can run and finish as well.
This scheme generalizes: If we have n competing
landlock_restrict_self() calls, then in worst case, at least n-1 of
these system calls need to be interrupted so that they can all
terminate.
2.2 Can we guarantee that two system calls get interrupted?
-----------------------------------------------------------
In case of competing landlock_restrict_self() calls, I think it is
possible that not all relevant system calls get seen. The scenario is
one where we have a "red" and "green" thread calling
landlock_restrict_self().
(a set of additional threads)
t0: task_works: R G
t1: task_works: G R
tR: red thread
tG: green thread
In the red thread, the following happens:
* Under RCU, count the number of total threads => get a low number
* Allocate space for that number of task_works
* Under RCU
* Enqueue "R" into t0 and t1
* Enqueue "R" for some of the "additional threads"
* But we do not have enough pre-allocated space to enqueue "R" for
the green thread tG.
The same thing happens in the green thread as well.
The result is that we still have a deadlock between t0 and t1, but
neither the red nor the green thread get interrupted so that they can
resolve it.
(FWIW, you could resolve it from the outside by sending a signal to
the red or green thread manually, but it is not guaranteed to happen
on its own.)
Caveat: I am making pessimistic assumptions about the iteration order
of the task list here, and I am assuming that the number of
"additional threads" is swinging up and down during the competing
enforcement, so that the enforcing threads are mis-approximating the
required space for memory pre-allocation.
2.3 Possible resolutions
------------------------
* We could try to interrupt all sibling threads during the teardown,
to fix the issue discussed in 2.2. (Downside: Complicated, more
expensive)
* The reason why landlock_restrict_self() can't return is because it
needs to wait until all task works are done before it can free the
memory. Alternatively, we could make the task works take ownership
of these memory structures (refcounting the shared_ctx). (Downside:
The used memory is not linear to the number of threads any more.)
Side remark: In testing, I had the impression that the
landlock_restrict_self() calls can go into a retry loop for a while
where all competing threads get interrupted all the time; in a debug
build, when the Syzkaller test prints out a line for each attempt,
sometimes it was hanging for seconds and *then* resolving itself
again.
3 Conclusion
---------------
I would prefer if the final solution would not require deadlock
reasoning at that level and we could do it in simpler way. I
therefore propose to do what Ding Yihan suggested, and what we had
also discussed previously in the code review:
* Let's serialize the landlock_restrict_self()-with-TSYNC operations
through the cred_guard_mutex.
This will resolve the issue where competing landlock_restrict_self()
calls with TSYNC can deadlock. It will also remove the jittery
behavior for that worst case where the conflict is resolved through
retry.
So in my mind, we need both patches:
* The fix to the cleanup path from 1. above, to make interruption
work more reliably and to correct the misunderstandings in the
comments.
* cred_guard_mutex to serialize the TSYNC invocations.
Please let me know what you think.
Thanks,
–Günther
^ permalink raw reply
* Re: [PATCH v2 12/15] ovl: change ovl_create_real() to get a new lock when re-opening created file.
From: Amir Goldstein @ 2026-02-23 9:39 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
Darrick J. Wong, linux-kernel, netfs, linux-fsdevel, linux-nfs,
linux-unionfs, apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-13-neilb@ownmail.net>
On Mon, Feb 23, 2026 at 2:14 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> When ovl_create_real() is used to create a file on the upper filesystem
> it needs to return the resulting dentry - positive and hashed.
> It is usually the case the that dentry passed to the create function
> (e.g. vfs_create()) will be suitable but this is not guaranteed. The
> filesystem may unhash that dentry forcing a repeat lookup next time the
> name is wanted.
>
> So ovl_create_real() must be (and is) aware of this and prepared to
> perform that lookup to get a hash positive dentry.
>
> This is currently done under that same directory lock that provided
> exclusion for the create. Proposed changes to locking will make this
> not possible - as the name, rather than the directory, will be locked.
> The new APIs provided for lookup and locking do not and cannot support
> this pattern.
>
> The lock isn't needed. ovl_create_real() can drop the lock and then get
> a new lock for the lookup - then check that the lookup returned the
> correct inode. In a well-behaved configuration where the upper
> filesystem is not being modified by a third party, this will always work
> reliably, and if there are separate modification it will fail cleanly.
>
> So change ovl_create_real() to drop the lock and call
> ovl_start_creating_upper() to find the correct dentry. Note that
> start_creating doesn't fail if the name already exists.
>
> The lookup previously used the name from newdentry which was guaranteed
> to be stable because the parent directory was locked. As we now drop
> the lock we lose that guarantee. As newdentry is unhashed it is
> unlikely for the name to change, but safest not to depend on that. So
> the expected name is now passed in to ovl_create_real() and that is
> used.
>
> This removes the only remaining use of ovl_lookup_upper, so it is
> removed.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/dir.c | 36 ++++++++++++++++++++++++------------
> fs/overlayfs/overlayfs.h | 8 +-------
> fs/overlayfs/super.c | 1 +
> 3 files changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index c4feb89ad1e3..6285069ccc59 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -159,7 +159,8 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> }
>
> struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> - struct dentry *newdentry, struct ovl_cattr *attr)
> + struct dentry *newdentry, struct qstr *qname,
> + struct ovl_cattr *attr)
> {
> struct inode *dir = parent->d_inode;
> int err;
> @@ -221,19 +222,29 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> struct dentry *d;
> /*
> * Some filesystems (i.e. casefolded) may return an unhashed
> - * negative dentry from the ovl_lookup_upper() call before
> + * negative dentry from the ovl_start_creating_upper() call before
> * ovl_create_real().
> * In that case, lookup again after making the newdentry
> * positive, so ovl_create_upper() always returns a hashed
> - * positive dentry.
> + * positive dentry. We lookup using qname which should be
> + * the same name as newentry, but is certain not to change.
> + * As we have to drop the lock before the lookup a race
> + * could result in a lookup failure. In that case we return
> + * an error.
> */
> - d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> - newdentry->d_name.len);
> - dput(newdentry);
> - if (IS_ERR_OR_NULL(d))
> + end_creating_keep(newdentry);
> + d = ovl_start_creating_upper(ofs, parent, qname);
> +
> + if (IS_ERR_OR_NULL(d)) {
> err = d ? PTR_ERR(d) : -ENOENT;
> - else
> + } else if (d->d_inode != newdentry->d_inode) {
> + err = -EIO;
> + dput(newdentry);
> + } else {
> + dput(newdentry);
> return d;
> + }
> + return ERR_PTR(err);
> }
> out:
> if (err) {
> @@ -252,7 +263,7 @@ struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
> ret = ovl_start_creating_temp(ofs, workdir, name);
> if (IS_ERR(ret))
> return ret;
> - ret = ovl_create_real(ofs, workdir, ret, attr);
> + ret = ovl_create_real(ofs, workdir, ret, &QSTR(name), attr);
> return end_creating_keep(ret);
> }
>
> @@ -352,14 +363,15 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
> struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
> struct dentry *newdentry;
> + struct qstr qname = QSTR_LEN(dentry->d_name.name,
> + dentry->d_name.len);
> int err;
>
> newdentry = ovl_start_creating_upper(ofs, upperdir,
> - &QSTR_LEN(dentry->d_name.name,
> - dentry->d_name.len));
> + &qname);
> if (IS_ERR(newdentry))
> return PTR_ERR(newdentry);
> - newdentry = ovl_create_real(ofs, upperdir, newdentry, attr);
> + newdentry = ovl_create_real(ofs, upperdir, newdentry, &qname, attr);
> if (IS_ERR(newdentry))
> return PTR_ERR(newdentry);
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index cad2055ebf18..714a1cec3709 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -406,13 +406,6 @@ static inline struct file *ovl_do_tmpfile(struct ovl_fs *ofs,
> return file;
> }
>
> -static inline struct dentry *ovl_lookup_upper(struct ovl_fs *ofs,
> - const char *name,
> - struct dentry *base, int len)
> -{
> - return lookup_one(ovl_upper_mnt_idmap(ofs), &QSTR_LEN(name, len), base);
> -}
> -
> static inline struct dentry *ovl_lookup_upper_unlocked(struct ovl_fs *ofs,
> const char *name,
> struct dentry *base,
> @@ -888,6 +881,7 @@ struct ovl_cattr {
>
> struct dentry *ovl_create_real(struct ovl_fs *ofs,
> struct dentry *parent, struct dentry *newdentry,
> + struct qstr *qname,
> struct ovl_cattr *attr);
> int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
> #define OVL_TEMPNAME_SIZE 20
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index d4c12feec039..109643930b9f 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -634,6 +634,7 @@ static struct dentry *ovl_lookup_or_create(struct ovl_fs *ofs,
> if (!IS_ERR(child)) {
> if (!child->d_inode)
> child = ovl_create_real(ofs, parent, child,
> + &QSTR(name),
> OVL_CATTR(mode));
> end_creating_keep(child);
> }
> --
> 2.50.0.107.gf914562f5916.dirty
>
^ permalink raw reply
* Re: [PATCH v2 11/15] ovl: pass name buffer to ovl_start_creating_temp()
From: Amir Goldstein @ 2026-02-23 9:35 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
Darrick J. Wong, linux-kernel, netfs, linux-fsdevel, linux-nfs,
linux-unionfs, apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-12-neilb@ownmail.net>
On Mon, Feb 23, 2026 at 2:14 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> Now ovl_start_creating_temp() is passed a buffer in which to store the
> temp name. This will be useful in a future patch were ovl_create_real()
> will need access to that name.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/dir.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index ff3dbd1ca61f..c4feb89ad1e3 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -66,10 +66,9 @@ void ovl_tempname(char name[OVL_TEMPNAME_SIZE])
> }
>
> static struct dentry *ovl_start_creating_temp(struct ovl_fs *ofs,
> - struct dentry *workdir)
> + struct dentry *workdir,
> + char name[OVL_TEMPNAME_SIZE])
> {
> - char name[OVL_TEMPNAME_SIZE];
> -
> ovl_tempname(name);
> return start_creating(ovl_upper_mnt_idmap(ofs), workdir,
> &QSTR(name));
> @@ -81,11 +80,12 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> struct dentry *whiteout, *link;
> struct dentry *workdir = ofs->workdir;
> struct inode *wdir = workdir->d_inode;
> + char name[OVL_TEMPNAME_SIZE];
>
> guard(mutex)(&ofs->whiteout_lock);
>
> if (!ofs->whiteout) {
> - whiteout = ovl_start_creating_temp(ofs, workdir);
> + whiteout = ovl_start_creating_temp(ofs, workdir, name);
> if (IS_ERR(whiteout))
> return whiteout;
> err = ovl_do_whiteout(ofs, wdir, whiteout);
> @@ -97,7 +97,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> }
>
> if (!ofs->no_shared_whiteout) {
> - link = ovl_start_creating_temp(ofs, workdir);
> + link = ovl_start_creating_temp(ofs, workdir, name);
> if (IS_ERR(link))
> return link;
> err = ovl_do_link(ofs, ofs->whiteout, wdir, link);
> @@ -247,7 +247,9 @@ struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
> struct ovl_cattr *attr)
> {
> struct dentry *ret;
> - ret = ovl_start_creating_temp(ofs, workdir);
> + char name[OVL_TEMPNAME_SIZE];
> +
> + ret = ovl_start_creating_temp(ofs, workdir, name);
> if (IS_ERR(ret))
> return ret;
> ret = ovl_create_real(ofs, workdir, ret, attr);
> --
> 2.50.0.107.gf914562f5916.dirty
>
^ permalink raw reply
* Re: [PATCH v4 15/17] module: Introduce hash-based integrity checking
From: Thomas Weißschuh @ 2026-02-23 7:53 UTC (permalink / raw)
To: Nicolas Schier
Cc: Nathan Chancellor, Arnd Bergmann, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Paul Moore, James Morris,
Serge E. Hallyn, Jonathan Corbet, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Naveen N Rao, Mimi Zohar,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Daniel Gomez,
Aaron Tomlin, Christophe Leroy (CS GROUP), Nicolas Bouchinet,
Xiu Jianfeng, Fabian Grünbichler, Arnout Engelen,
Mattia Rizzolo, kpcyrd, Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <aZol1Rsa2tX-WNaZ@derry.ads.avm.de>
On 2026-02-21 22:38:29+0100, Nicolas Schier wrote:
> On Tue, Jan 13, 2026 at 01:28:59PM +0100, Thomas Weißschuh wrote:
> > The current signature-based module integrity checking has some drawbacks
> > in combination with reproducible builds. Either the module signing key
> > is generated at build time, which makes the build unreproducible, or a
> > static signing key is used, which precludes rebuilds by third parties
> > and makes the whole build and packaging process much more complicated.
> >
> > The goal is to reach bit-for-bit reproducibility. Excluding certain
> > parts of the build output from the reproducibility analysis would be
> > error-prone and force each downstream consumer to introduce new tooling.
> >
> > Introduce a new mechanism to ensure only well-known modules are loaded
> > by embedding a merkle tree root of all modules built as part of the full
> > kernel build into vmlinux.
> >
> > Non-builtin modules can be validated as before through signatures.
> >
> > Normally the .ko module files depend on a fully built vmlinux to be
> > available for modpost validation and BTF generation. With
> > CONFIG_MODULE_HASHES, vmlinux now depends on the modules
> > to build a merkle tree. This introduces a dependency cycle which is
> > impossible to satisfy. Work around this by building the modules during
> > link-vmlinux.sh, after vmlinux is complete enough for modpost and BTF
> > but before the final module hashes are
> >
> > The PKCS7 format which is used for regular module signatures can not
> > represent Merkle proofs, so a new kind of module signature is
> > introduced. As this signature type is only ever used for builtin
> > modules, no compatibility issues can arise.
> >
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> > .gitignore | 1 +
> > Documentation/kbuild/reproducible-builds.rst | 5 +-
> > Makefile | 8 +-
> > include/asm-generic/vmlinux.lds.h | 11 +
> > include/linux/module_hashes.h | 25 ++
> > include/linux/module_signature.h | 1 +
> > kernel/module/Kconfig | 21 +-
> > kernel/module/Makefile | 1 +
> > kernel/module/hashes.c | 92 ++++++
> > kernel/module/hashes_root.c | 6 +
> > kernel/module/internal.h | 1 +
> > kernel/module/main.c | 4 +-
> > scripts/.gitignore | 1 +
> > scripts/Makefile | 3 +
> > scripts/Makefile.modfinal | 11 +
> > scripts/Makefile.modinst | 13 +
> > scripts/Makefile.vmlinux | 5 +
> > scripts/link-vmlinux.sh | 14 +-
> > scripts/modules-merkle-tree.c | 467 +++++++++++++++++++++++++++
> > security/lockdown/Kconfig | 2 +-
> > 20 files changed, 685 insertions(+), 7 deletions(-)
> >
> [...]
>
> > diff --git a/kernel/module/hashes_root.c b/kernel/module/hashes_root.c
> > new file mode 100644
> > index 000000000000..1abfcd3aa679
> > --- /dev/null
> > +++ b/kernel/module/hashes_root.c
> > @@ -0,0 +1,6 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/module_hashes.h>
> > +
> > +/* Blank dummy data. Will be overridden by link-vmlinux.sh */
> > +const struct module_hashes_root module_hashes_root __module_hashes_section = {};
> > diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> > index e2d49122c2a1..e22837d3ac76 100644
> > --- a/kernel/module/internal.h
> > +++ b/kernel/module/internal.h
> > @@ -338,6 +338,7 @@ void module_mark_ro_after_init(const Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
> > const char *secstrings);
> >
> > int module_sig_check(struct load_info *info, const u8 *sig, size_t sig_len);
> > +int module_hash_check(struct load_info *info, const u8 *sig, size_t sig_len);
> >
> > #ifdef CONFIG_DEBUG_KMEMLEAK
> > void kmemleak_load_module(const struct module *mod, const struct load_info *info);
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index 2a28a0ece809..fa30b6387936 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -3362,8 +3362,10 @@ static int module_integrity_check(struct load_info *info, int flags)
> >
> > if (IS_ENABLED(CONFIG_MODULE_SIG) && sig_type == PKEY_ID_PKCS7) {
> > err = module_sig_check(info, sig, sig_len);
> > + } else if (IS_ENABLED(CONFIG_MODULE_HASHES) && sig_type == PKEY_ID_MERKLE) {
> > + err = module_hash_check(info, sig, sig_len);
> > } else {
> > - pr_err("module: not signed with expected PKCS#7 message\n");
> > + pr_err("module: not signed with signature mechanism\n");
> > err = -ENOPKG;
>
> To prevent others from running into the same issue:
>
> My first test got stuck here, as I tested with virtme-ng, which symlinks
> modules from build tree to /lib/modules/$(uname -r)/..., resulting in
>
> [ 15.956855] module: not signed with signature mechanism
> modprobe: ERROR: could not insert 'efivarfs': Package not installed
>
> As the modules_install step was missing, modules were not being signed.
Currently the signing is deferred to installation time to keep in sync
with regular module signing and to keep the logic simpler by not having
to gracefully handle previously-signed files.
But this could be changed.
> [...]
> > diff --git a/scripts/modules-merkle-tree.c b/scripts/modules-merkle-tree.c
> > new file mode 100644
> > index 000000000000..a6ec0e21213b
> > --- /dev/null
> > +++ b/scripts/modules-merkle-tree.c
> > @@ -0,0 +1,467 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Compute hashes for modules files and build a merkle tree.
> > + *
> > + * Copyright (C) 2025 Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> > + * Copyright (C) 2025 Thomas Weißschuh <linux@weissschuh.net>
> > + *
> > + */
> > +#define _GNU_SOURCE 1
> > +#include <arpa/inet.h>
> > +#include <err.h>
> > +#include <unistd.h>
> > +#include <fcntl.h>
> > +#include <stdarg.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <stdbool.h>
> > +#include <stdlib.h>
> > +
> > +#include <sys/stat.h>
> > +#include <sys/mman.h>
> > +
> > +#include <openssl/evp.h>
> > +#include <openssl/err.h>
> > +
> > +#include "ssl-common.h"
> > +
> > +static int hash_size;
> > +static EVP_MD_CTX *ctx;
> > +
> > +struct module_signature {
> > + uint8_t algo; /* Public-key crypto algorithm [0] */
> > + uint8_t hash; /* Digest algorithm [0] */
> > + uint8_t id_type; /* Key identifier type [PKEY_ID_PKCS7] */
> > + uint8_t signer_len; /* Length of signer's name [0] */
> > + uint8_t key_id_len; /* Length of key identifier [0] */
> > + uint8_t __pad[3];
> > + uint32_t sig_len; /* Length of signature data */
> > +};
> > +
> > +#define PKEY_ID_MERKLE 3
> > +
> > +static const char magic_number[] = "~Module signature appended~\n";
>
> This here will be the forth definition of struct module_signature,
> increasing the risk of unwanted diversion. I second Petr's suggestion
> to reuse a _common_ definition instead.
Ack.
> (Here, even include/linux/module_signature.h could be included itself.)
I'd like to avoid including internal headers from other components.
We could move it to an UAPI header. Various other subsystems use those
for not-really-UAPI but still ABI definitions.
(...)
> > +static inline char *xasprintf(const char *fmt, ...)
> > +{
> > + va_list ap;
> > + char *strp;
> > + int ret;
> > +
> > + va_start(ap, fmt);
> > + ret = vasprintf(&strp, fmt, ap);
> > + va_end(ap);
> > + if (ret == -1)
> > + err(1, "Memory allocation failed");
> > +
> > + return strp;
> > +}
>
> Please consider moving these x* functions into scripts/include/xalloc.h
> for reuse. (I am sure someone else wrote this already, but I can't find
> it...)
Petr suggested it somewhere, it is done for the next revision.
> thanks for all your efforts for reproducibility!
>
> As I have no clue about that: Is the patent for merkle trees [1] a
> problem when integrating that here?
That should have expired a long time ago [2].
And fs-verity is also using merkle trees.
> Can you verify if I get the mechanics roughly correct?
>
> * Modules are merkle tree leaves. Modules are built and logically
> paired by the order from modules.order; a single left-over module is
> paired with itself.
>
> * Hashes of paired modules are hashed again (branch node hash);
> hashes of pairs of branch nodes' hashes are hashed again;
> repeat until we reach the single merkle tree root hash
>
> * The final merkle tree root hash (and the count of tree levels) is
> included in vmlinux
The merkle tree code was written by Sebastian so he will have the best
knowledge about it. But this is also my understanding.
> 'make && find . -name '*.ko' -exec rm {} \; && make' does not rebuild
> the in-tree modules. Shifting the module-hashes support from
> scripts/link-vmlinux.sh to scripts/Makefile.vmlinux might (make it
> easier) to fix this again.
I'll take a look at it.
> [1]: https://worldwide.espacenet.com/patent/search/family/022107098/publication/US4309569A?q=pn%3DUS4309569
[2] https://patents.stackexchange.com/questions/17901/validity-of-patent-on-merkle-trees
Thomas
^ permalink raw reply
* [PATCH v2 15/15] VFS: unexport lock_rename(), lock_rename_child(), unlock_rename()
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
These three function are now only used in namei.c, so they don't need to
be exported.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
Documentation/filesystems/porting.rst | 7 +++++++
fs/namei.c | 9 +++------
include/linux/namei.h | 3 ---
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
index 1dd31ab417a2..d02aa57e4477 100644
--- a/Documentation/filesystems/porting.rst
+++ b/Documentation/filesystems/porting.rst
@@ -1368,3 +1368,10 @@ lifetime, consider using inode_set_cached_link() instead.
lookup_one_qstr_excl() is no longer exported - use start_creating() or
similar.
+---
+
+** mandatory**
+
+lock_rename(), lock_rename_child(), unlock_rename() are no
+longer available. Use start_renaming() or similar.
+
diff --git a/fs/namei.c b/fs/namei.c
index e6a3717d7065..3547a2a1bfd1 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3775,7 +3775,7 @@ static struct dentry *lock_two_directories(struct dentry *p1, struct dentry *p2)
/*
* p1 and p2 should be directories on the same fs.
*/
-struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
+static struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
{
if (p1 == p2) {
inode_lock_nested(p1->d_inode, I_MUTEX_PARENT);
@@ -3785,12 +3785,11 @@ struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
mutex_lock(&p1->d_sb->s_vfs_rename_mutex);
return lock_two_directories(p1, p2);
}
-EXPORT_SYMBOL(lock_rename);
/*
* c1 and p2 should be on the same fs.
*/
-struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
+static struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
{
if (READ_ONCE(c1->d_parent) == p2) {
/*
@@ -3827,9 +3826,8 @@ struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
mutex_unlock(&c1->d_sb->s_vfs_rename_mutex);
return NULL;
}
-EXPORT_SYMBOL(lock_rename_child);
-void unlock_rename(struct dentry *p1, struct dentry *p2)
+static void unlock_rename(struct dentry *p1, struct dentry *p2)
{
inode_unlock(p1->d_inode);
if (p1 != p2) {
@@ -3837,7 +3835,6 @@ void unlock_rename(struct dentry *p1, struct dentry *p2)
mutex_unlock(&p1->d_sb->s_vfs_rename_mutex);
}
}
-EXPORT_SYMBOL(unlock_rename);
/**
* __start_renaming - lookup and lock names for rename
diff --git a/include/linux/namei.h b/include/linux/namei.h
index c7a7288cdd25..2ad6dd9987b9 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -165,9 +165,6 @@ extern int follow_down_one(struct path *);
extern int follow_down(struct path *path, unsigned int flags);
extern int follow_up(struct path *);
-extern struct dentry *lock_rename(struct dentry *, struct dentry *);
-extern struct dentry *lock_rename_child(struct dentry *, struct dentry *);
-extern void unlock_rename(struct dentry *, struct dentry *);
int start_renaming(struct renamedata *rd, int lookup_flags,
struct qstr *old_last, struct qstr *new_last);
int start_renaming_dentry(struct renamedata *rd, int lookup_flags,
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 14/15] ovl: remove ovl_lock_rename_workdir()
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
This function is unused since
Commit 833d2b3a072f ("Add start_renaming_two_dentries()")
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/overlayfs/overlayfs.h | 2 --
fs/overlayfs/util.c | 25 -------------------------
2 files changed, 27 deletions(-)
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 714a1cec3709..6fb99c583c31 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -569,8 +569,6 @@ bool ovl_is_inuse(struct dentry *dentry);
bool ovl_need_index(struct dentry *dentry);
int ovl_nlink_start(struct dentry *dentry);
void ovl_nlink_end(struct dentry *dentry);
-int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
- struct dentry *upperdir, struct dentry *upper);
int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
struct ovl_metacopy *data);
int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 3f1b763a8bb4..aa2a32793c2f 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1213,31 +1213,6 @@ void ovl_nlink_end(struct dentry *dentry)
ovl_inode_unlock(inode);
}
-int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
- struct dentry *upperdir, struct dentry *upper)
-{
- struct dentry *trap;
-
- /* Workdir should not be subdir of upperdir and vice versa */
- trap = lock_rename(workdir, upperdir);
- if (IS_ERR(trap))
- goto err;
- if (trap)
- goto err_unlock;
- if (work && (work->d_parent != workdir || d_unhashed(work)))
- goto err_unlock;
- if (upper && (upper->d_parent != upperdir || d_unhashed(upper)))
- goto err_unlock;
-
- return 0;
-
-err_unlock:
- unlock_rename(workdir, upperdir);
-err:
- pr_err("failed to lock workdir+upperdir\n");
- return -EIO;
-}
-
/*
* err < 0, 0 if no metacopy xattr, metacopy data size if xattr found.
* an empty xattr returns OVL_METACOPY_MIN_SIZE to distinguish from no xattr value.
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 13/15] ovl: use is_subdir() for testing if one thing is a subdir of another
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
Rather than using lock_rename(), use the more obvious is_subdir() for
ensuring that neither upper nor workdir contain the other.
Also be explicit in the comment that the two directories cannot be the
same.
As this is a point-it-time sanity check and does not provide any
on-going guarantees, the removal of locking does not introduce any
interesting races.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/overlayfs/super.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 109643930b9f..58adefb1c5b8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -451,18 +451,13 @@ static int ovl_lower_dir(const char *name, const struct path *path,
return 0;
}
-/* Workdir should not be subdir of upperdir and vice versa */
+/*
+ * Workdir should not be subdir of upperdir and vice versa, and
+ * they should not be the same.
+ */
static bool ovl_workdir_ok(struct dentry *workdir, struct dentry *upperdir)
{
- bool ok = false;
-
- if (workdir != upperdir) {
- struct dentry *trap = lock_rename(workdir, upperdir);
- if (!IS_ERR(trap))
- unlock_rename(workdir, upperdir);
- ok = (trap == NULL);
- }
- return ok;
+ return !is_subdir(workdir, upperdir) && !is_subdir(upperdir, workdir);
}
static int ovl_setup_trap(struct super_block *sb, struct dentry *dir,
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 12/15] ovl: change ovl_create_real() to get a new lock when re-opening created file.
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
When ovl_create_real() is used to create a file on the upper filesystem
it needs to return the resulting dentry - positive and hashed.
It is usually the case the that dentry passed to the create function
(e.g. vfs_create()) will be suitable but this is not guaranteed. The
filesystem may unhash that dentry forcing a repeat lookup next time the
name is wanted.
So ovl_create_real() must be (and is) aware of this and prepared to
perform that lookup to get a hash positive dentry.
This is currently done under that same directory lock that provided
exclusion for the create. Proposed changes to locking will make this
not possible - as the name, rather than the directory, will be locked.
The new APIs provided for lookup and locking do not and cannot support
this pattern.
The lock isn't needed. ovl_create_real() can drop the lock and then get
a new lock for the lookup - then check that the lookup returned the
correct inode. In a well-behaved configuration where the upper
filesystem is not being modified by a third party, this will always work
reliably, and if there are separate modification it will fail cleanly.
So change ovl_create_real() to drop the lock and call
ovl_start_creating_upper() to find the correct dentry. Note that
start_creating doesn't fail if the name already exists.
The lookup previously used the name from newdentry which was guaranteed
to be stable because the parent directory was locked. As we now drop
the lock we lose that guarantee. As newdentry is unhashed it is
unlikely for the name to change, but safest not to depend on that. So
the expected name is now passed in to ovl_create_real() and that is
used.
This removes the only remaining use of ovl_lookup_upper, so it is
removed.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/overlayfs/dir.c | 36 ++++++++++++++++++++++++------------
fs/overlayfs/overlayfs.h | 8 +-------
fs/overlayfs/super.c | 1 +
3 files changed, 26 insertions(+), 19 deletions(-)
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index c4feb89ad1e3..6285069ccc59 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -159,7 +159,8 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
}
struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
- struct dentry *newdentry, struct ovl_cattr *attr)
+ struct dentry *newdentry, struct qstr *qname,
+ struct ovl_cattr *attr)
{
struct inode *dir = parent->d_inode;
int err;
@@ -221,19 +222,29 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
struct dentry *d;
/*
* Some filesystems (i.e. casefolded) may return an unhashed
- * negative dentry from the ovl_lookup_upper() call before
+ * negative dentry from the ovl_start_creating_upper() call before
* ovl_create_real().
* In that case, lookup again after making the newdentry
* positive, so ovl_create_upper() always returns a hashed
- * positive dentry.
+ * positive dentry. We lookup using qname which should be
+ * the same name as newentry, but is certain not to change.
+ * As we have to drop the lock before the lookup a race
+ * could result in a lookup failure. In that case we return
+ * an error.
*/
- d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
- newdentry->d_name.len);
- dput(newdentry);
- if (IS_ERR_OR_NULL(d))
+ end_creating_keep(newdentry);
+ d = ovl_start_creating_upper(ofs, parent, qname);
+
+ if (IS_ERR_OR_NULL(d)) {
err = d ? PTR_ERR(d) : -ENOENT;
- else
+ } else if (d->d_inode != newdentry->d_inode) {
+ err = -EIO;
+ dput(newdentry);
+ } else {
+ dput(newdentry);
return d;
+ }
+ return ERR_PTR(err);
}
out:
if (err) {
@@ -252,7 +263,7 @@ struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
ret = ovl_start_creating_temp(ofs, workdir, name);
if (IS_ERR(ret))
return ret;
- ret = ovl_create_real(ofs, workdir, ret, attr);
+ ret = ovl_create_real(ofs, workdir, ret, &QSTR(name), attr);
return end_creating_keep(ret);
}
@@ -352,14 +363,15 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
struct dentry *newdentry;
+ struct qstr qname = QSTR_LEN(dentry->d_name.name,
+ dentry->d_name.len);
int err;
newdentry = ovl_start_creating_upper(ofs, upperdir,
- &QSTR_LEN(dentry->d_name.name,
- dentry->d_name.len));
+ &qname);
if (IS_ERR(newdentry))
return PTR_ERR(newdentry);
- newdentry = ovl_create_real(ofs, upperdir, newdentry, attr);
+ newdentry = ovl_create_real(ofs, upperdir, newdentry, &qname, attr);
if (IS_ERR(newdentry))
return PTR_ERR(newdentry);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index cad2055ebf18..714a1cec3709 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -406,13 +406,6 @@ static inline struct file *ovl_do_tmpfile(struct ovl_fs *ofs,
return file;
}
-static inline struct dentry *ovl_lookup_upper(struct ovl_fs *ofs,
- const char *name,
- struct dentry *base, int len)
-{
- return lookup_one(ovl_upper_mnt_idmap(ofs), &QSTR_LEN(name, len), base);
-}
-
static inline struct dentry *ovl_lookup_upper_unlocked(struct ovl_fs *ofs,
const char *name,
struct dentry *base,
@@ -888,6 +881,7 @@ struct ovl_cattr {
struct dentry *ovl_create_real(struct ovl_fs *ofs,
struct dentry *parent, struct dentry *newdentry,
+ struct qstr *qname,
struct ovl_cattr *attr);
int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
#define OVL_TEMPNAME_SIZE 20
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index d4c12feec039..109643930b9f 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -634,6 +634,7 @@ static struct dentry *ovl_lookup_or_create(struct ovl_fs *ofs,
if (!IS_ERR(child)) {
if (!child->d_inode)
child = ovl_create_real(ofs, parent, child,
+ &QSTR(name),
OVL_CATTR(mode));
end_creating_keep(child);
}
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 11/15] ovl: pass name buffer to ovl_start_creating_temp()
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
Now ovl_start_creating_temp() is passed a buffer in which to store the
temp name. This will be useful in a future patch were ovl_create_real()
will need access to that name.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/overlayfs/dir.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index ff3dbd1ca61f..c4feb89ad1e3 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -66,10 +66,9 @@ void ovl_tempname(char name[OVL_TEMPNAME_SIZE])
}
static struct dentry *ovl_start_creating_temp(struct ovl_fs *ofs,
- struct dentry *workdir)
+ struct dentry *workdir,
+ char name[OVL_TEMPNAME_SIZE])
{
- char name[OVL_TEMPNAME_SIZE];
-
ovl_tempname(name);
return start_creating(ovl_upper_mnt_idmap(ofs), workdir,
&QSTR(name));
@@ -81,11 +80,12 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
struct dentry *whiteout, *link;
struct dentry *workdir = ofs->workdir;
struct inode *wdir = workdir->d_inode;
+ char name[OVL_TEMPNAME_SIZE];
guard(mutex)(&ofs->whiteout_lock);
if (!ofs->whiteout) {
- whiteout = ovl_start_creating_temp(ofs, workdir);
+ whiteout = ovl_start_creating_temp(ofs, workdir, name);
if (IS_ERR(whiteout))
return whiteout;
err = ovl_do_whiteout(ofs, wdir, whiteout);
@@ -97,7 +97,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
}
if (!ofs->no_shared_whiteout) {
- link = ovl_start_creating_temp(ofs, workdir);
+ link = ovl_start_creating_temp(ofs, workdir, name);
if (IS_ERR(link))
return link;
err = ovl_do_link(ofs, ofs->whiteout, wdir, link);
@@ -247,7 +247,9 @@ struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
struct ovl_cattr *attr)
{
struct dentry *ret;
- ret = ovl_start_creating_temp(ofs, workdir);
+ char name[OVL_TEMPNAME_SIZE];
+
+ ret = ovl_start_creating_temp(ofs, workdir, name);
if (IS_ERR(ret))
return ret;
ret = ovl_create_real(ofs, workdir, ret, attr);
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 10/15] cachefiles: change cachefiles_bury_object to use start_renaming_dentry()
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
Rather then using lock_rename() and lookup_one() etc we can use
the new start_renaming_dentry(). This is part of centralising dir
locking and lookup so that locking rules can be changed.
Some error check are removed as not necessary. Checks for rep being a
non-dir or IS_DEADDIR and the check that ->graveyard is still a
directory only provide slightly more informative errors and have been
dropped.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/cachefiles/namei.c | 76 ++++++++-----------------------------------
1 file changed, 14 insertions(+), 62 deletions(-)
diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index e5ec90dccc27..3af42ec78411 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -270,7 +270,8 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
struct dentry *rep,
enum fscache_why_object_killed why)
{
- struct dentry *grave, *trap;
+ struct dentry *grave;
+ struct renamedata rd = {};
struct path path, path_to_graveyard;
char nbuffer[8 + 8 + 1];
int ret;
@@ -302,77 +303,36 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
(uint32_t) ktime_get_real_seconds(),
(uint32_t) atomic_inc_return(&cache->gravecounter));
- /* do the multiway lock magic */
- trap = lock_rename(cache->graveyard, dir);
- if (IS_ERR(trap))
- return PTR_ERR(trap);
-
- /* do some checks before getting the grave dentry */
- if (rep->d_parent != dir || IS_DEADDIR(d_inode(rep))) {
- /* the entry was probably culled when we dropped the parent dir
- * lock */
- unlock_rename(cache->graveyard, dir);
- _leave(" = 0 [culled?]");
- return 0;
- }
-
- if (!d_can_lookup(cache->graveyard)) {
- unlock_rename(cache->graveyard, dir);
- cachefiles_io_error(cache, "Graveyard no longer a directory");
- return -EIO;
- }
-
- if (trap == rep) {
- unlock_rename(cache->graveyard, dir);
- cachefiles_io_error(cache, "May not make directory loop");
+ rd.mnt_idmap = &nop_mnt_idmap;
+ rd.old_parent = dir;
+ rd.new_parent = cache->graveyard;
+ rd.flags = 0;
+ ret = start_renaming_dentry(&rd, 0, rep, &QSTR(nbuffer));
+ if (ret) {
+ cachefiles_io_error(cache, "Cannot lock/lookup in graveyard");
return -EIO;
}
if (d_mountpoint(rep)) {
- unlock_rename(cache->graveyard, dir);
+ end_renaming(&rd);
cachefiles_io_error(cache, "Mountpoint in cache");
return -EIO;
}
- grave = lookup_one(&nop_mnt_idmap, &QSTR(nbuffer), cache->graveyard);
- if (IS_ERR(grave)) {
- unlock_rename(cache->graveyard, dir);
- trace_cachefiles_vfs_error(object, d_inode(cache->graveyard),
- PTR_ERR(grave),
- cachefiles_trace_lookup_error);
-
- if (PTR_ERR(grave) == -ENOMEM) {
- _leave(" = -ENOMEM");
- return -ENOMEM;
- }
-
- cachefiles_io_error(cache, "Lookup error %ld", PTR_ERR(grave));
- return -EIO;
- }
-
+ grave = rd.new_dentry;
if (d_is_positive(grave)) {
- unlock_rename(cache->graveyard, dir);
- dput(grave);
+ end_renaming(&rd);
grave = NULL;
cond_resched();
goto try_again;
}
if (d_mountpoint(grave)) {
- unlock_rename(cache->graveyard, dir);
- dput(grave);
+ end_renaming(&rd);
cachefiles_io_error(cache, "Mountpoint in graveyard");
return -EIO;
}
- /* target should not be an ancestor of source */
- if (trap == grave) {
- unlock_rename(cache->graveyard, dir);
- dput(grave);
- cachefiles_io_error(cache, "May not make directory loop");
- return -EIO;
- }
-
/* attempt the rename */
path.mnt = cache->mnt;
path.dentry = dir;
@@ -382,13 +342,6 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
if (ret < 0) {
cachefiles_io_error(cache, "Rename security error %d", ret);
} else {
- struct renamedata rd = {
- .mnt_idmap = &nop_mnt_idmap,
- .old_parent = dir,
- .old_dentry = rep,
- .new_parent = cache->graveyard,
- .new_dentry = grave,
- };
trace_cachefiles_rename(object, d_inode(rep)->i_ino, why);
ret = cachefiles_inject_read_error();
if (ret == 0)
@@ -402,8 +355,7 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
}
__cachefiles_unmark_inode_in_use(object, d_inode(rep));
- unlock_rename(cache->graveyard, dir);
- dput(grave);
+ end_renaming(&rd);
_leave(" = 0");
return 0;
}
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 09/15] ovl: Simplify ovl_lookup_real_one()
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
The primary purpose of this patch is to remove the locking from
ovl_lookup_real_one() as part of centralising all locking of directories
for name operations.
The locking here isn't needed. By performing consistency tests after
the lookup we can be sure that the result of the lookup was valid at
least for a moment, which is all the original code promised.
lookup_noperm_unlocked() is used for the lookup and it will take the
lock if needed only where it is needed.
Also:
- don't take a reference to real->d_parent. The parent is
only use for a pointer comparison, and no reference is needed for
that.
- Several "if" statements have a "goto" followed by "else" - the
else isn't needed: the following statement can directly follow
the "if" as a new statement
- Use a consistent pattern of setting "err" before performing a test
and possibly going to "fail".
- remove the "out" label (now that we don't need to dput(parent) or
unlock) and simply return from fail:.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/overlayfs/export.c | 71 ++++++++++++++++++++-----------------------
1 file changed, 33 insertions(+), 38 deletions(-)
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 83f80fdb1567..b448fc9424b6 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -349,69 +349,64 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
return NULL;
}
-/*
- * Lookup a child overlay dentry to get a connected overlay dentry whose real
- * dentry is @real. If @real is on upper layer, we lookup a child overlay
- * dentry with the same name as the real dentry. Otherwise, we need to consult
- * index for lookup.
+/**
+ * ovl_lookup_real_one - Lookup a child overlay dentry to get an overlay dentry whose real dentry is given
+ * @connected: parent overlay dentry
+ * @real: given child real dentry
+ * @layer: layer in which @real exists
+ *
+ *
+ * Lookup a child overlay dentry in @connected with the same name as the @real
+ * dentry. Then check that the parent of the result is the real dentry for
+ * @connected, and @real is the real dentry for the result.
+ *
+ * Returns:
+ * %-ECHILD if the parent of @real is no longer the real dentry for @connected.
+ * %-ESTALE if @real is no the real dentry of the found dentry.
+ * Otherwise the found dentry is returned.
*/
static struct dentry *ovl_lookup_real_one(struct dentry *connected,
struct dentry *real,
const struct ovl_layer *layer)
{
- struct inode *dir = d_inode(connected);
- struct dentry *this, *parent = NULL;
+ struct dentry *this;
struct name_snapshot name;
int err;
/*
- * Lookup child overlay dentry by real name. The dir mutex protects us
- * from racing with overlay rename. If the overlay dentry that is above
- * real has already been moved to a parent that is not under the
- * connected overlay dir, we return -ECHILD and restart the lookup of
- * connected real path from the top.
- */
- inode_lock_nested(dir, I_MUTEX_PARENT);
- err = -ECHILD;
- parent = dget_parent(real);
- if (ovl_dentry_real_at(connected, layer->idx) != parent)
- goto fail;
-
- /*
- * We also need to take a snapshot of real dentry name to protect us
+ * We need to take a snapshot of real dentry name to protect us
* from racing with underlying layer rename. In this case, we don't
* care about returning ESTALE, only from dereferencing a free name
* pointer because we hold no lock on the real dentry.
*/
take_dentry_name_snapshot(&name, real);
- /*
- * No idmap handling here: it's an internal lookup.
- */
- this = lookup_noperm(&name.name, connected);
+ this = lookup_noperm_unlocked(&name.name, connected);
release_dentry_name_snapshot(&name);
+
+ err = -ECHILD;
+ if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
+ goto fail;
+
err = PTR_ERR(this);
- if (IS_ERR(this)) {
+ if (IS_ERR(this))
goto fail;
- } else if (!this || !this->d_inode) {
- dput(this);
- err = -ENOENT;
+
+ err = -ENOENT;
+ if (!this || !this->d_inode)
goto fail;
- } else if (ovl_dentry_real_at(this, layer->idx) != real) {
- dput(this);
- err = -ESTALE;
+
+ err = -ESTALE;
+ if (ovl_dentry_real_at(this, layer->idx) != real)
goto fail;
- }
-out:
- dput(parent);
- inode_unlock(dir);
return this;
fail:
pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
real, layer->idx, connected, err);
- this = ERR_PTR(err);
- goto out;
+ if (!IS_ERR(this))
+ dput(this);
+ return ERR_PTR(err);
}
static struct dentry *ovl_lookup_real(struct super_block *sb,
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 08/15] VFS: make lookup_one_qstr_excl() static.
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
lookup_one_qstr_excl() is no longer used outside of namei.c, so
make it static.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
Documentation/filesystems/porting.rst | 7 +++++++
fs/namei.c | 5 ++---
include/linux/namei.h | 3 ---
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
index 52ff1d19405b..1dd31ab417a2 100644
--- a/Documentation/filesystems/porting.rst
+++ b/Documentation/filesystems/porting.rst
@@ -1361,3 +1361,10 @@ to match what strlen() would return if it was ran on the string.
However, if the string is freely accessible for the duration of inode's
lifetime, consider using inode_set_cached_link() instead.
+
+---
+
+**mandatory**
+
+lookup_one_qstr_excl() is no longer exported - use start_creating() or
+similar.
diff --git a/fs/namei.c b/fs/namei.c
index d80b81a1f06a..e6a3717d7065 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1782,8 +1782,8 @@ static struct dentry *lookup_dcache(const struct qstr *name,
* Will return -ENOENT if name isn't found and LOOKUP_CREATE wasn't passed.
* Will return -EEXIST if name is found and LOOKUP_EXCL was passed.
*/
-struct dentry *lookup_one_qstr_excl(const struct qstr *name,
- struct dentry *base, unsigned int flags)
+static struct dentry *lookup_one_qstr_excl(const struct qstr *name,
+ struct dentry *base, unsigned int flags)
{
struct dentry *dentry;
struct dentry *old;
@@ -1820,7 +1820,6 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
}
return dentry;
}
-EXPORT_SYMBOL(lookup_one_qstr_excl);
/**
* lookup_fast - do fast lockless (but racy) lookup of a dentry
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 58600cf234bc..c7a7288cdd25 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -54,9 +54,6 @@ extern int path_pts(struct path *path);
extern int user_path_at(int, const char __user *, unsigned, struct path *);
-struct dentry *lookup_one_qstr_excl(const struct qstr *name,
- struct dentry *base,
- unsigned int flags);
extern int kern_path(const char *, unsigned, struct path *);
struct dentry *kern_path_parent(const char *name, struct path *parent);
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 07/15] nfsd: switch purge_old() to use start_removing_noperm()
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
Rather than explicit locking, use the start_removing_noperm() and
end_removing() wrappers.
This was not done with other start_removing changes due to conflicting
in-flight patches.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/nfsd/nfs4recover.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index b4bf85f96f6e..b338473d6e52 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -352,16 +352,14 @@ purge_old(struct dentry *parent, char *cname, struct nfsd_net *nn)
if (nfs4_has_reclaimed_state(name, nn))
goto out_free;
- inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
- child = lookup_one(&nop_mnt_idmap, &QSTR(cname), parent);
+ child = start_removing_noperm(parent, &QSTR(cname));
if (!IS_ERR(child)) {
status = vfs_rmdir(&nop_mnt_idmap, d_inode(parent), child, NULL);
if (status)
printk("failed to remove client recovery directory %pd\n",
child);
- dput(child);
}
- inode_unlock(d_inode(parent));
+ end_removing(child);
out_free:
kfree(name.data);
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 06/15] selinux: Use simple_start_creating() / simple_done_creating()
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
Instead of explicitly locking the parent and performing a lookup in
selinux, use simple_start_creating(), and then use
simple_done_creating() to unlock.
This extends the region that the directory is locked for, and also
performs a lookup.
The lock extension is of no real consequence.
The lookup uses simple_lookup() and so always succeeds. Thus when
d_make_persistent() is called the dentry will already be hashed.
d_make_persistent() handles this case.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Acked-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
security/selinux/selinuxfs.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 3245cc531555..7d4f90e5b12a 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1931,15 +1931,16 @@ static const struct inode_operations swapover_dir_inode_operations = {
static struct dentry *sel_make_swapover_dir(struct super_block *sb,
unsigned long *ino)
{
- struct dentry *dentry = d_alloc_name(sb->s_root, ".swapover");
+ struct dentry *dentry;
struct inode *inode;
- if (!dentry)
+ inode = sel_make_inode(sb, S_IFDIR);
+ if (!inode)
return ERR_PTR(-ENOMEM);
- inode = sel_make_inode(sb, S_IFDIR);
- if (!inode) {
- dput(dentry);
+ dentry = simple_start_creating(sb->s_root, ".swapover");
+ if (!dentry) {
+ iput(inode);
return ERR_PTR(-ENOMEM);
}
@@ -1947,11 +1948,9 @@ static struct dentry *sel_make_swapover_dir(struct super_block *sb,
inode->i_ino = ++(*ino);
/* directory inodes start off with i_nlink == 2 (for "." entry) */
inc_nlink(inode);
- inode_lock(sb->s_root->d_inode);
d_make_persistent(dentry, inode);
inc_nlink(sb->s_root->d_inode);
- inode_unlock(sb->s_root->d_inode);
- dput(dentry);
+ simple_done_creating(dentry);
return dentry; // borrowed
}
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 05/15] Apparmor: Use simple_start_creating() / simple_done_creating()
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
Instead of explicitly locking the parent and performing a look up in
apparmor, use simple_start_creating(), and then simple_done_creating()
to unlock and drop the dentry.
This removes the need to check for an existing entry (as
simple_start_creating() acts like an exclusive create and can return
-EEXIST), simplifies error paths, and keeps dir locking code
centralised.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
security/apparmor/apparmorfs.c | 35 ++++++++--------------------------
1 file changed, 8 insertions(+), 27 deletions(-)
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 2f84bd23edb6..f93c4f31d02a 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -282,32 +282,20 @@ static struct dentry *aafs_create(const char *name, umode_t mode,
dir = d_inode(parent);
- inode_lock(dir);
- dentry = lookup_noperm(&QSTR(name), parent);
+ dentry = simple_start_creating(parent, name);
if (IS_ERR(dentry)) {
error = PTR_ERR(dentry);
- goto fail_lock;
- }
-
- if (d_really_is_positive(dentry)) {
- error = -EEXIST;
- goto fail_dentry;
+ goto fail;
}
error = __aafs_setup_d_inode(dir, dentry, mode, data, link, fops, iops);
+ simple_done_creating(dentry);
if (error)
- goto fail_dentry;
- inode_unlock(dir);
-
+ goto fail;
return dentry;
-fail_dentry:
- dput(dentry);
-
-fail_lock:
- inode_unlock(dir);
+fail:
simple_release_fs(&aafs_mnt, &aafs_count);
-
return ERR_PTR(error);
}
@@ -2585,8 +2573,7 @@ static int aa_mk_null_file(struct dentry *parent)
if (error)
return error;
- inode_lock(d_inode(parent));
- dentry = lookup_noperm(&QSTR(NULL_FILE_NAME), parent);
+ dentry = simple_start_creating(parent, NULL_FILE_NAME);
if (IS_ERR(dentry)) {
error = PTR_ERR(dentry);
goto out;
@@ -2594,7 +2581,7 @@ static int aa_mk_null_file(struct dentry *parent)
inode = new_inode(parent->d_inode->i_sb);
if (!inode) {
error = -ENOMEM;
- goto out1;
+ goto out;
}
inode->i_ino = get_next_ino();
@@ -2606,18 +2593,12 @@ static int aa_mk_null_file(struct dentry *parent)
aa_null.dentry = dget(dentry);
aa_null.mnt = mntget(mount);
- error = 0;
-
-out1:
- dput(dentry);
out:
- inode_unlock(d_inode(parent));
+ simple_done_creating(dentry);
simple_release_fs(&mount, &count);
return error;
}
-
-
static const char *policy_get_link(struct dentry *dentry,
struct inode *inode,
struct delayed_call *done)
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 04/15] libfs: change simple_done_creating() to use end_creating()
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
simple_done_creating() and end_creating() are identical.
So change the former to use the latter. This further centralises
unlocking of directories.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/libfs.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index 74134ba2e8d1..63b4fb082435 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -2318,7 +2318,6 @@ EXPORT_SYMBOL(simple_start_creating);
/* parent must have been held exclusive since simple_start_creating() */
void simple_done_creating(struct dentry *child)
{
- inode_unlock(child->d_parent->d_inode);
- dput(child);
+ end_creating(child);
}
EXPORT_SYMBOL(simple_done_creating);
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 03/15] VFS: move the start_dirop() kerndoc comment to before start_dirop()
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
This kerneldoc comment was always meant for start_dirop(), not for
__start_dirop() which is a static function and doesn't need
documentation.
It was in the wrong place and was then incorrectly renamed (instead of
moved) and useless "documentation" was added for "@state" was provided.
This patch reverts the name, removes the mention of @state, and moves
the comment to where it belongs.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/namei.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index e4ac07a4090e..d80b81a1f06a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2893,20 +2893,6 @@ static int filename_parentat(int dfd, struct filename *name,
return __filename_parentat(dfd, name, flags, parent, last, type, NULL);
}
-/**
- * __start_dirop - begin a create or remove dirop, performing locking and lookup
- * @parent: the dentry of the parent in which the operation will occur
- * @name: a qstr holding the name within that parent
- * @lookup_flags: intent and other lookup flags.
- * @state: task state bitmask
- *
- * The lookup is performed and necessary locks are taken so that, on success,
- * the returned dentry can be operated on safely.
- * The qstr must already have the hash value calculated.
- *
- * Returns: a locked dentry, or an error.
- *
- */
static struct dentry *__start_dirop(struct dentry *parent, struct qstr *name,
unsigned int lookup_flags,
unsigned int state)
@@ -2928,6 +2914,19 @@ static struct dentry *__start_dirop(struct dentry *parent, struct qstr *name,
return dentry;
}
+/**
+ * start_dirop - begin a create or remove dirop, performing locking and lookup
+ * @parent: the dentry of the parent in which the operation will occur
+ * @name: a qstr holding the name within that parent
+ * @lookup_flags: intent and other lookup flags.
+ *
+ * The lookup is performed and necessary locks are taken so that, on success,
+ * the returned dentry can be operated on safely.
+ * The qstr must already have the hash value calculated.
+ *
+ * Returns: a locked dentry, or an error.
+ *
+ */
struct dentry *start_dirop(struct dentry *parent, struct qstr *name,
unsigned int lookup_flags)
{
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 02/15] fs/proc: Don't lock root inode when creating "self" and "thread-self"
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
proc_setup_self() and proc_setup_thread_self() are only called from
proc_fill_super() which is before the filesystem is "live". So there is
no need to lock the root directory when adding "self" and "thread-self".
This is clear from simple_fill_super() which provides similar
functionality for other filesystems and does not lock anything.
The locking is not harmful, except that it may be confusing to a reader.
As part of an effort to centralise all locking for directories for
name-based operations (prior to changing some locking rules), it is
simplest to remove the locking here.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/proc/self.c | 3 ---
fs/proc/thread_self.c | 3 ---
2 files changed, 6 deletions(-)
diff --git a/fs/proc/self.c b/fs/proc/self.c
index 62d2c0cfe35c..56adf1c68f7a 100644
--- a/fs/proc/self.c
+++ b/fs/proc/self.c
@@ -35,11 +35,9 @@ unsigned self_inum __ro_after_init;
int proc_setup_self(struct super_block *s)
{
- struct inode *root_inode = d_inode(s->s_root);
struct dentry *self;
int ret = -ENOMEM;
- inode_lock(root_inode);
self = d_alloc_name(s->s_root, "self");
if (self) {
struct inode *inode = new_inode(s);
@@ -55,7 +53,6 @@ int proc_setup_self(struct super_block *s)
}
dput(self);
}
- inode_unlock(root_inode);
if (ret)
pr_err("proc_fill_super: can't allocate /proc/self\n");
diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
index d6113dbe58e0..61ac62c3fd9f 100644
--- a/fs/proc/thread_self.c
+++ b/fs/proc/thread_self.c
@@ -35,11 +35,9 @@ unsigned thread_self_inum __ro_after_init;
int proc_setup_thread_self(struct super_block *s)
{
- struct inode *root_inode = d_inode(s->s_root);
struct dentry *thread_self;
int ret = -ENOMEM;
- inode_lock(root_inode);
thread_self = d_alloc_name(s->s_root, "thread-self");
if (thread_self) {
struct inode *inode = new_inode(s);
@@ -55,7 +53,6 @@ int proc_setup_thread_self(struct super_block *s)
}
dput(thread_self);
}
- inode_unlock(root_inode);
if (ret)
pr_err("proc_fill_super: can't allocate /proc/thread-self\n");
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2 01/15] VFS: note error returns is documentation for various lookup functions
From: NeilBrown @ 2026-02-23 1:06 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
Darrick recently noted that try_lookup_noperm() is documented as
"Look up a dentry by name in the dcache, returning NULL if it does not
currently exist." but it can in fact return an error.
So update the documentation for that and related function.
Link: https://lore.kernel.org/all/20260218234917.GA6490@frogsfrogsfrogs/
Cc: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/namei.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/fs/namei.c b/fs/namei.c
index 58f715f7657e..e4ac07a4090e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3124,7 +3124,8 @@ static int lookup_one_common(struct mnt_idmap *idmap,
* @base: base directory to lookup from
*
* Look up a dentry by name in the dcache, returning NULL if it does not
- * currently exist. The function does not try to create a dentry and if one
+ * currently exist or an error is there is a problem with the name.
+ * The function does not try to create a dentry and if one
* is found it doesn't try to revalidate it.
*
* Note that this routine is purely a helper for filesystem usage and should
@@ -3132,6 +3133,11 @@ static int lookup_one_common(struct mnt_idmap *idmap,
*
* No locks need be held - only a counted reference to @base is needed.
*
+ * Returns:
+ * - ref-counted dentry on success, or
+ * - %NULL if name could not be found, or
+ * - ERR_PTR(-EACCES) if name is dot or dotdot or contains a slash or nul, or
+ * - ERR_PTR() if fs provide ->d_hash, and this returned an error.
*/
struct dentry *try_lookup_noperm(struct qstr *name, struct dentry *base)
{
@@ -3208,6 +3214,11 @@ EXPORT_SYMBOL(lookup_one);
*
* Unlike lookup_one, it should be called without the parent
* i_rwsem held, and will take the i_rwsem itself if necessary.
+ *
+ * Returns:= A dentry, possibly negative, or
+ * - same errors as try_lookup_noperm() or
+ * - ERR_PTR(-ENOENT) if parent has been removed, or
+ * - ERR_PTR(-EACCES) if parent directory is not searchable.
*/
struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap, struct qstr *name,
struct dentry *base)
@@ -3244,6 +3255,10 @@ EXPORT_SYMBOL(lookup_one_unlocked);
* It should be called without the parent i_rwsem held, and will take
* the i_rwsem itself if necessary. If a fatal signal is pending or
* delivered, it will return %-EINTR if the lock is needed.
+ *
+ * Returns: A dentry, possibly negative, or
+ * - same errors as lookup_one_unlocked() or
+ * - ERR_PTR(-EINTR) is a fatal signal is pending.
*/
struct dentry *lookup_one_positive_killable(struct mnt_idmap *idmap,
struct qstr *name,
@@ -3283,6 +3298,10 @@ EXPORT_SYMBOL(lookup_one_positive_killable);
* This can be used for in-kernel filesystem clients such as file servers.
*
* The helper should be called without i_rwsem held.
+ *
+ * Returns: A positive dentry, or
+ * - ERR_PTR(-ENOENT) if the name could not be found, or
+ * - same errors as lookup_one_unlocked().
*/
struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
struct qstr *name,
@@ -3311,6 +3330,10 @@ EXPORT_SYMBOL(lookup_one_positive_unlocked);
*
* Unlike try_lookup_noperm() it *does* revalidate the dentry if it already
* existed.
+ *
+ * Returns: A dentry, possibly negative, or
+ * - ERR_PTR(-ENOENT) if parent has been removed, or
+ * - same errors as try_lookup_noperm()
*/
struct dentry *lookup_noperm_unlocked(struct qstr *name, struct dentry *base)
{
@@ -3335,6 +3358,10 @@ EXPORT_SYMBOL(lookup_noperm_unlocked);
* _can_ become positive at any time, so callers of lookup_noperm_unlocked()
* need to be very careful; pinned positives have ->d_inode stable, so
* this one avoids such problems.
+ *
+ * Returns: A positive dentry, or
+ * - ERR_PTR(-ENOENT) if name cannot be found or parent has been removed, or
+ * - same errors as try_lookup_noperm()
*/
struct dentry *lookup_noperm_positive_unlocked(struct qstr *name,
struct dentry *base)
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
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