[PATCH] ocfs2: Fix possible deadlock in ocfs2_fiemap

Lizhi Xu posted 1 patch 1 month, 1 week ago
fs/ocfs2/extent_map.c |  2 ++
fs/ocfs2/mmap.c       | 14 +++++++-------
2 files changed, 9 insertions(+), 7 deletions(-)
[PATCH] ocfs2: Fix possible deadlock in ocfs2_fiemap
Posted by Lizhi Xu 1 month, 1 week ago
Syzbot reported a possible circular locking dependency.
WARNING: possible circular locking dependency detected
6.12.0-rc2-syzkaller-00205-g1d227fcc7222 #0 Not tainted
------------------------------------------------------
syz-executor161/5226 is trying to acquire lock:
ffff88807e907398 (&mm->mmap_lock){++++}-{3:3}, at: __might_fault+0xaa/0x120 mm/memory.c:6700

but task is already holding lock:
ffff8880720d0660 (&oi->ip_alloc_sem){++++}-{3:3}, at: ocfs2_fiemap+0x377/0xf80 fs/ocfs2/extent_map.c:755

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #2 (&oi->ip_alloc_sem){++++}-{3:3}:
       lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5825
       down_write+0x99/0x220 kernel/locking/rwsem.c:1577
       ocfs2_page_mkwrite+0x346/0xed0 fs/ocfs2/mmap.c:142
       do_page_mkwrite+0x198/0x480 mm/memory.c:3162
       do_shared_fault mm/memory.c:5358 [inline]
       do_fault mm/memory.c:5420 [inline]
       do_pte_missing mm/memory.c:3965 [inline]
       handle_pte_fault+0x11fa/0x6800 mm/memory.c:5751
       __handle_mm_fault mm/memory.c:5894 [inline]
       handle_mm_fault+0x1053/0x1ad0 mm/memory.c:6062
       do_user_addr_fault arch/x86/mm/fault.c:1389 [inline]
       handle_page_fault arch/x86/mm/fault.c:1481 [inline]
       exc_page_fault+0x2b9/0x8c0 arch/x86/mm/fault.c:1539
       asm_exc_page_fault+0x26/0x30 arch/x86/include/asm/idtentry.h:623

-> #1 (sb_pagefaults){.+.+}-{0:0}:
       lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5825
       percpu_down_read include/linux/percpu-rwsem.h:51 [inline]
       __sb_start_write include/linux/fs.h:1716 [inline]
       sb_start_pagefault include/linux/fs.h:1881 [inline]
       ocfs2_page_mkwrite+0x222/0xed0 fs/ocfs2/mmap.c:122
       do_page_mkwrite+0x198/0x480 mm/memory.c:3162
       do_shared_fault mm/memory.c:5358 [inline]
       do_fault mm/memory.c:5420 [inline]
       do_pte_missing mm/memory.c:3965 [inline]
       handle_pte_fault+0x11fa/0x6800 mm/memory.c:5751
       __handle_mm_fault mm/memory.c:5894 [inline]
       handle_mm_fault+0x1053/0x1ad0 mm/memory.c:6062
       do_user_addr_fault arch/x86/mm/fault.c:1389 [inline]
       handle_page_fault arch/x86/mm/fault.c:1481 [inline]
       exc_page_fault+0x2b9/0x8c0 arch/x86/mm/fault.c:1539
       asm_exc_page_fault+0x26/0x30 arch/x86/include/asm/idtentry.h:623

