affs_remove_header() acquires i_hash_lock on the parent directory at the top of the function and again on the child inode inside the ST_USERDIR arm, both via affs_lock_dir() which uses SINGLE_DEPTH_NESTING. Lockdep sees two acquisitions of the same lock class in the same task at the same subclass and warns: WARNING: possible recursive locking detected syz.2.17/3573 is trying to acquire lock: ffff888123db0778 (&ei->i_ext_lock/1){+.+.}-{4:4}, at: affs_lock_dir affs_remove_header+0x72f fs/affs/amigaffs.c:296 but task is already holding lock: ffff888123db0100 (&ei->i_ext_lock/1){+.+.}-{4:4}, at: affs_lock_dir affs_remove_header+0x261 fs/affs/amigaffs.c:289 Call Trace: affs_lock_dir fs/affs/affs.h:311 [inline] affs_remove_header+0x72f/0x1ab0 fs/affs/amigaffs.c:296 vfs_rmdir+0x20b/0x6a0 fs/namei.c:4446 do_rmdir+0x2ed/0x3c0 fs/namei.c:4524 __x64_sys_unlinkat+0xf4/0x140 fs/namei.c:4692 With panic_on_warn this terminates the kernel. The two lock instances in the report are distinct inodes (db0778 vs db0100), so what lockdep is reporting is two acquisitions of the same lock class at the same subclass -- a missing subclass distinction in the annotation. Trigger requires the ability to mount a crafted image (CAP_SYS_ADMIN or equivalent) and is reproduced by rmdir() of a subdirectory under a crafted AFFS image. The same locking pattern is also vulnerable to a strict self-deadlock on a crafted image whose on-disk hash table contains an entry with i_ino equal to its containing directory's own block number: affs_iget() would return the same in-memory inode for both d_inode(dentry) and d_inode(dentry->d_parent), and the two affs_lock_dir() calls would take the same mutex twice. No lockdep distinction would prevent this since the same lock instance is acquired twice. Address both by: 1. Adding affs_lock_subdir() that uses SINGLE_DEPTH_NESTING + 1, so lockdep can prove the nested parent->child acquisition is safe. 2. Rejecting the case where the parsed child inode coincides with the parent before taking the second lock, returning -EIO under the existing done_unlock path so all previously-acquired locks are released cleanly. Reported-by: Farhad Alemi Closes: https://lore.kernel.org/lkml/CA+0ovCiA7huMwMxvWgC8km2P+gJwd-jax+ACo=EbGrJ6FVp55A@mail.gmail.com/ Signed-off-by: Farhad Alemi --- fs/affs/affs.h | 6 ++++++ fs/affs/amigaffs.c | 6 +++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/fs/affs/affs.h b/fs/affs/affs.h index a0caf6ace860..c4af169294ed 100644 --- a/fs/affs/affs.h +++ b/fs/affs/affs.h @@ -313,6 +313,12 @@ affs_lock_dir(struct inode *inode) mutex_lock_nested(&AFFS_I(inode)->i_hash_lock, SINGLE_DEPTH_NESTING); } static inline void +affs_lock_subdir(struct inode *inode) +{ + mutex_lock_nested(&AFFS_I(inode)->i_hash_lock, + SINGLE_DEPTH_NESTING + 1); +} +static inline void affs_unlock_dir(struct inode *inode) { mutex_unlock(&AFFS_I(inode)->i_hash_lock); diff --git a/fs/affs/amigaffs.c b/fs/affs/amigaffs.c index bed4fc805e8e..fefc2bb11d43 100644 --- a/fs/affs/amigaffs.c +++ b/fs/affs/amigaffs.c @@ -293,7 +293,11 @@ affs_remove_header(struct dentry *dentry) * i_hash_lock of the inode must only be * taken after some checks */ - affs_lock_dir(inode); + if (inode == dir) { + retval = -EIO; + goto done_unlock; + } + affs_lock_subdir(inode); retval = affs_empty_dir(inode); affs_unlock_dir(inode); if (retval) -- 2.43.0