Skip to contentSkip to conversation

ocfs2: Fix circular locking dependency in ocfs2_del_inode_from_orphan()

PATCH In review Prithvi Tambewagh 4 participants 10 messages Jan 6 – Feb 28

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ocfs2: Fix circular locking dependency in ocfs2_del_inode_from_orphan()

A possible circular locking dependency in ocfs2_del_inode_from_orphan() is detected by syzbot, which occurs due to change in the sequence of acquiring locks. The existing chain is:

&ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE] --> &dquot->dq_lock --> &ocfs2_quota_ip_alloc_sem_key

In ocfs2_dio_end_io_write(), &ocfs2_quota_ip_alloc_sem_key is acquired, and then ocfs2_del_inode_from_orphan() is called, which acquires &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]. This opposes the existing dependency chain:

-> #3 (&ocfs2_quota_ip_alloc_sem_key){++++}-{4:4}:

       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       down_write+0x3a/0x50 kernel/locking/rwsem.c:1590
       ocfs2_create_local_dquot+0x19d/0x1a40 fs/ocfs2/quota_local.c:1227
       ocfs2_acquire_dquot+0x80f/0xb30 fs/ocfs2/quota_global.c:883
       dqget+0x7c1/0xf20 fs/quota/dquot.c:980
       __dquot_initialize+0x3b3/0xcb0 fs/quota/dquot.c:1508
       ocfs2_get_init_inode+0x13b/0x1b0 fs/ocfs2/namei.c:205
       ocfs2_mknod+0x863/0x2050 fs/ocfs2/namei.c:313
       ocfs2_create+0x1a5/0x440 fs/ocfs2/namei.c:676
       lookup_open fs/namei.c:3796 [inline]
       open_last_lookups fs/namei.c:3895 [inline]
       path_openat+0x1500/0x3840 fs/namei.c:4131
       do_filp_open+0x1fa/0x410 fs/namei.c:4161
       do_sys_openat2+0x121/0x1c0 fs/open.c:1437
       do_sys_open fs/open.c:1452 [inline]
       __do_sys_openat fs/open.c:1468 [inline]
       __se_sys_openat fs/open.c:1463 [inline]
       __x64_sys_openat+0x138/0x170 fs/open.c:1463
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #2 (&dquot->dq_lock){+.+.}-{4:4}:

       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       __mutex_lock_common kernel/locking/rtmutex_api.c:535 [inline]
       mutex_lock_nested+0x5a/0x1d0 kernel/locking/rtmutex_api.c:547
       wait_on_dquot fs/quota/dquot.c:357 [inline]
       dqget+0x73a/0xf20 fs/quota/dquot.c:975
       __dquot_initialize+0x3b3/0xcb0 fs/quota/dquot.c:1508
       ocfs2_get_init_inode+0x13b/0x1b0 fs/ocfs2/namei.c:205
       ocfs2_mknod+0x863/0x2050 fs/ocfs2/namei.c:313
       ocfs2_create+0x1a5/0x440 fs/ocfs2/namei.c:676
       lookup_open fs/namei.c:3796 [inline]
       open_last_lookups fs/namei.c:3895 [inline]
       path_openat+0x1500/0x3840 fs/namei.c:4131
       do_filp_open+0x1fa/0x410 fs/namei.c:4161
       do_sys_openat2+0x121/0x1c0 fs/open.c:1437
       do_sys_open fs/open.c:1452 [inline]
       __do_sys_openat fs/open.c:1468 [inline]
       __se_sys_openat fs/open.c:1463 [inline]
       __x64_sys_openat+0x138/0x170 fs/open.c:1463
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #1 (&ocfs2_sysfile_lock_key[INODE_ALLOC_SYSTEM_INODE]){+.+.}-{4:4}:

       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       down_write+0x3a/0x50 kernel/locking/rwsem.c:1590
       inode_lock include/linux/fs.h:980 [inline]
       ocfs2_remove_inode fs/ocfs2/inode.c:731 [inline]
       ocfs2_wipe_inode fs/ocfs2/inode.c:894 [inline]
       ocfs2_delete_inode fs/ocfs2/inode.c:1155 [inline]
       ocfs2_evict_inode+0x153d/0x40d0 fs/ocfs2/inode.c:1295
       evict+0x504/0x9c0 fs/inode.c:810
       do_unlinkat+0x39f/0x570 fs/namei.c:4744
       __do_sys_unlinkat fs/namei.c:4778 [inline]
       __se_sys_unlinkat fs/namei.c:4771 [inline]
       __x64_sys_unlinkat+0xd3/0xf0 fs/namei.c:4771
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #0 (&ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]){+.+.}-{4:4}:

       check_prev_add kernel/locking/lockdep.c:3165 [inline]
       check_prevs_add kernel/locking/lockdep.c:3284 [inline]
       validate_chain+0xb9b/0x2140 kernel/locking/lockdep.c:3908
       __lock_acquire+0xab9/0xd20 kernel/locking/lockdep.c:5237
       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       down_write+0x3a/0x50 kernel/locking/rwsem.c:1590
       inode_lock include/linux/fs.h:980 [inline]
       ocfs2_del_inode_from_orphan+0x134/0x740 fs/ocfs2/namei.c:2730
       ocfs2_dio_end_io_write fs/ocfs2/aops.c:2306 [inline]
       ocfs2_dio_end_io+0x47b/0x1100 fs/ocfs2/aops.c:2404
       dio_complete+0x25e/0x790 fs/direct-io.c:281
       __blockdev_direct_IO+0x2bc0/0x31f0 fs/direct-io.c:1303
       ocfs2_direct_IO+0x260/0x2d0 fs/ocfs2/aops.c:2441
       generic_file_direct_write+0x1dc/0x3e0 mm/filemap.c:4189
       __generic_file_write_iter+0x120/0x240 mm/filemap.c:4358
       ocfs2_file_write_iter+0x157d/0x1d20 fs/ocfs2/file.c:2469
       iter_file_splice_write+0x97a/0x10f0 fs/splice.c:738
       do_splice_from fs/splice.c:938 [inline]
       direct_splice_actor+0x104/0x160 fs/splice.c:1161
       splice_direct_to_actor+0x5b3/0xcd0 fs/splice.c:1105
       do_splice_direct_actor fs/splice.c:1204 [inline]
       do_splice_direct+0x187/0x270 fs/splice.c:1230
       do_sendfile+0x4ec/0x7f0 fs/read_write.c:1370
       __do_sys_sendfile64 fs/read_write.c:1431 [inline]
       __se_sys_sendfile64+0x13e/0x190 fs/read_write.c:1417
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

