Message-ID: <CAGudoHHfGndcMwXMupOs82HM6c_ajMw8GETxPdkqzORrEq0btA@mail.gmail.com>
Date: Mon, 24 Nov 2025 10:01:53 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: brauner@...nel.org, neil@...wn.name
Cc: agruenba@...hat.com, almaz.alexandrovich@...agon-software.com, 
	dhowells@...hat.com, gfs2@...ts.linux.dev, jack@...e.cz, 
	linux-afs@...ts.infradead.org, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, marc.dionne@...istor.com, ntfs3@...ts.linux.dev, 
	syzkaller-bugs@...glegroups.com, viro@...iv.linux.org.uk, 
	syzbot+2fefb910d2c20c0698d8@...kaller.appspotmail.com
Subject: Re: [syzbot] [ntfs3?] INFO: task hung in __start_renaming

sigh, so it *is* my patch, based on syzbot testing specifically on
directory locking vs inode branches, but I don't see why.

I take it the open() codepath took the rwsem, hence the rename is
sleeping. Given that all reproducers find it *on* cpu, it may be this
is busy looping for some reason.

I don't have time to dig more into it right now, so I think it would
be best to *drop* my patch for the time being. Once I figure it out
I'll send a v2.

On Mon, Nov 24, 2025 at 8:54 AM Mateusz Guzik <mjguzik@...il.com> wrote:
>
> On Sun, Nov 23, 2025 at 11:13:03PM -0800, syzbot wrote:
> > syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> > INFO: task hung in __start_renaming
> >
> > INFO: task syz.0.17:6473 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.17        state:D stack:27936 pid:6473  tgid:6471  ppid:6352   task_flags:0x400040 flags:0x00080002
> > Call Trace:
> >  <TASK>
> >  context_switch kernel/sched/core.c:5257 [inline]
> >  __schedule+0x14bc/0x5030 kernel/sched/core.c:6864
> >  __schedule_loop kernel/sched/core.c:6946 [inline]
> >  schedule+0x165/0x360 kernel/sched/core.c:6961
> >  schedule_preempt_disabled+0x13/0x30 kernel/sched/core.c:7018
> >  rwsem_down_write_slowpath+0x872/0xfe0 kernel/locking/rwsem.c:1185
> >  __down_write_common kernel/locking/rwsem.c:1317 [inline]
> >  __down_write kernel/locking/rwsem.c:1326 [inline]
> >  down_write_nested+0x1b5/0x200 kernel/locking/rwsem.c:1707
> >  inode_lock_nested include/linux/fs.h:1072 [inline]
> >  lock_rename fs/namei.c:3681 [inline]
> >  __start_renaming+0x148/0x410 fs/namei.c:3777
> >  do_renameat2+0x399/0x8e0 fs/namei.c:5991
> >  __do_sys_rename fs/namei.c:6059 [inline]
> >  __se_sys_rename fs/namei.c:6057 [inline]
> >  __x64_sys_rename+0x82/0x90 fs/namei.c:6057
> >  do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> >  do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
> >  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > RIP: 0033:0x7f2425d8f749
> > RSP: 002b:00007f2426c44038 EFLAGS: 00000246 ORIG_RAX: 0000000000000052
> > RAX: ffffffffffffffda RBX: 00007f2425fe6090 RCX: 00007f2425d8f749
> > RDX: 0000000000000000 RSI: 0000200000000080 RDI: 0000200000000340
> > RBP: 00007f2425e13f91 R08: 0000000000000000 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> > R13: 00007f2425fe6128 R14: 00007f2425fe6090 R15: 00007ffcd5a91138
> >  </TASK>
> >
>
> So at the end of the day I think this erroneously bisected to my patch.
> While it may sound like famous last words, hear me out.
>
> The reproducer fails to trigger the problem on my test jig, thus I
> resorted to asking syzbot.
>
> First patch I sent for testing was merely a sanity check -- prior to my
> patch inode_unhashed() checks were *always* executing, with my patch
> they only happen if I_NEW was spotted. This patch "solved" the problem.
>
> Second patch added some asserts on inode_unhashed() and syzbot had some
> internal issues, ultimately it was not tested.
>
> Third patch added even *more* asserts and t0his time around things
> failed the previously reported way about half an hour of testing. But if
> the first patch indeed solved something, the BUG_ONs would have
> triggered instead.
>
> So that's for testing.
>
> In my first response I made a remark in my first reply that ntfs is at
> fault.  After a quick skim I spotted d_instantiate() instead of
> d_instantiate_new() and jumped to conclusions. It calls unlock_new_inode()
> later which does the right thing, so it is fine AFAICS.
>
> So what about correctness of my patch?
>
> My patch lifted the go-to-sleep code from inode_wait_for_lru_isolating().
> In principle that can be buggy but is just used rarely enough that it
> went unnoticed. I don't see anything wrong with it though, including
> after comparing it with __wait_on_freeing_inode(). Notably both
> synchronize with the ->i_lock. No games played.
>
> I figure maybe there is something fucky going on with ordering on wakeup
> side:
>
> inode_state_clear(inode, I_NEW | I_CREATING);
> inode_wake_up_bit(inode, __I_NEW);
>
> Going through __wake_up_common_lock takes a spinlock, which on amd64
> would have a side effect of publishing that I_NEW store, even ignoring
> therest of the ordering.
>
> On going to sleep side to the flag is only ever tested with ->i_lock
> held anyway, so it can't be an ordering issue on that front. The thread
> could not have been missed from the sleepers list as going to sleep is
> again ->i_lock protected, with the lock only dropped around the call to
> schedule().
>
> So I don't see how this can be buggy.
>
> At the same time the traces report the thing off cpu is playing around
> with rwsems with the __start_renaming et al patchset, while the code for
> inode hash manipulation is decidedly *ON* cpu -- reported by NMIs, not
> hung test detector.
>
> In principle this still can be a thread hung waiting on I_NEW somewhere,
> but syzbot did not produce a collection of backtraces for other threads.
>
> However, given that the __start_renaming et al patchset is complicated
> *and* that syzbot could mistakenly report (or not) a bug I'm led to
> conclude the reproducer is highly unreliable and my commit landed as a
> random victim.
>
> All that said, I think the folk working on that patchset should take
> over.
>
> My patch is a minor optimization and can be skipped in this merge window
> at no real loss,
>
> My take is that the big patchset *should* be skipped in this merge
> window given the above, unless the problem is uickly identified.

Powered by blists - more mailing lists