Re: [syzbot] [ntfs3?] INFO: task hung in do_user_addr_fault (3)

[Hillf Danton <hdanton@sina.com>]

On 2 Jan 2023 17:24:24 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> On 2023/01/02 10:40, Linus Torvalds wrote:
> > So I think that we have:
> > 
> >  - ntfs_truncate()  gets the ni_lock (fs/ntfs3/file.c:393)
> > 
> >  - it then - while holding that lock - calls (on line 395):
> > 
> >       truncate_setsize ->
> >          truncate_pagecache ->
> >             truncate_inode_pages ->
> >                truncate_inode_pages_range ->
> >                   folio_lock
> > 
> > but that deadlocks on another process that wants to read that page,
> > and that needs ni_lock to do so.
> > 
> > So yes, it does look like a ntfs3 deadlock involving ni_lock.
> 
> Yes, I think you are right. My patch confirmed that other threads are not
> holding ni_lock lock, which means that this is a deadlock between
> PG_locked bit and ni_lock lock.
> 
> filemap_update_page() calls filemap_read_folio() after calling
> folio_trylock(). Since folio_trylock() sets PG_locked bit,
> 
>  mutex_lock_nested+0x17/0x20 kernel/locking/mutex.c:799
>  ni_lock fs/ntfs3/ntfs_fs.h:1122 [inline]
>  attr_data_get_block+0x4a6/0x2e40 fs/ntfs3/attrib.c:919
>  ntfs_get_block_vbo+0x374/0xd20 fs/ntfs3/inode.c:573
>  do_mpage_readpage+0x98b/0x1bb0 fs/mpage.c:208
>  mpage_read_folio+0x103/0x1d0 fs/mpage.c:379
>  filemap_read_folio+0x1ba/0x7f0 mm/filemap.c:2426
>  filemap_update_page+0x3ca/0x550 mm/filemap.c:2511
>  filemap_get_pages+0x8d8/0x1110 mm/filemap.c:2624
>  filemap_read+0x3e7/0xee0 mm/filemap.c:2694
> 
> is trying to take ni_lock after setting PG_locked bit.
> 
> On the other hand, folio_lock() waits until PG_locked bit is cleared,
> but unfortunately ntfs3_setattr() already took ni_lock before calling
> folio_lock().
> 
>  io_schedule+0x83/0x100 kernel/sched/core.c:8811
>  folio_wait_bit_common+0x8ca/0x1390 mm/filemap.c:1297
>  folio_lock include/linux/pagemap.h:938 [inline]
>  truncate_inode_pages_range+0xc8d/0x1650 mm/truncate.c:421
>  truncate_inode_pages mm/truncate.c:448 [inline]
>  truncate_pagecache mm/truncate.c:743 [inline]
>  truncate_setsize+0xcb/0xf0 mm/truncate.c:768
>  ntfs_truncate fs/ntfs3/file.c:395 [inline]
>  ntfs3_setattr+0x5a5/0xca0 fs/ntfs3/file.c:696
> 
> Since no lockdep annotation is used for e.g. PG_locked bit, this deadlock
> cannot be detected by lockdep...

Good work, Tetsuo, thanks.

Given inode_lock before ni_lock in the reported lock chain,

> > 3 locks held by syz-executor394/5222:
> >  #0: ffff88801ee04460 (sb_writers#9){.+.+}-{0:0}, at: mnt_want_write+0x3b/0x80 fs/namespace.c:508
> >  #1: ffff888073930b00 (&sb->s_type->i_mutex_key#14){+.+.}-{3:3}, at: inode_lock include/linux/fs.h:756 [inline]
> >  #1: ffff888073930b00 (&sb->s_type->i_mutex_key#14){+.+.}-{3:3}, at: do_truncate+0x205/0x300 fs/open.c:63
> >  #2: ffff888073930860 (&ni->ni_lock/4){+.+.}-{3:3}, at: ni_lock fs/ntfs3/ntfs_fs.h:1122 [inline]
> >  #2: ffff888073930860 (&ni->ni_lock/4){+.+.}-{3:3}, at: ntfs_truncate fs/ntfs3/file.c:393 [inline]
> >  #2: ffff888073930860 (&ni->ni_lock/4){+.+.}-{3:3}, at: ntfs3_setattr+0x596/0xca0 fs/ntfs3/file.c:696

a workaround is take ni_lock after truncate_setsize(). Lets see the echo
from syzbot.

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git  master

--- x/fs/ntfs3/file.c
+++ y/fs/ntfs3/file.c
@@ -390,10 +390,10 @@ static int ntfs_truncate(struct inode *i
 
 	new_valid = ntfs_up_block(sb, min_t(u64, ni->i_valid, new_size));
 
-	ni_lock(ni);
-
 	truncate_setsize(inode, new_size);
 
+	ni_lock(ni);
+
 	down_write(&ni->file.run_lock);
 	err = attr_set_size(ni, ATTR_DATA, NULL, 0, &ni->file.run, new_size,
 			    &new_valid, ni->mi.sbi->options->prealloc, NULL);
--