-> #0 (&mm->mmap_lock){++++}-{3:3}:
       check_prev_add kernel/locking/lockdep.c:3161 [inline]
       check_prevs_add kernel/locking/lockdep.c:3280 [inline]
       validate_chain+0x18ef/0x5920 kernel/locking/lockdep.c:3904
       __lock_acquire+0x1384/0x2050 kernel/locking/lockdep.c:5202
       lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5825
       __might_fault+0xc6/0x120 mm/memory.c:6700
       _inline_copy_to_user include/linux/uaccess.h:183 [inline]
       _copy_to_user+0x2a/0xb0 lib/usercopy.c:26
       copy_to_user include/linux/uaccess.h:216 [inline]
       fiemap_fill_next_extent+0x235/0x410 fs/ioctl.c:145
       ocfs2_fiemap+0x9f1/0xf80 fs/ocfs2/extent_map.c:796
       ioctl_fiemap fs/ioctl.c:220 [inline]
       do_vfs_ioctl+0x1bf8/0x2e40 fs/ioctl.c:841
       __do_sys_ioctl fs/ioctl.c:905 [inline]
       __se_sys_ioctl+0x81/0x170 fs/ioctl.c:893
       do_syscall_x64 arch/x86/entry/common.c:52 [inline]
       do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

other info that might help us debug this:

Chain exists of:
  &mm->mmap_lock --> sb_pagefaults --> &oi->ip_alloc_sem

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  rlock(&oi->ip_alloc_sem);
                               lock(sb_pagefaults);
                               lock(&oi->ip_alloc_sem);
  rlock(&mm->mmap_lock);

 *** DEADLOCK ***

Fix it by reordering locks sb_pagefaults and ip_alloc_sem, because
fiemap_fill_next_extent() does not need to be in the lock ip_alloc_sem,
cancel and retrieve ip_alloc_sem before and after the execution of it.

Reported-and-tested-by: syzbot+ca440b457d21568f8021@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ca440b457d21568f8021
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/ocfs2/extent_map.c |  2 ++
 fs/ocfs2/mmap.c       | 14 +++++++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
index f7672472fa82..6d5ffa803b31 100644
--- a/fs/ocfs2/extent_map.c
+++ b/fs/ocfs2/extent_map.c
@@ -793,8 +793,10 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		phys_bytes = le64_to_cpu(rec.e_blkno) << osb->sb->s_blocksize_bits;
 		virt_bytes = (u64)le32_to_cpu(rec.e_cpos) << osb->s_clustersize_bits;
 
+		up_read(&OCFS2_I(inode)->ip_alloc_sem);
 		ret = fiemap_fill_next_extent(fieinfo, virt_bytes, phys_bytes,
 					      len_bytes, fe_flags);
+		down_read(&OCFS2_I(inode)->ip_alloc_sem);
 		if (ret)
 			break;
 
diff --git a/fs/ocfs2/mmap.c b/fs/ocfs2/mmap.c
index 6ef4cb045ccd..f7863f7fb4a1 100644
--- a/fs/ocfs2/mmap.c
+++ b/fs/ocfs2/mmap.c
@@ -119,9 +119,6 @@ static vm_fault_t ocfs2_page_mkwrite(struct vm_fault *vmf)
 	int err;
 	vm_fault_t ret;
 
-	sb_start_pagefault(inode->i_sb);
-	ocfs2_block_signals(&oldset);
-
 	/*
 	 * The cluster locks taken will block a truncate from another
 	 * node. Taking the data lock will also ensure that we don't
@@ -131,7 +128,7 @@ static vm_fault_t ocfs2_page_mkwrite(struct vm_fault *vmf)
 	if (err < 0) {
 		mlog_errno(err);
 		ret = vmf_error(err);
-		goto out;
+		return ret;
 	}
 
 	/*
@@ -141,16 +138,19 @@ static vm_fault_t ocfs2_page_mkwrite(struct vm_fault *vmf)
 	 */
 	down_write(&OCFS2_I(inode)->ip_alloc_sem);
 
+	sb_start_pagefault(inode->i_sb);
+	ocfs2_block_signals(&oldset);
+
 	ret = __ocfs2_page_mkwrite(vmf->vma->vm_file, di_bh, page);
 
+	ocfs2_unblock_signals(&oldset);
+	sb_end_pagefault(inode->i_sb);
+
 	up_write(&OCFS2_I(inode)->ip_alloc_sem);
 
 	brelse(di_bh);
 	ocfs2_inode_unlock(inode, 1);
 
-out:
-	ocfs2_unblock_signals(&oldset);
-	sb_end_pagefault(inode->i_sb);
 	return ret;
 }
 
-- 
2.43.0