Re: [PATCH] fs/ntfs3: use d_instantiate_new() in ntfs_create_inode() and murder syzbot's "WARNING in do_new_mount" saga

[Jan Kara]

On Wed 09-09-26 11:03:18, Christian Brauner wrote:
> ntfs_create_inode() creates a new inode via ntfs_new_inode(). It hashes
> it with insert_inode_locked() and so it's marked as I_NEW until
> unlock_new_inode().
> 
> ntfs 3 calls d_instantiate() in between though... Since the dentry was
> already hashed by the lookup before the create any path walk finds it
> without touching the parent's i_rwsem and so can lock the inode.
> 
> If the inode is a directory unlock_new_inode() calls
> lockdep_annotate_inode_mutex_key() and marks i_rwsem with the
> i_mutex_dir_key class.
> 
> That resets the count and the owner of a lock somebody else may already
> hold by now...
> 
> syzbot has been spamming us with the same godforsaken bug
> 
>   "WARNING in do_new_mount"
> 
> since 2023. I can't take it anymore so I went looking. Afaict, syzbot's
> executor chdirs into a freshly mounted ntfs3 image, creates a
> directory and then mounts some pseudofs on it. Everytime the mkdir()
> takes longer than syzbot waits mount() runs concurrently:
> 
>   mkdir("./sys")                        mount(NULL, "./sys", "sysfs")
>   ntfs_create_inode()
>     d_instantiate()
>                                         user_path_at() finds the dentry
>                                         do_lock_mount()
>                                           inode_lock(inode)
>                                           namespace_lock()
>     unlock_new_inode()
>       lockdep_annotate_inode_mutex_key()
>         init_rwsem(&inode->i_rwsem)
>                                         unlock_mount()
>                                           inode_unlock(inode)
> 
> The mount side then releases a lock that according to the rwsem nobody
> holds:
> 
>   DEBUG_RWSEMS_WARN_ON((rwsem_owner(sem) != current) && ...):
>   count = 0x0, magic = 0xffff888043a854e8, owner = 0x0,
>   curr 0xffff888000244880, list empty
>   WARNING: CPU: 0 PID: 5346 at kernel/locking/rwsem.c:1368 __up_write
>   Call Trace:
>    inode_unlock include/linux/fs.h:877 [inline]
>    unlock_mount fs/namespace.c:2892 [inline]
>    do_new_mount_fc fs/namespace.c:3828 [inline]
>    do_new_mount+0x777/0xa40 fs/namespace.c:3887
> 
> On PREEMPT_RT the same thing shows up as
> 
>   DEBUG_LOCKS_WARN_ON(rt_mutex_owner(lock) != current)
>   WARNING: kernel/locking/rtmutex_common.h:193 at rt_mutex_slowunlock
> 
> The up_write() underflows the reset count. A following inode_lock() on
> that directory then never returns. A path walk into the new directory
> racing with the mkdir() corrupts the lock the same way via
> inode_lock_shared() in lookup_slow().
> 
> Switch to d_instantiate_new() and drop the trailing unlock_new_inode().
> All error paths bail out before that point with I_NEW still set and
> keep using discard_new_inode().
> 
> May we never see this fscking bug report again.
> 
> Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
> Cc: stable@xxxxxxxxxxxxxxx # v5.15+
> Reported-by: syzbot+2a13ad6914e6fcec716c@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://lore.kernel.org/6a9beced.a5e650b3.26d8a.000b.GAE@xxxxxxxxxx
> Signed-off-by: Christian Brauner (Amutable) <brauner@xxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/ntfs3/inode.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
> index 56b4f6469a28..4ac26c80bd34 100644
> --- a/fs/ntfs3/inode.c
> +++ b/fs/ntfs3/inode.c
> @@ -1866,10 +1866,10 @@ int ntfs_create_inode(struct mnt_idmap *idmap, struct inode *dir,
>  		goto out6;
>  
>  	/*
> -	 * Call 'd_instantiate' after inode->i_op is set
> +	 * Call 'd_instantiate_new' after inode->i_op is set
>  	 * but before finish_open.
>  	 */
> -	d_instantiate(dentry, inode);
> +	d_instantiate_new(dentry, inode);
>  
>  	/* Set original time. inode times (i_ctime) may be changed in ntfs_init_acl. */
>  	inode_set_atime_to_ts(inode, ni->i_crtime);
> @@ -1917,9 +1917,6 @@ int ntfs_create_inode(struct mnt_idmap *idmap, struct inode *dir,
>  	if (!fnd)
>  		ni_unlock(dir_ni);
>  
> -	if (!err)
> -		unlock_new_inode(inode);
> -
>  	return err;
>  }
>  
> 
> ---
> base-commit: 893e11787f78e43b534e252249ac3fff4d1333f8
> change-id: 20260909-work-ntfs3-d_instantiate_new-814ad31dea82
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR