ocfs2: Fix circular locking dependency in ocfs2_del_inode_from_orphan()
Subsystems (2)
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/0x7fFix 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
@@ -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
Joseph Qi
maintainerQuoted 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_keyIn
ocfs2_dio_end_io_write(),&ocfs2_quota_ip_alloc_sem_keyis acquired, and thenocfs2_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/0x7fFix this by acquiring
&ocfs2_quota_ip_alloc_sem_keyafter acquiring &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE], effectively callingdown_write(&oi->ip_alloc_sem)after the call toocfs2_del_inode_from_orphan().Reported-by [email protected]Tested-by [email protected]Signed-off-by Prithvi Tambewagh <[email protected]>--- fs/ocfs2/aops.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)fs/ocfs2/aops.c +2
-2@@ -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
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_keyIn
ocfs2_dio_end_io_write(),&ocfs2_quota_ip_alloc_sem_keyis acquired, and thenocfs2_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/0x7fFix this by acquiring
&ocfs2_quota_ip_alloc_sem_keyafter acquiring &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE], effectively callingdown_write(&oi->ip_alloc_sem)after the call toocfs2_del_inode_from_orphan().Reported-by [email protected]Tested-by [email protected]Signed-off-by Prithvi Tambewagh <[email protected]>--- fs/ocfs2/aops.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)fs/ocfs2/aops.c +2
-2@@ -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
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_keyIn
ocfs2_dio_end_io_write(),&ocfs2_quota_ip_alloc_sem_keyis acquired, and thenocfs2_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,
JosephIIUC 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_keyso 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_keycan be acquired. But the existing code inocfs2_dio_end_io_write()first acquires&ocfs2_quota_ip_alloc_sem_key, thus triggering the circular dependency warning.
Quoted 135 lines
Thanks,
PrithviQuoted 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/0x7fFix this by acquiring
&ocfs2_quota_ip_alloc_sem_keyafter acquiring &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE], effectively callingdown_write(&oi->ip_alloc_sem)after the call toocfs2_del_inode_from_orphan().Reported-by [email protected]Tested-by [email protected]Signed-off-by Prithvi Tambewagh <[email protected]>--- fs/ocfs2/aops.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)fs/ocfs2/aops.c +2
-2@@ -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
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_keyso 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?
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_keyso 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
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_keyso 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
@@ -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
@@ -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
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_keyso 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 whencreate_ino= 1. Upon adding a few printk statements in__ocfs2_mknod_locked()I found that the inode 16979 strangely heldocfs2_quota_ip_allc_semlock 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_semlock.IIUC, I did not find any way the semaphore is cleaned when an inode is evicted. Hence I think to add
init_rwsem()to theocfs2_clear_inode()to stop the warning, since now that inode heldip_alloc_semlock 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@@ -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@@ -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
Heming Zhao
reviewerQuoted 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_keyso 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 whencreate_ino= 1. Upon adding a few printk statements in__ocfs2_mknod_locked()I found that the inode 16979 strangely heldocfs2_quota_ip_allc_semlock 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_semlock.IIUC, I did not find any way the semaphore is cleaned when an inode is evicted. Hence I think to add
init_rwsem()to theocfs2_clear_inode()to stop the warning, since now that inode heldip_alloc_semlock 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@@ -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@@ -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,
PrithviHello 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
-EPERMin the if condition inocfs2_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/#m00f39888c9cfdb78b2c5eff8d1e2cba82e4fd9ccThanks,
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
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_keyso 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 whencreate_ino= 1. Upon adding a few printk statements in__ocfs2_mknod_locked()I found that the inode 16979 strangely heldocfs2_quota_ip_allc_semlock 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_semlock.IIUC, I did not find any way the semaphore is cleaned when an inode is evicted. Hence I think to add
init_rwsem()to theocfs2_clear_inode()to stop the warning, since now that inode heldip_alloc_semlock 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@@ -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@@ -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,
PrithviHello 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
-EPERMin the if condition inocfs2_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/#m00f39888c9cfdb78b2c5eff8d1e2cba82e4fd9ccThanks,
PrithviHi 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/#uThanks,
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