Fix this by acquiring &ocfs2_quota_ip_alloc_sem_key after acquiring &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE], effectively calling down_write(&oi->ip_alloc_sem) after the call to ocfs2_del_inode_from_orphan().

---
 fs/ocfs2/aops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/ocfs2/aops.c +2 -2
index 76c86f1c2b1c..586e3b74d782 100644
@@ -2295,8 +2295,6 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 		goto out;
 	}
 
-	down_write(&oi->ip_alloc_sem);
-
 	/* Delete orphan before acquire i_rwsem. */
 	if (dwc->dw_orphaned) {
 		BUG_ON(dwc->dw_writer_pid != task_pid_nr(current));
@@ -2309,6 +2307,8 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 			mlog_errno(ret);
 	}
 
+	down_write(&oi->ip_alloc_sem);
+
 	di = (struct ocfs2_dinode *)di_bh->b_data;
 
 	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh);

base-commit: 765e56e41a5af2d456ddda6cbd617b9d3295ab4e

Signature · 1 line
2.34.1
maintainer
#1 lore raw
Quoted 4 lines · Prithvi Tambewagh

A possible circular locking dependency in ocfs2_del_inode_from_orphan() is detected by syzbot, which occurs due to change in the sequence of acquiring locks. The existing chain is:

Quoted 8 lines

&ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE] --> &dquot->dq_lock --> &ocfs2_quota_ip_alloc_sem_key

In ocfs2_dio_end_io_write(), &ocfs2_quota_ip_alloc_sem_key is acquired, and then ocfs2_del_inode_from_orphan() is called, which acquires &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]. This opposes the existing dependency chain:

IIUC, ocfs2_dio_end_io_write() only gets the &ocfs2_file_ip_alloc_sem_key. So how it happens?

Thanks,
Joseph

Quoted 131 lines

-> #3 (&ocfs2_quota_ip_alloc_sem_key){++++}-{4:4}:

       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       down_write+0x3a/0x50 kernel/locking/rwsem.c:1590
       ocfs2_create_local_dquot+0x19d/0x1a40 fs/ocfs2/quota_local.c:1227
       ocfs2_acquire_dquot+0x80f/0xb30 fs/ocfs2/quota_global.c:883
       dqget+0x7c1/0xf20 fs/quota/dquot.c:980
       __dquot_initialize+0x3b3/0xcb0 fs/quota/dquot.c:1508
       ocfs2_get_init_inode+0x13b/0x1b0 fs/ocfs2/namei.c:205
       ocfs2_mknod+0x863/0x2050 fs/ocfs2/namei.c:313
       ocfs2_create+0x1a5/0x440 fs/ocfs2/namei.c:676
       lookup_open fs/namei.c:3796 [inline]
       open_last_lookups fs/namei.c:3895 [inline]
       path_openat+0x1500/0x3840 fs/namei.c:4131
       do_filp_open+0x1fa/0x410 fs/namei.c:4161
       do_sys_openat2+0x121/0x1c0 fs/open.c:1437
       do_sys_open fs/open.c:1452 [inline]
       __do_sys_openat fs/open.c:1468 [inline]
       __se_sys_openat fs/open.c:1463 [inline]
       __x64_sys_openat+0x138/0x170 fs/open.c:1463
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #2 (&dquot->dq_lock){+.+.}-{4:4}:

       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       __mutex_lock_common kernel/locking/rtmutex_api.c:535 [inline]
       mutex_lock_nested+0x5a/0x1d0 kernel/locking/rtmutex_api.c:547
       wait_on_dquot fs/quota/dquot.c:357 [inline]
       dqget+0x73a/0xf20 fs/quota/dquot.c:975
       __dquot_initialize+0x3b3/0xcb0 fs/quota/dquot.c:1508
       ocfs2_get_init_inode+0x13b/0x1b0 fs/ocfs2/namei.c:205
       ocfs2_mknod+0x863/0x2050 fs/ocfs2/namei.c:313
       ocfs2_create+0x1a5/0x440 fs/ocfs2/namei.c:676
       lookup_open fs/namei.c:3796 [inline]
       open_last_lookups fs/namei.c:3895 [inline]
       path_openat+0x1500/0x3840 fs/namei.c:4131
       do_filp_open+0x1fa/0x410 fs/namei.c:4161
       do_sys_openat2+0x121/0x1c0 fs/open.c:1437
       do_sys_open fs/open.c:1452 [inline]
       __do_sys_openat fs/open.c:1468 [inline]
       __se_sys_openat fs/open.c:1463 [inline]
       __x64_sys_openat+0x138/0x170 fs/open.c:1463
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #1 (&ocfs2_sysfile_lock_key[INODE_ALLOC_SYSTEM_INODE]){+.+.}-{4:4}:

       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       down_write+0x3a/0x50 kernel/locking/rwsem.c:1590
       inode_lock include/linux/fs.h:980 [inline]
       ocfs2_remove_inode fs/ocfs2/inode.c:731 [inline]
       ocfs2_wipe_inode fs/ocfs2/inode.c:894 [inline]
       ocfs2_delete_inode fs/ocfs2/inode.c:1155 [inline]
       ocfs2_evict_inode+0x153d/0x40d0 fs/ocfs2/inode.c:1295
       evict+0x504/0x9c0 fs/inode.c:810
       do_unlinkat+0x39f/0x570 fs/namei.c:4744
       __do_sys_unlinkat fs/namei.c:4778 [inline]
       __se_sys_unlinkat fs/namei.c:4771 [inline]
       __x64_sys_unlinkat+0xd3/0xf0 fs/namei.c:4771
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #0 (&ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]){+.+.}-{4:4}:

       check_prev_add kernel/locking/lockdep.c:3165 [inline]
       check_prevs_add kernel/locking/lockdep.c:3284 [inline]
       validate_chain+0xb9b/0x2140 kernel/locking/lockdep.c:3908
       __lock_acquire+0xab9/0xd20 kernel/locking/lockdep.c:5237
       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       down_write+0x3a/0x50 kernel/locking/rwsem.c:1590
       inode_lock include/linux/fs.h:980 [inline]
       ocfs2_del_inode_from_orphan+0x134/0x740 fs/ocfs2/namei.c:2730
       ocfs2_dio_end_io_write fs/ocfs2/aops.c:2306 [inline]
       ocfs2_dio_end_io+0x47b/0x1100 fs/ocfs2/aops.c:2404
       dio_complete+0x25e/0x790 fs/direct-io.c:281
       __blockdev_direct_IO+0x2bc0/0x31f0 fs/direct-io.c:1303
       ocfs2_direct_IO+0x260/0x2d0 fs/ocfs2/aops.c:2441
       generic_file_direct_write+0x1dc/0x3e0 mm/filemap.c:4189
       __generic_file_write_iter+0x120/0x240 mm/filemap.c:4358
       ocfs2_file_write_iter+0x157d/0x1d20 fs/ocfs2/file.c:2469
       iter_file_splice_write+0x97a/0x10f0 fs/splice.c:738
       do_splice_from fs/splice.c:938 [inline]
       direct_splice_actor+0x104/0x160 fs/splice.c:1161
       splice_direct_to_actor+0x5b3/0xcd0 fs/splice.c:1105
       do_splice_direct_actor fs/splice.c:1204 [inline]
       do_splice_direct+0x187/0x270 fs/splice.c:1230
       do_sendfile+0x4ec/0x7f0 fs/read_write.c:1370
       __do_sys_sendfile64 fs/read_write.c:1431 [inline]
       __se_sys_sendfile64+0x13e/0x190 fs/read_write.c:1417
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

