Re: [PATCH v2] ocfs2: use inode_lock_nested() for orphan dir locking

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Incorrect assignment of the I_MUTEX_NONDIR2 subclass to the orphan directory violates VFS semantics, introduces a new lockdep cycle during rename, and fragments lockdep tracking.

Pre-existing issues:
- [Critical] Unvalidated on-disk fields i_dio_orphaned_slot and i_orphaned_slot are used as array indices, leading to out-of-bounds memory accesses.
--

commit f1e102271ff71f1f4452e0af815188536b47c93e
Author: Deepanshu Kartikey <kartikey406@xxxxxxxxx>

ocfs2: use inode_lock_nested() for orphan dir locking
    
PREEMPT_RT's rtmutex PI chain walker warns about a lock dependency
cycle when inode_lock(orphan_dir_inode) is called while holding
inode_lock(file_inode):

[ ... ]

Closes: https://syzkaller.appspot.com/bug?extid=ce129763ce7d7e914739

> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index 1277666c77cd2..4cfd7b3d3e1a4 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -2126,7 +2126,7 @@ static int ocfs2_lookup_lock_orphan_dir(struct ocfs2_super *osb,
>  		return ret;
>  	}
>  
> -	inode_lock(orphan_dir_inode);
> +	inode_lock_nested(orphan_dir_inode, I_MUTEX_NONDIR2);

[Severity: High]
Does this violate vfs locking rules by assigning a non-directory lock class
to a directory? The orphan_dir_inode is a directory, so it expects a parent
lock class like I_MUTEX_PARENT.

Also, are we missing other locations that lock the orphan directory?
For example, in fs/ocfs2/inode.c:ocfs2_wipe_inode() and
fs/ocfs2/journal.c:ocfs2_recover_orphans(), we still call
inode_lock(orphan_dir_inode). Could using different subclasses for the same
inode corrupt lockdep dependency tracking?

[ ... ]

> @@ -2725,7 +2725,7 @@ int ocfs2_del_inode_from_orphan(struct ocfs2_super *osb,
>  		goto bail;
>  	}
>  

[Severity: Critical]
This is a pre-existing issue, but can the unvalidated on-disk value
di->i_dio_orphaned_slot lead to an out-of-bounds access?

A few lines above, we read the slot directly from the on-disk buffer:

    orphan_dir_inode = ocfs2_get_system_file_inode(osb,
            ORPHAN_DIR_SYSTEM_INODE,
            le16_to_cpu(di->i_dio_orphaned_slot));

If the on-disk slot value is maliciously crafted,
fs/ocfs2/sysfile.c:get_local_system_inode() will use it as an array
index for osb->local_system_inodes, leading to an out-of-bounds read.

Similarly, could this cause an out-of-bounds write in
fs/ocfs2/inode.c:ocfs2_check_orphan_recovery_state() when it increments
osb->osb_orphan_wipes[slot]?

> -	inode_lock(orphan_dir_inode);
> +	inode_lock_nested(orphan_dir_inode, I_MUTEX_NONDIR2);

[ ... ]

> @@ -2838,7 +2838,7 @@ int ocfs2_mv_orphaned_inode_to_new(struct inode *dir,
>  		goto leave;
>  	}
>  
> -	inode_lock(orphan_dir_inode);
> +	inode_lock_nested(orphan_dir_inode, I_MUTEX_NONDIR2);

[Severity: High]
Could using I_MUTEX_NONDIR2 here introduce a new lockdep cycle during
file-over-file rename operations?

In the rename path, vfs_rename() uses lock_two_nondirectories() to lock the
target file with I_MUTEX_NONDIR2:

fs/inode.c:lock_two_nondirectories() {
    ...
    inode_lock_nested(inode2, I_MUTEX_NONDIR2);
    ...
}

If ocfs2_rename() then orphans the overwritten target file, it calls
ocfs2_prepare_orphan_dir(), which will now try to acquire the orphan
directory lock with I_MUTEX_NONDIR2. Could acquiring subclass 4 while
already holding subclass 4 trigger a 'possible recursive locking detected'
warning?

>  
>  	status = ocfs2_inode_lock(orphan_dir_inode, &orphan_dir_bh, 1);
>  	if (status < 0) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260620231223.46588-1-kartikey406@xxxxxxxxx?part=1