Re: [PATCH] ocfs2: fix lockdep class of newly created directories

[Joseph Qi]

On 11/10/26 9:45 AM, Shuai Liu wrote:
> Directories and regular files use different lockdep classes for
> i_rwsem (i_mutex_dir_key vs i_mutex_key) so that lockdep can verify
> the documented locking order "parent[2] -> child -> normal" without
> spurious class-level cycles: e.g. cross-directory rename takes
> I_MUTEX_CHILD on the source directory followed by an unclassed
> I_MUTEX_NORMAL on the target inode, while other paths take
> I_MUTEX_NORMAL on a file before touching a child-class inode.
> 
> Since commit 14358e6ddaed ("lockdep: annotate dir vs file i_mutex"),
> this split is applied in unlock_new_inode(), which relies on the
> inode being created with I_NEW set.  ocfs2 creates inodes through
> new_inode() (no I_NEW) and instantiates them with a plain
> d_instantiate(), so lockdep_annotate_inode_mutex_key() never runs on
> this path.  Every directory created after mount therefore keeps the
> plain i_mutex_key class for its whole lifetime, breaking the
> dir/file class separation that the VFS locking order relies on.
> 
> With such a directory the class-level chain
> 
>   i_mutex_key/I_MUTEX_NORMAL -> i_mutex_key/I_MUTEX_CHILD
> 
> recorded by the reflink path (ocfs2_vfs_reflink() locks the source
> inode, then __ocfs2_reflink() takes I_MUTEX_CHILD on the orphan
> inode) collides with the chain
> 
>   i_mutex_key/I_MUTEX_CHILD -> i_mutex_key
> 
> recorded by vfs_rename() when renaming a directory onto an existing
> file, and lockdep reports a "possible deadlock in vfs_rename":
> 
>   syz-executor105/5623 is trying to acquire lock:
>   ffff8880767009a0 (&sb->s_type->i_mutex_key#18){+.+.}-{4:4}, at: vfs_rename+0x6e3
>   but task is already holding lock:
>   ffff8880764ff9a0 (&sb->s_type->i_mutex_key#18/2){+.+.}-{4:4}, at: vfs_rename+0x64d
> 
>   Chain exists of:
>     &sb->s_type->i_mutex_key#18 --> &oi->ip_alloc_sem --> &sb->s_type->i_mutex_key#18/2
> 
> Fix it by calling lockdep_annotate_inode_mutex_key() in
> ocfs2_get_init_inode() right after the final mode is established,
> before the inode can be found or locked by anyone else.  This is the
> same treatment shmem got (commit b45d71fb89ab8adf, "mm: shmem.c:
> Correctly annotate new inodes for lockdep"), following the hugetlbfs
> pattern of annotating right after new inode creation.  Freshly
> created ocfs2 directories are now properly separated from files in
> lockdep and the rename vs reflink cycle disappears.
> 
> The C reproducer from syzbot (mount ocfs2 image, OCFS2_IOC_REFLINK,
> mkdir two directories, rename a directory onto the reflinked file)
> hits the lockdep warning on every run without the fix and runs
> cleanly with it.
> 
> Reported-by: syzbot+c73ff6790f3d6c635abc@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=c73ff6790f3d6c635abc
> Fixes: 14358e6ddaed ("lockdep: annotate dir vs file i_mutex")
> Signed-off-by: Shuai Liu <847807867@xxxxxx>

Looks fine.
Reviewed-by: Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx>
> ---
>  fs/ocfs2/namei.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index d8759605786c4..f3ec348eb709d 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -203,6 +203,13 @@ static struct inode *ocfs2_get_init_inode(struct inode *dir, umode_t mode)
>  		set_nlink(inode, 2);
>  	mode = mode_strip_sgid(&nop_mnt_idmap, dir, mode);
>  	inode_init_owner(&nop_mnt_idmap, inode, dir, mode);
> +	/*
> +	 * This inode never goes through unlock_new_inode() (there is no
> +	 * I_NEW state on this path), so the directory vs file lockdep
> +	 * class split for i_rwsem has to be done here, before the inode
> +	 * becomes visible to anybody else.
> +	 */
> +	lockdep_annotate_inode_mutex_key(inode);
>  	status = dquot_initialize(inode);
>  	if (status) {
>  		iput(inode);