Fix this by acquiring &ocfs2_quota_ip_alloc_sem_key after acquiring &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE], effectively calling down_write(&oi->ip_alloc_sem) after the call to ocfs2_del_inode_from_orphan().

---
 fs/ocfs2/aops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/ocfs2/aops.c +2 -2
index 76c86f1c2b1c..586e3b74d782 100644
@@ -2295,8 +2295,6 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 		goto out;
 	}
 
-	down_write(&oi->ip_alloc_sem);
-
 	/* Delete orphan before acquire i_rwsem. */
 	if (dwc->dw_orphaned) {
 		BUG_ON(dwc->dw_writer_pid != task_pid_nr(current));
@@ -2309,6 +2307,8 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 			mlog_errno(ret);
 	}
 
+	down_write(&oi->ip_alloc_sem);
+
 	di = (struct ocfs2_dinode *)di_bh->b_data;
 
 	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh);

base-commit: 765e56e41a5af2d456ddda6cbd617b9d3295ab4e

author
#2 lore raw
Quoted 16 lines · Joseph Qi
Quoted 4 lines · Prithvi Tambewagh

A possible circular locking dependency in ocfs2_del_inode_from_orphan() is detected by syzbot, which occurs due to change in the sequence of acquiring locks. The existing chain is:

Quoted 8 lines

&ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE] --> &dquot->dq_lock --> &ocfs2_quota_ip_alloc_sem_key

In ocfs2_dio_end_io_write(), &ocfs2_quota_ip_alloc_sem_key is acquired, and then ocfs2_del_inode_from_orphan() is called, which acquires &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]. This opposes the existing dependency chain:

Quoted 6 lines

IIUC, ocfs2_dio_end_io_write() only gets the &ocfs2_file_ip_alloc_sem_key. So how it happens?

Thanks,
Joseph

IIUC that would be the case when ocfs2_dio_end_io_write() operates on normal files. However, according to the report of the bug, ocfs2_dio_end_io_write() is holding &ocfs2_quota_ip_alloc_sem_key so I think due to random fuzzing by syzkaller, the function is curently operating on a quota file.

Then it calls ocfs2_del_inode_from_orphan() which tries to acquire &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]. According to the existing dependency chain, after acquiring it, &ocfs2_quota_ip_alloc_sem_key can be acquired. But the existing code in ocfs2_dio_end_io_write() first acquires &ocfs2_quota_ip_alloc_sem_key, thus triggering the circular dependency warning.

Thanks,
Prithvi

Quoted 132 lines
Quoted 131 lines

-> #3 (&ocfs2_quota_ip_alloc_sem_key){++++}-{4:4}:

       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       down_write+0x3a/0x50 kernel/locking/rwsem.c:1590
       ocfs2_create_local_dquot+0x19d/0x1a40 fs/ocfs2/quota_local.c:1227
       ocfs2_acquire_dquot+0x80f/0xb30 fs/ocfs2/quota_global.c:883
       dqget+0x7c1/0xf20 fs/quota/dquot.c:980
       __dquot_initialize+0x3b3/0xcb0 fs/quota/dquot.c:1508
       ocfs2_get_init_inode+0x13b/0x1b0 fs/ocfs2/namei.c:205
       ocfs2_mknod+0x863/0x2050 fs/ocfs2/namei.c:313
       ocfs2_create+0x1a5/0x440 fs/ocfs2/namei.c:676
       lookup_open fs/namei.c:3796 [inline]
       open_last_lookups fs/namei.c:3895 [inline]
       path_openat+0x1500/0x3840 fs/namei.c:4131
       do_filp_open+0x1fa/0x410 fs/namei.c:4161
       do_sys_openat2+0x121/0x1c0 fs/open.c:1437
       do_sys_open fs/open.c:1452 [inline]
       __do_sys_openat fs/open.c:1468 [inline]
       __se_sys_openat fs/open.c:1463 [inline]
       __x64_sys_openat+0x138/0x170 fs/open.c:1463
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #2 (&dquot->dq_lock){+.+.}-{4:4}:

       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       __mutex_lock_common kernel/locking/rtmutex_api.c:535 [inline]
       mutex_lock_nested+0x5a/0x1d0 kernel/locking/rtmutex_api.c:547
       wait_on_dquot fs/quota/dquot.c:357 [inline]
       dqget+0x73a/0xf20 fs/quota/dquot.c:975
       __dquot_initialize+0x3b3/0xcb0 fs/quota/dquot.c:1508
       ocfs2_get_init_inode+0x13b/0x1b0 fs/ocfs2/namei.c:205
       ocfs2_mknod+0x863/0x2050 fs/ocfs2/namei.c:313
       ocfs2_create+0x1a5/0x440 fs/ocfs2/namei.c:676
       lookup_open fs/namei.c:3796 [inline]
       open_last_lookups fs/namei.c:3895 [inline]
       path_openat+0x1500/0x3840 fs/namei.c:4131
       do_filp_open+0x1fa/0x410 fs/namei.c:4161
       do_sys_openat2+0x121/0x1c0 fs/open.c:1437
       do_sys_open fs/open.c:1452 [inline]
       __do_sys_openat fs/open.c:1468 [inline]
       __se_sys_openat fs/open.c:1463 [inline]
       __x64_sys_openat+0x138/0x170 fs/open.c:1463
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #1 (&ocfs2_sysfile_lock_key[INODE_ALLOC_SYSTEM_INODE]){+.+.}-{4:4}:

       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       down_write+0x3a/0x50 kernel/locking/rwsem.c:1590
       inode_lock include/linux/fs.h:980 [inline]
       ocfs2_remove_inode fs/ocfs2/inode.c:731 [inline]
       ocfs2_wipe_inode fs/ocfs2/inode.c:894 [inline]
       ocfs2_delete_inode fs/ocfs2/inode.c:1155 [inline]
       ocfs2_evict_inode+0x153d/0x40d0 fs/ocfs2/inode.c:1295
       evict+0x504/0x9c0 fs/inode.c:810
       do_unlinkat+0x39f/0x570 fs/namei.c:4744
       __do_sys_unlinkat fs/namei.c:4778 [inline]
       __se_sys_unlinkat fs/namei.c:4771 [inline]
       __x64_sys_unlinkat+0xd3/0xf0 fs/namei.c:4771
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #0 (&ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]){+.+.}-{4:4}:

       check_prev_add kernel/locking/lockdep.c:3165 [inline]
       check_prevs_add kernel/locking/lockdep.c:3284 [inline]
       validate_chain+0xb9b/0x2140 kernel/locking/lockdep.c:3908
       __lock_acquire+0xab9/0xd20 kernel/locking/lockdep.c:5237
       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       down_write+0x3a/0x50 kernel/locking/rwsem.c:1590
       inode_lock include/linux/fs.h:980 [inline]
       ocfs2_del_inode_from_orphan+0x134/0x740 fs/ocfs2/namei.c:2730
       ocfs2_dio_end_io_write fs/ocfs2/aops.c:2306 [inline]
       ocfs2_dio_end_io+0x47b/0x1100 fs/ocfs2/aops.c:2404
       dio_complete+0x25e/0x790 fs/direct-io.c:281
       __blockdev_direct_IO+0x2bc0/0x31f0 fs/direct-io.c:1303
       ocfs2_direct_IO+0x260/0x2d0 fs/ocfs2/aops.c:2441
       generic_file_direct_write+0x1dc/0x3e0 mm/filemap.c:4189
       __generic_file_write_iter+0x120/0x240 mm/filemap.c:4358
       ocfs2_file_write_iter+0x157d/0x1d20 fs/ocfs2/file.c:2469
       iter_file_splice_write+0x97a/0x10f0 fs/splice.c:738
       do_splice_from fs/splice.c:938 [inline]
       direct_splice_actor+0x104/0x160 fs/splice.c:1161
       splice_direct_to_actor+0x5b3/0xcd0 fs/splice.c:1105
       do_splice_direct_actor fs/splice.c:1204 [inline]
       do_splice_direct+0x187/0x270 fs/splice.c:1230
       do_sendfile+0x4ec/0x7f0 fs/read_write.c:1370
       __do_sys_sendfile64 fs/read_write.c:1431 [inline]
       __se_sys_sendfile64+0x13e/0x190 fs/read_write.c:1417
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

Fix this by acquiring &ocfs2_quota_ip_alloc_sem_key after acquiring &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE], effectively calling down_write(&oi->ip_alloc_sem) after the call to ocfs2_del_inode_from_orphan().

---
 fs/ocfs2/aops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/ocfs2/aops.c +2 -2
index 76c86f1c2b1c..586e3b74d782 100644
@@ -2295,8 +2295,6 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 		goto out;
 	}
 
-	down_write(&oi->ip_alloc_sem);
-
 	/* Delete orphan before acquire i_rwsem. */
 	if (dwc->dw_orphaned) {
 		BUG_ON(dwc->dw_writer_pid != task_pid_nr(current));
@@ -2309,6 +2307,8 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 			mlog_errno(ret);
 	}
 
+	down_write(&oi->ip_alloc_sem);
+
 	di = (struct ocfs2_dinode *)di_bh->b_data;
 
 	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh);

base-commit: 765e56e41a5af2d456ddda6cbd617b9d3295ab4e

author
#3 lore raw
Quoted 36 lines · Prithvi
Quoted 16 lines · Joseph Qi
Quoted 4 lines · Prithvi Tambewagh

A possible circular locking dependency in ocfs2_del_inode_from_orphan() is detected by syzbot, which occurs due to change in the sequence of acquiring locks. The existing chain is:

Quoted 8 lines

&ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE] --> &dquot->dq_lock --> &ocfs2_quota_ip_alloc_sem_key

In ocfs2_dio_end_io_write(), &ocfs2_quota_ip_alloc_sem_key is acquired, and then ocfs2_del_inode_from_orphan() is called, which acquires &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]. This opposes the existing dependency chain:

Quoted 6 lines

IIUC, ocfs2_dio_end_io_write() only gets the &ocfs2_file_ip_alloc_sem_key. So how it happens?

Thanks,
Joseph

IIUC that would be the case when ocfs2_dio_end_io_write() operates on normal files. However, according to the report of the bug, ocfs2_dio_end_io_write() is holding &ocfs2_quota_ip_alloc_sem_key so I think due to random fuzzing by syzkaller, the function is curently operating on a quota file.

Then it calls ocfs2_del_inode_from_orphan() which tries to acquire &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]. According to the existing dependency chain, after acquiring it, &ocfs2_quota_ip_alloc_sem_key can be acquired. But the existing code in ocfs2_dio_end_io_write() first acquires &ocfs2_quota_ip_alloc_sem_key, thus triggering the circular dependency warning.

Quoted 135 lines

Thanks,
Prithvi

Quoted 132 lines
Quoted 131 lines

-> #3 (&ocfs2_quota_ip_alloc_sem_key){++++}-{4:4}:

       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       down_write+0x3a/0x50 kernel/locking/rwsem.c:1590
       ocfs2_create_local_dquot+0x19d/0x1a40 fs/ocfs2/quota_local.c:1227
       ocfs2_acquire_dquot+0x80f/0xb30 fs/ocfs2/quota_global.c:883
       dqget+0x7c1/0xf20 fs/quota/dquot.c:980
       __dquot_initialize+0x3b3/0xcb0 fs/quota/dquot.c:1508
       ocfs2_get_init_inode+0x13b/0x1b0 fs/ocfs2/namei.c:205
       ocfs2_mknod+0x863/0x2050 fs/ocfs2/namei.c:313
       ocfs2_create+0x1a5/0x440 fs/ocfs2/namei.c:676
       lookup_open fs/namei.c:3796 [inline]
       open_last_lookups fs/namei.c:3895 [inline]
       path_openat+0x1500/0x3840 fs/namei.c:4131
       do_filp_open+0x1fa/0x410 fs/namei.c:4161
       do_sys_openat2+0x121/0x1c0 fs/open.c:1437
       do_sys_open fs/open.c:1452 [inline]
       __do_sys_openat fs/open.c:1468 [inline]
       __se_sys_openat fs/open.c:1463 [inline]
       __x64_sys_openat+0x138/0x170 fs/open.c:1463
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #2 (&dquot->dq_lock){+.+.}-{4:4}:

       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       __mutex_lock_common kernel/locking/rtmutex_api.c:535 [inline]
       mutex_lock_nested+0x5a/0x1d0 kernel/locking/rtmutex_api.c:547
       wait_on_dquot fs/quota/dquot.c:357 [inline]
       dqget+0x73a/0xf20 fs/quota/dquot.c:975
       __dquot_initialize+0x3b3/0xcb0 fs/quota/dquot.c:1508
       ocfs2_get_init_inode+0x13b/0x1b0 fs/ocfs2/namei.c:205
       ocfs2_mknod+0x863/0x2050 fs/ocfs2/namei.c:313
       ocfs2_create+0x1a5/0x440 fs/ocfs2/namei.c:676
       lookup_open fs/namei.c:3796 [inline]
       open_last_lookups fs/namei.c:3895 [inline]
       path_openat+0x1500/0x3840 fs/namei.c:4131
       do_filp_open+0x1fa/0x410 fs/namei.c:4161
       do_sys_openat2+0x121/0x1c0 fs/open.c:1437
       do_sys_open fs/open.c:1452 [inline]
       __do_sys_openat fs/open.c:1468 [inline]
       __se_sys_openat fs/open.c:1463 [inline]
       __x64_sys_openat+0x138/0x170 fs/open.c:1463
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #1 (&ocfs2_sysfile_lock_key[INODE_ALLOC_SYSTEM_INODE]){+.+.}-{4:4}:

       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       down_write+0x3a/0x50 kernel/locking/rwsem.c:1590
       inode_lock include/linux/fs.h:980 [inline]
       ocfs2_remove_inode fs/ocfs2/inode.c:731 [inline]
       ocfs2_wipe_inode fs/ocfs2/inode.c:894 [inline]
       ocfs2_delete_inode fs/ocfs2/inode.c:1155 [inline]
       ocfs2_evict_inode+0x153d/0x40d0 fs/ocfs2/inode.c:1295
       evict+0x504/0x9c0 fs/inode.c:810
       do_unlinkat+0x39f/0x570 fs/namei.c:4744
       __do_sys_unlinkat fs/namei.c:4778 [inline]
       __se_sys_unlinkat fs/namei.c:4771 [inline]
       __x64_sys_unlinkat+0xd3/0xf0 fs/namei.c:4771
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #0 (&ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]){+.+.}-{4:4}:

       check_prev_add kernel/locking/lockdep.c:3165 [inline]
       check_prevs_add kernel/locking/lockdep.c:3284 [inline]
       validate_chain+0xb9b/0x2140 kernel/locking/lockdep.c:3908
       __lock_acquire+0xab9/0xd20 kernel/locking/lockdep.c:5237
       lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5868
       down_write+0x3a/0x50 kernel/locking/rwsem.c:1590
       inode_lock include/linux/fs.h:980 [inline]
       ocfs2_del_inode_from_orphan+0x134/0x740 fs/ocfs2/namei.c:2730
       ocfs2_dio_end_io_write fs/ocfs2/aops.c:2306 [inline]
       ocfs2_dio_end_io+0x47b/0x1100 fs/ocfs2/aops.c:2404
       dio_complete+0x25e/0x790 fs/direct-io.c:281
       __blockdev_direct_IO+0x2bc0/0x31f0 fs/direct-io.c:1303
       ocfs2_direct_IO+0x260/0x2d0 fs/ocfs2/aops.c:2441
       generic_file_direct_write+0x1dc/0x3e0 mm/filemap.c:4189
       __generic_file_write_iter+0x120/0x240 mm/filemap.c:4358
       ocfs2_file_write_iter+0x157d/0x1d20 fs/ocfs2/file.c:2469
       iter_file_splice_write+0x97a/0x10f0 fs/splice.c:738
       do_splice_from fs/splice.c:938 [inline]
       direct_splice_actor+0x104/0x160 fs/splice.c:1161
       splice_direct_to_actor+0x5b3/0xcd0 fs/splice.c:1105
       do_splice_direct_actor fs/splice.c:1204 [inline]
       do_splice_direct+0x187/0x270 fs/splice.c:1230
       do_sendfile+0x4ec/0x7f0 fs/read_write.c:1370
       __do_sys_sendfile64 fs/read_write.c:1431 [inline]
       __se_sys_sendfile64+0x13e/0x190 fs/read_write.c:1417
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

Fix this by acquiring &ocfs2_quota_ip_alloc_sem_key after acquiring &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE], effectively calling down_write(&oi->ip_alloc_sem) after the call to ocfs2_del_inode_from_orphan().

---
 fs/ocfs2/aops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/ocfs2/aops.c +2 -2
index 76c86f1c2b1c..586e3b74d782 100644
@@ -2295,8 +2295,6 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 		goto out;
 	}
 
-	down_write(&oi->ip_alloc_sem);
-
 	/* Delete orphan before acquire i_rwsem. */
 	if (dwc->dw_orphaned) {
 		BUG_ON(dwc->dw_writer_pid != task_pid_nr(current));
@@ -2309,6 +2307,8 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 			mlog_errno(ret);
 	}
 
+	down_write(&oi->ip_alloc_sem);
+
 	di = (struct ocfs2_dinode *)di_bh->b_data;
 
 	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh);

base-commit: 765e56e41a5af2d456ddda6cbd617b9d3295ab4e

Hello all,

Just a gentle ping on this thread.

Thanks,
Prithvi

#4 lore raw
Quoted 5 lines · Prithvi
Quoted 4 lines · Prithvi

IIUC that would be the case when ocfs2_dio_end_io_write() operates on normal files. However, according to the report of the bug, ocfs2_dio_end_io_write() is holding &ocfs2_quota_ip_alloc_sem_key so I think due to random fuzzing by syzkaller, the function is curently operating on a quota file.

So the right fix is probably to disallow DIO to a quota file, wouldn't you say?

author
#5 lore raw
Quoted 9 lines · Matthew Wilcox
Quoted 5 lines · Prithvi
Quoted 4 lines · Prithvi

IIUC that would be the case when ocfs2_dio_end_io_write() operates on normal files. However, according to the report of the bug, ocfs2_dio_end_io_write() is holding &ocfs2_quota_ip_alloc_sem_key so I think due to random fuzzing by syzkaller, the function is curently operating on a quota file.

So the right fix is probably to disallow DIO to a quota file, wouldn't you say?

Hello Matthew,

Thanks for the feedback! I agree that disallowing DIO on quota files can probably be the fix for this bug. I will work in this direction. I think Joseph was indicating earlier that DIO doesn't act on quota files, but I misinterpreted his feedback...apologies for that.

Thanks,
Prithvi

author
#6 lore raw
In reply to Matthew Wilcox
Quoted 9 lines · Matthew Wilcox
Quoted 5 lines · Prithvi
Quoted 4 lines · Prithvi

IIUC that would be the case when ocfs2_dio_end_io_write() operates on normal files. However, according to the report of the bug, ocfs2_dio_end_io_write() is holding &ocfs2_quota_ip_alloc_sem_key so I think due to random fuzzing by syzkaller, the function is curently operating on a quota file.

So the right fix is probably to disallow DIO to a quota file, wouldn't you say?

Hello all,

Firstly I apologize for the delay...I was trying to understand the bug in detail and few strange things I noticed. I have now confirmed the bug is due to uninitialized locks during slab reuse. Here are the deatils:

According to earlier observations, we knew that a file with a quota lock was trying to enter DIO path which is not permitted. I observed that that in spite of adding checks like IS_NOQUOTA(inode) since I observed:

else if (fe->i_flags & cpu_to_le32(OCFS2_QUOTA_FL)) {
        inode->i_flags |= S_NOQUOTA;
}

in ocfs2_populate_inode(). However with this change also the issue didn't seem to get resolved.

I found that it was some inode numbered 16979 which was entering DIO. However, upon investigation I noticed that inode 16979 passed through ocfs2_populate_inode() only when create_ino = 1. Upon adding a few printk statements in __ocfs2_mknod_locked() I found that the inode 16979 strangely held ocfs2_quota_ip_allc_sem lock at the mknod stage. I suspected it to be a lock cleanup issue in the slab object when inodes are returned to the slab.

I found that in one mount session, an inode was mounted as a quota file, with a very small number (mostly <50) then later on in another mount session that same slab object is reused for the inode with number 16979 which immediately causes the lockdep warning to occur since it still holds the quota_ip_alloc_sem lock.

IIUC, I did not find any way the semaphore is cleaned when an inode is evicted. Hence I think to add init_rwsem() to the ocfs2_clear_inode() to stop the warning, since now that inode held ip_alloc_sem lock only, thus no quota lock acquired during DIO and the issue never occurs, even if same slab object is used which retains the lock identity. In addition, a check can be added to block DIO on quota files.

Thus, I think the following change should work:

fs/ocfs2/aops.c +7 -1
index 76c86f1c2b1c..372c198afa25 100644
@@ -2420,7 +2420,13 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
        struct inode *inode = file->f_mapping->host;
        struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
        get_block_t *get_block;
-
+
+       if(IS_NOQUOTA(inode)) {
+               mlog(ML_ERROR, "Direct IO is not allowed for Quota inode %lu\n",
+                    inode->i_ino);
+               return -ENOTSUP;
+       }
+
        /*
         * Fallback to buffered I/O if we see an inode without
         * extents.
fs/ocfs2/inode.c +2 -1
index b5fcc2725a29..3e36da2ca5d0 100644
@@ -1268,7 +1268,8 @@ static void ocfs2_clear_inode(struct inode *inode)
                        "Clear inode of %llu, alloc_sem is locked\n",
                        (unsigned long long)oi->ip_blkno);
        up_write(&oi->ip_alloc_sem);
-
+       init_rwsem(&oi->ip_alloc_sem);
+
        mlog_bug_on_msg(oi->ip_open_count,
                        "Clear inode of %llu has open count %d\n",
                        (unsigned long long)oi->ip_blkno, oi->ip_open_count);

What do you think?

Thank you and best regards,
Prithvi

author
#7 lore raw
Quoted 84 lines · Prithvi
Quoted 9 lines · Matthew Wilcox
Quoted 5 lines · Prithvi
Quoted 4 lines · Prithvi

IIUC that would be the case when ocfs2_dio_end_io_write() operates on normal files. However, according to the report of the bug, ocfs2_dio_end_io_write() is holding &ocfs2_quota_ip_alloc_sem_key so I think due to random fuzzing by syzkaller, the function is curently operating on a quota file.

So the right fix is probably to disallow DIO to a quota file, wouldn't you say?

Hello all,

Firstly I apologize for the delay...I was trying to understand the bug in detail and few strange things I noticed. I have now confirmed the bug is due to uninitialized locks during slab reuse. Here are the deatils:

According to earlier observations, we knew that a file with a quota lock was trying to enter DIO path which is not permitted. I observed that that in spite of adding checks like IS_NOQUOTA(inode) since I observed:

else if (fe->i_flags & cpu_to_le32(OCFS2_QUOTA_FL)) {
        inode->i_flags |= S_NOQUOTA;
}

in ocfs2_populate_inode(). However with this change also the issue didn't seem to get resolved.

I found that it was some inode numbered 16979 which was entering DIO. However, upon investigation I noticed that inode 16979 passed through ocfs2_populate_inode() only when create_ino = 1. Upon adding a few printk statements in __ocfs2_mknod_locked() I found that the inode 16979 strangely held ocfs2_quota_ip_allc_sem lock at the mknod stage. I suspected it to be a lock cleanup issue in the slab object when inodes are returned to the slab.

I found that in one mount session, an inode was mounted as a quota file, with a very small number (mostly <50) then later on in another mount session that same slab object is reused for the inode with number 16979 which immediately causes the lockdep warning to occur since it still holds the quota_ip_alloc_sem lock.

IIUC, I did not find any way the semaphore is cleaned when an inode is evicted. Hence I think to add init_rwsem() to the ocfs2_clear_inode() to stop the warning, since now that inode held ip_alloc_sem lock only, thus no quota lock acquired during DIO and the issue never occurs, even if same slab object is used which retains the lock identity. In addition, a check can be added to block DIO on quota files.

Thus, I think the following change should work:

fs/ocfs2/aops.c +7 -1
index 76c86f1c2b1c..372c198afa25 100644
@@ -2420,7 +2420,13 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
        struct inode *inode = file->f_mapping->host;
        struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
        get_block_t *get_block;
-
+
+       if(IS_NOQUOTA(inode)) {
+               mlog(ML_ERROR, "Direct IO is not allowed for Quota inode %lu\n",
+                    inode->i_ino);
+               return -ENOTSUP;
+       }
+
        /*
         * Fallback to buffered I/O if we see an inode without
         * extents.
fs/ocfs2/inode.c +2 -1
index b5fcc2725a29..3e36da2ca5d0 100644
@@ -1268,7 +1268,8 @@ static void ocfs2_clear_inode(struct inode *inode)
                        "Clear inode of %llu, alloc_sem is locked\n",
                        (unsigned long long)oi->ip_blkno);
        up_write(&oi->ip_alloc_sem);
-
+       init_rwsem(&oi->ip_alloc_sem);
+
        mlog_bug_on_msg(oi->ip_open_count,
                        "Clear inode of %llu has open count %d\n",
                        (unsigned long long)oi->ip_blkno, oi->ip_open_count);
Quoted 4 lines

What do you think?

Thank you and best regards,
Prithvi

Hello everyone,

Just a gentle ping on this thread...I wanted to seek feedback regarding the analysis of uninitialized locks during slab reuse. If the proposed fix is correct, I can go ahead and send a v2 patch for the same.

In addition, I also realised using -EPERM in the if condition in ocfs2_direct_IO() might be more relevant so I tested a patch with that change and it doesn't trigger any issue with the testing with reproducer program as tested here :
https://lore.kernel.org/all/[email protected]/T/#m00f39888c9cfdb78b2c5eff8d1e2cba82e4fd9cc

Thanks,
Prithvi

reviewer
#8 lore raw
Quoted 96 lines · Prithvi
Quoted 84 lines · Prithvi
Quoted 9 lines · Matthew Wilcox
Quoted 5 lines · Prithvi
Quoted 4 lines · Prithvi

IIUC that would be the case when ocfs2_dio_end_io_write() operates on normal files. However, according to the report of the bug, ocfs2_dio_end_io_write() is holding &ocfs2_quota_ip_alloc_sem_key so I think due to random fuzzing by syzkaller, the function is curently operating on a quota file.

So the right fix is probably to disallow DIO to a quota file, wouldn't you say?

Hello all,

Firstly I apologize for the delay...I was trying to understand the bug in detail and few strange things I noticed. I have now confirmed the bug is due to uninitialized locks during slab reuse. Here are the deatils:

According to earlier observations, we knew that a file with a quota lock was trying to enter DIO path which is not permitted. I observed that that in spite of adding checks like IS_NOQUOTA(inode) since I observed:

else if (fe->i_flags & cpu_to_le32(OCFS2_QUOTA_FL)) {
        inode->i_flags |= S_NOQUOTA;
}

in ocfs2_populate_inode(). However with this change also the issue didn't seem to get resolved.

I found that it was some inode numbered 16979 which was entering DIO. However, upon investigation I noticed that inode 16979 passed through ocfs2_populate_inode() only when create_ino = 1. Upon adding a few printk statements in __ocfs2_mknod_locked() I found that the inode 16979 strangely held ocfs2_quota_ip_allc_sem lock at the mknod stage. I suspected it to be a lock cleanup issue in the slab object when inodes are returned to the slab.

I found that in one mount session, an inode was mounted as a quota file, with a very small number (mostly <50) then later on in another mount session that same slab object is reused for the inode with number 16979 which immediately causes the lockdep warning to occur since it still holds the quota_ip_alloc_sem lock.

IIUC, I did not find any way the semaphore is cleaned when an inode is evicted. Hence I think to add init_rwsem() to the ocfs2_clear_inode() to stop the warning, since now that inode held ip_alloc_sem lock only, thus no quota lock acquired during DIO and the issue never occurs, even if same slab object is used which retains the lock identity. In addition, a check can be added to block DIO on quota files.

Thus, I think the following change should work:

fs/ocfs2/aops.c +7 -1
index 76c86f1c2b1c..372c198afa25 100644
@@ -2420,7 +2420,13 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
        struct inode *inode = file->f_mapping->host;
        struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
        get_block_t *get_block;
-
+
+       if(IS_NOQUOTA(inode)) {
+               mlog(ML_ERROR, "Direct IO is not allowed for Quota inode %lu\n",
+                    inode->i_ino);
+               return -ENOTSUP;
+       }
+
        /*
         * Fallback to buffered I/O if we see an inode without
         * extents.
fs/ocfs2/inode.c +2 -1
index b5fcc2725a29..3e36da2ca5d0 100644
@@ -1268,7 +1268,8 @@ static void ocfs2_clear_inode(struct inode *inode)
                        "Clear inode of %llu, alloc_sem is locked\n",
                        (unsigned long long)oi->ip_blkno);
        up_write(&oi->ip_alloc_sem);
-
+       init_rwsem(&oi->ip_alloc_sem);
+
        mlog_bug_on_msg(oi->ip_open_count,
                        "Clear inode of %llu has open count %d\n",
                        (unsigned long long)oi->ip_blkno, oi->ip_open_count);
Quoted 4 lines

What do you think?

Thank you and best regards,
Prithvi

Hello everyone,

Just a gentle ping on this thread...I wanted to seek feedback regarding the analysis of uninitialized locks during slab reuse. If the proposed fix is correct, I can go ahead and send a v2 patch for the same.

Quoted 8 lines

In addition, I also realised using -EPERM in the if condition in ocfs2_direct_IO() might be more relevant so I tested a patch with that change and it doesn't trigger any issue with the testing with reproducer program as tested here :
https://lore.kernel.org/all/[email protected]/T/#m00f39888c9cfdb78b2c5eff8d1e2cba82e4fd9cc

Thanks,
Prithvi

Hi Prithvi,

This patch disables DIO on quota files, which will lead to a performance regression. Other filesystems (such as ext4 and gfs2) do not forbid DIO on quota files. Therefore, I NAK this patch.

I have sent an alternative patch[1] to address this issue. Please take a look.

[1]:
https://lore.kernel.org/ocfs2-devel/[email protected]/T/#u

Thanks,
Heming

author
#9 lore raw
Quoted 112 lines · Heming Zhao
Quoted 96 lines · Prithvi
Quoted 84 lines · Prithvi
Quoted 9 lines · Matthew Wilcox
Quoted 5 lines · Prithvi
Quoted 4 lines · Prithvi

IIUC that would be the case when ocfs2_dio_end_io_write() operates on normal files. However, according to the report of the bug, ocfs2_dio_end_io_write() is holding &ocfs2_quota_ip_alloc_sem_key so I think due to random fuzzing by syzkaller, the function is curently operating on a quota file.

So the right fix is probably to disallow DIO to a quota file, wouldn't you say?

Hello all,

Firstly I apologize for the delay...I was trying to understand the bug in detail and few strange things I noticed. I have now confirmed the bug is due to uninitialized locks during slab reuse. Here are the deatils:

According to earlier observations, we knew that a file with a quota lock was trying to enter DIO path which is not permitted. I observed that that in spite of adding checks like IS_NOQUOTA(inode) since I observed:

else if (fe->i_flags & cpu_to_le32(OCFS2_QUOTA_FL)) {
        inode->i_flags |= S_NOQUOTA;
}

in ocfs2_populate_inode(). However with this change also the issue didn't seem to get resolved.

I found that it was some inode numbered 16979 which was entering DIO. However, upon investigation I noticed that inode 16979 passed through ocfs2_populate_inode() only when create_ino = 1. Upon adding a few printk statements in __ocfs2_mknod_locked() I found that the inode 16979 strangely held ocfs2_quota_ip_allc_sem lock at the mknod stage. I suspected it to be a lock cleanup issue in the slab object when inodes are returned to the slab.

I found that in one mount session, an inode was mounted as a quota file, with a very small number (mostly <50) then later on in another mount session that same slab object is reused for the inode with number 16979 which immediately causes the lockdep warning to occur since it still holds the quota_ip_alloc_sem lock.

IIUC, I did not find any way the semaphore is cleaned when an inode is evicted. Hence I think to add init_rwsem() to the ocfs2_clear_inode() to stop the warning, since now that inode held ip_alloc_sem lock only, thus no quota lock acquired during DIO and the issue never occurs, even if same slab object is used which retains the lock identity. In addition, a check can be added to block DIO on quota files.

Thus, I think the following change should work:

fs/ocfs2/aops.c +7 -1
index 76c86f1c2b1c..372c198afa25 100644
@@ -2420,7 +2420,13 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
        struct inode *inode = file->f_mapping->host;
        struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
        get_block_t *get_block;
-
+
+       if(IS_NOQUOTA(inode)) {
+               mlog(ML_ERROR, "Direct IO is not allowed for Quota inode %lu\n",
+                    inode->i_ino);
+               return -ENOTSUP;
+       }
+
        /*
         * Fallback to buffered I/O if we see an inode without
         * extents.
fs/ocfs2/inode.c +2 -1
index b5fcc2725a29..3e36da2ca5d0 100644
@@ -1268,7 +1268,8 @@ static void ocfs2_clear_inode(struct inode *inode)
                        "Clear inode of %llu, alloc_sem is locked\n",
                        (unsigned long long)oi->ip_blkno);
        up_write(&oi->ip_alloc_sem);
-
+       init_rwsem(&oi->ip_alloc_sem);
+
        mlog_bug_on_msg(oi->ip_open_count,
                        "Clear inode of %llu has open count %d\n",
                        (unsigned long long)oi->ip_blkno, oi->ip_open_count);
Quoted 4 lines

What do you think?

Thank you and best regards,
Prithvi

Hello everyone,

Just a gentle ping on this thread...I wanted to seek feedback regarding the analysis of uninitialized locks during slab reuse. If the proposed fix is correct, I can go ahead and send a v2 patch for the same.

Quoted 8 lines

In addition, I also realised using -EPERM in the if condition in ocfs2_direct_IO() might be more relevant so I tested a patch with that change and it doesn't trigger any issue with the testing with reproducer program as tested here :
https://lore.kernel.org/all/[email protected]/T/#m00f39888c9cfdb78b2c5eff8d1e2cba82e4fd9cc

Thanks,
Prithvi

Hi Prithvi,

This patch disables DIO on quota files, which will lead to a performance regression. Other filesystems (such as ext4 and gfs2) do not forbid DIO on quota files. Therefore, I NAK this patch.

Quoted 7 lines

I have sent an alternative patch[1] to address this issue. Please take a look.

[1]:
https://lore.kernel.org/ocfs2-devel/[email protected]/T/#u

Thanks,
Heming

Hello Heming,

Thanks for your feedback! I see your point about performance regression and agree that your approach using down_write_trylock is a much more robust way to fix this issue.

Thanks,
Prithvi