All of lore.kernel.org
 help / color / mirror / Atom feed
* [syzbot] [reiserfs?] possible deadlock in open_xa_dir
@ 2022-12-31  6:35 syzbot
  2023-05-05  7:10 ` syzbot
                   ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: syzbot @ 2022-12-31  6:35 UTC (permalink / raw)
  To: linux-kernel, reiserfs-devel, syzkaller-bugs

Hello,

syzbot found the following issue on:

HEAD commit:    1b929c02afd3 Linux 6.2-rc1
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11da7f8c480000
kernel config:  https://syzkaller.appspot.com/x/.config?x=2651619a26b4d687
dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/d562ace1a56c/disk-1b929c02.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/39f000fe6b9e/vmlinux-1b929c02.xz
kernel image: https://storage.googleapis.com/syzbot-assets/1c67e48de5a0/bzImage-1b929c02.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com

======================================================
WARNING: possible circular locking dependency detected
6.2.0-rc1-syzkaller #0 Not tainted
------------------------------------------------------
syz-executor.1/19784 is trying to acquire lock:
ffff88807d682090 (&sbi->lock){+.+.}-{3:3}, at: reiserfs_write_lock+0x79/0x100 fs/reiserfs/lock.c:27

but task is already holding lock:
ffff8880899d6ce0 (&type->i_mutex_dir_key#9/3){+.+.}-{3:3}, at: inode_lock_nested include/linux/fs.h:791 [inline]
ffff8880899d6ce0 (&type->i_mutex_dir_key#9/3){+.+.}-{3:3}, at: open_xa_root fs/reiserfs/xattr.c:127 [inline]
ffff8880899d6ce0 (&type->i_mutex_dir_key#9/3){+.+.}-{3:3}, at: open_xa_dir+0x127/0x830 fs/reiserfs/xattr.c:152

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #1 (&type->i_mutex_dir_key#9/3){+.+.}-{3:3}:
       down_write_nested+0x98/0x220 kernel/locking/rwsem.c:1672
       inode_lock_nested include/linux/fs.h:791 [inline]
       open_xa_root fs/reiserfs/xattr.c:127 [inline]
       open_xa_dir+0x127/0x830 fs/reiserfs/xattr.c:152
       reiserfs_for_each_xattr+0x1ab/0x9a0 fs/reiserfs/xattr.c:252
       reiserfs_delete_xattrs+0x20/0xa0 fs/reiserfs/xattr.c:364
       reiserfs_evict_inode+0x2e7/0x540 fs/reiserfs/inode.c:53
       evict+0x2ed/0x6b0 fs/inode.c:664
       iput_final fs/inode.c:1747 [inline]
       iput.part.0+0x59b/0x880 fs/inode.c:1773
       iput+0x5c/0x80 fs/inode.c:1763
       reiserfs_create+0x65a/0x730 fs/reiserfs/namei.c:688
       vfs_create fs/namei.c:3115 [inline]
       vfs_create+0x3ed/0x670 fs/namei.c:3101
       do_mknodat+0x3c4/0x510 fs/namei.c:3965
       __do_sys_mknod fs/namei.c:3998 [inline]
       __se_sys_mknod fs/namei.c:3996 [inline]
       __x64_sys_mknod+0x11e/0x180 fs/namei.c:3996
       do_syscall_x64 arch/x86/entry/common.c:50 [inline]
       do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
       entry_SYSCALL_64_after_hwframe+0x63/0xcd

-> #0 (&sbi->lock){+.+.}-{3:3}:
       check_prev_add kernel/locking/lockdep.c:3097 [inline]
       check_prevs_add kernel/locking/lockdep.c:3216 [inline]
       validate_chain kernel/locking/lockdep.c:3831 [inline]
       __lock_acquire+0x2a43/0x56d0 kernel/locking/lockdep.c:5055
       lock_acquire kernel/locking/lockdep.c:5668 [inline]
       lock_acquire+0x1e3/0x630 kernel/locking/lockdep.c:5633
       __mutex_lock_common kernel/locking/mutex.c:603 [inline]
       __mutex_lock+0x12f/0x1360 kernel/locking/mutex.c:747
       reiserfs_write_lock+0x79/0x100 fs/reiserfs/lock.c:27
       reiserfs_mkdir+0x31c/0x990 fs/reiserfs/namei.c:831
       xattr_mkdir fs/reiserfs/xattr.c:76 [inline]
       open_xa_root fs/reiserfs/xattr.c:136 [inline]
       open_xa_dir+0x6a3/0x830 fs/reiserfs/xattr.c:152
       xattr_lookup+0x21/0x3d0 fs/reiserfs/xattr.c:395
       reiserfs_xattr_set_handle+0xfb/0xb00 fs/reiserfs/xattr.c:533
       reiserfs_xattr_set+0x454/0x5b0 fs/reiserfs/xattr.c:633
       trusted_set+0xa7/0xd0 fs/reiserfs/xattr_trusted.c:31
       __vfs_setxattr+0x173/0x1e0 fs/xattr.c:202
       __vfs_setxattr_noperm+0x129/0x5f0 fs/xattr.c:236
       __vfs_setxattr_locked+0x1d3/0x260 fs/xattr.c:297
       vfs_setxattr+0x143/0x340 fs/xattr.c:323
       do_setxattr+0x151/0x190 fs/xattr.c:608
       setxattr+0x146/0x160 fs/xattr.c:631
       path_setxattr+0x197/0x1c0 fs/xattr.c:650
       __do_sys_setxattr fs/xattr.c:666 [inline]
       __se_sys_setxattr fs/xattr.c:662 [inline]
       __x64_sys_setxattr+0xc4/0x160 fs/xattr.c:662
       do_syscall_x64 arch/x86/entry/common.c:50 [inline]
       do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
       entry_SYSCALL_64_after_hwframe+0x63/0xcd

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&type->i_mutex_dir_key#9/3);
                               lock(&sbi->lock);
                               lock(&type->i_mutex_dir_key#9/3);
  lock(&sbi->lock);

 *** DEADLOCK ***

3 locks held by syz-executor.1/19784:
 #0: ffff88801c7fe460 (sb_writers#17){.+.+}-{0:0}, at: path_setxattr+0xb2/0x1c0 fs/xattr.c:648
 #1: ffff8880899d7a20 (&type->i_mutex_dir_key#9){++++}-{3:3}, at: inode_lock include/linux/fs.h:756 [inline]
 #1: ffff8880899d7a20 (&type->i_mutex_dir_key#9){++++}-{3:3}, at: vfs_setxattr+0x120/0x340 fs/xattr.c:322
 #2: ffff8880899d6ce0 (&type->i_mutex_dir_key#9/3){+.+.}-{3:3}, at: inode_lock_nested include/linux/fs.h:791 [inline]
 #2: ffff8880899d6ce0 (&type->i_mutex_dir_key#9/3){+.+.}-{3:3}, at: open_xa_root fs/reiserfs/xattr.c:127 [inline]
 #2: ffff8880899d6ce0 (&type->i_mutex_dir_key#9/3){+.+.}-{3:3}, at: open_xa_dir+0x127/0x830 fs/reiserfs/xattr.c:152

stack backtrace:
CPU: 1 PID: 19784 Comm: syz-executor.1 Not tainted 6.2.0-rc1-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xd1/0x138 lib/dump_stack.c:106
 check_noncircular+0x25f/0x2e0 kernel/locking/lockdep.c:2177
 check_prev_add kernel/locking/lockdep.c:3097 [inline]
 check_prevs_add kernel/locking/lockdep.c:3216 [inline]
 validate_chain kernel/locking/lockdep.c:3831 [inline]
 __lock_acquire+0x2a43/0x56d0 kernel/locking/lockdep.c:5055
 lock_acquire kernel/locking/lockdep.c:5668 [inline]
 lock_acquire+0x1e3/0x630 kernel/locking/lockdep.c:5633
 __mutex_lock_common kernel/locking/mutex.c:603 [inline]
 __mutex_lock+0x12f/0x1360 kernel/locking/mutex.c:747
 reiserfs_write_lock+0x79/0x100 fs/reiserfs/lock.c:27
 reiserfs_mkdir+0x31c/0x990 fs/reiserfs/namei.c:831
 xattr_mkdir fs/reiserfs/xattr.c:76 [inline]
 open_xa_root fs/reiserfs/xattr.c:136 [inline]
 open_xa_dir+0x6a3/0x830 fs/reiserfs/xattr.c:152
 xattr_lookup+0x21/0x3d0 fs/reiserfs/xattr.c:395
 reiserfs_xattr_set_handle+0xfb/0xb00 fs/reiserfs/xattr.c:533
 reiserfs_xattr_set+0x454/0x5b0 fs/reiserfs/xattr.c:633
 trusted_set+0xa7/0xd0 fs/reiserfs/xattr_trusted.c:31
 __vfs_setxattr+0x173/0x1e0 fs/xattr.c:202
 __vfs_setxattr_noperm+0x129/0x5f0 fs/xattr.c:236
 __vfs_setxattr_locked+0x1d3/0x260 fs/xattr.c:297
 vfs_setxattr+0x143/0x340 fs/xattr.c:323
 do_setxattr+0x151/0x190 fs/xattr.c:608
 setxattr+0x146/0x160 fs/xattr.c:631
 path_setxattr+0x197/0x1c0 fs/xattr.c:650
 __do_sys_setxattr fs/xattr.c:666 [inline]
 __se_sys_setxattr fs/xattr.c:662 [inline]
 __x64_sys_setxattr+0xc4/0x160 fs/xattr.c:662
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7fd78168c0a9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 19 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fd78242a168 EFLAGS: 00000246 ORIG_RAX: 00000000000000bc
RAX: ffffffffffffffda RBX: 00007fd7817abf80 RCX: 00007fd78168c0a9
RDX: 0000000020000440 RSI: 00000000200002c0 RDI: 0000000020000280
RBP: 00007fd7816e7ae9 R08: 0000000000000000 R09: 0000000000000000
R10: 000000000000010c R11: 0000000000000246 R12: 0000000000000000
R13: 00007fff9b71513f R14: 00007fd78242a300 R15: 0000000000022000
 </TASK>
REISERFS warning (device loop1): vs-13060 reiserfs_update_sd_size: stat data of object [3 6 0x0 SD] (nlink == 2) not found (pos 5)
REISERFS warning (device loop1): vs-13060 reiserfs_update_sd_size: stat data of object [3 6 0x0 SD] (nlink == 3) not found (pos 3)


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2022-12-31  6:35 [syzbot] [reiserfs?] possible deadlock in open_xa_dir syzbot
@ 2023-05-05  7:10 ` syzbot
  2023-05-05 20:51 ` syzbot
  2024-03-09 22:20 ` syzbot
  2 siblings, 0 replies; 23+ messages in thread
From: syzbot @ 2023-05-05  7:10 UTC (permalink / raw)
  To: linux-fsdevel, linux-kernel, reiserfs-devel, syzkaller-bugs

syzbot has found a reproducer for the following issue on:

HEAD commit:    78b421b6a7c6 Merge tag 'linux-watchdog-6.4-rc1' of git://w..
git tree:       upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=150c42a8280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=14e2e518280000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17c03112280000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/25168a5d3be8/disk-78b421b6.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/f5af9f784d9d/vmlinux-78b421b6.xz
kernel image: https://storage.googleapis.com/syzbot-assets/7781e4e3e606/bzImage-78b421b6.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/29a59b19c7a8/mount_0.gz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com

======================================================
WARNING: possible circular locking dependency detected
6.3.0-syzkaller-13164-g78b421b6a7c6 #0 Not tainted
------------------------------------------------------
syz-executor363/4996 is trying to acquire lock:
ffff888074558980 (&type->i_mutex_dir_key#6/3){+.+.}-{3:3}, at: inode_lock_nested include/linux/fs.h:810 [inline]
ffff888074558980 (&type->i_mutex_dir_key#6/3){+.+.}-{3:3}, at: open_xa_root fs/reiserfs/xattr.c:128 [inline]
ffff888074558980 (&type->i_mutex_dir_key#6/3){+.+.}-{3:3}, at: open_xa_dir+0x136/0x610 fs/reiserfs/xattr.c:153

but task is already holding lock:
ffff8880156f2090 (&sbi->lock){+.+.}-{3:3}, at: reiserfs_write_lock_nested+0x5f/0xd0 fs/reiserfs/lock.c:78

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #1 (&sbi->lock){+.+.}-{3:3}:
       lock_acquire+0x1e3/0x520 kernel/locking/lockdep.c:5691
       __mutex_lock_common+0x1d8/0x2530 kernel/locking/mutex.c:603
       __mutex_lock kernel/locking/mutex.c:747 [inline]
       mutex_lock_nested+0x1b/0x20 kernel/locking/mutex.c:799
       reiserfs_write_lock+0x7a/0xd0 fs/reiserfs/lock.c:27
       reiserfs_mkdir+0x321/0x8f0 fs/reiserfs/namei.c:829
       xattr_mkdir fs/reiserfs/xattr.c:77 [inline]
       open_xa_root fs/reiserfs/xattr.c:137 [inline]
       open_xa_dir+0x2cd/0x610 fs/reiserfs/xattr.c:153
       xattr_lookup+0x24/0x280 fs/reiserfs/xattr.c:396
       reiserfs_xattr_set_handle+0xfc/0xdc0 fs/reiserfs/xattr.c:534
       reiserfs_security_write+0x157/0x1d0 fs/reiserfs/xattr_security.c:106
       reiserfs_new_inode+0x1631/0x1d40 fs/reiserfs/inode.c:2111
       reiserfs_create+0x3e7/0x6e0 fs/reiserfs/namei.c:666
       lookup_open fs/namei.c:3492 [inline]
       open_last_lookups fs/namei.c:3560 [inline]
       path_openat+0x13df/0x3170 fs/namei.c:3788
       do_filp_open+0x234/0x490 fs/namei.c:3818
       do_sys_openat2+0x13f/0x500 fs/open.c:1356
       do_sys_open fs/open.c:1372 [inline]
       __do_sys_openat fs/open.c:1388 [inline]
       __se_sys_openat fs/open.c:1383 [inline]
       __x64_sys_openat+0x247/0x290 fs/open.c:1383
       do_syscall_x64 arch/x86/entry/common.c:50 [inline]
       do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
       entry_SYSCALL_64_after_hwframe+0x63/0xcd

-> #0 (&type->i_mutex_dir_key#6/3){+.+.}-{3:3}:
       check_prev_add kernel/locking/lockdep.c:3108 [inline]
       check_prevs_add kernel/locking/lockdep.c:3227 [inline]
       validate_chain+0x166b/0x58e0 kernel/locking/lockdep.c:3842
       __lock_acquire+0x1295/0x2000 kernel/locking/lockdep.c:5074
       lock_acquire+0x1e3/0x520 kernel/locking/lockdep.c:5691
       down_write_nested+0x3d/0x50 kernel/locking/rwsem.c:1689
       inode_lock_nested include/linux/fs.h:810 [inline]
       open_xa_root fs/reiserfs/xattr.c:128 [inline]
       open_xa_dir+0x136/0x610 fs/reiserfs/xattr.c:153
       xattr_lookup+0x24/0x280 fs/reiserfs/xattr.c:396
       reiserfs_xattr_get+0xfd/0x570 fs/reiserfs/xattr.c:678
       __vfs_getxattr+0x436/0x470 fs/xattr.c:424
       smk_fetch+0xb1/0x140 security/smack/smack_lsm.c:295
       smack_d_instantiate+0x6d9/0xb40 security/smack/smack_lsm.c:3475
       security_d_instantiate+0x9b/0xf0 security/security.c:3760
       d_instantiate_new+0x65/0x120 fs/dcache.c:2053
       reiserfs_create+0x5cf/0x6e0 fs/reiserfs/namei.c:692
       lookup_open fs/namei.c:3492 [inline]
       open_last_lookups fs/namei.c:3560 [inline]
       path_openat+0x13df/0x3170 fs/namei.c:3788
       do_filp_open+0x234/0x490 fs/namei.c:3818
       do_sys_openat2+0x13f/0x500 fs/open.c:1356
       do_sys_open fs/open.c:1372 [inline]
       __do_sys_openat fs/open.c:1388 [inline]
       __se_sys_openat fs/open.c:1383 [inline]
       __x64_sys_openat+0x247/0x290 fs/open.c:1383
       do_syscall_x64 arch/x86/entry/common.c:50 [inline]
       do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
       entry_SYSCALL_64_after_hwframe+0x63/0xcd

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&sbi->lock);
                               lock(&type->i_mutex_dir_key#6/3);
                               lock(&sbi->lock);
  lock(&type->i_mutex_dir_key#6/3);

 *** DEADLOCK ***

3 locks held by syz-executor363/4996:
 #0: ffff88807c6be460 (sb_writers#9){.+.+}-{0:0}, at: mnt_want_write+0x3f/0x90 fs/namespace.c:394
 #1: ffff8880745582e0 (&type->i_mutex_dir_key#6){+.+.}-{3:3}, at: inode_lock include/linux/fs.h:775 [inline]
 #1: ffff8880745582e0 (&type->i_mutex_dir_key#6){+.+.}-{3:3}, at: open_last_lookups fs/namei.c:3557 [inline]
 #1: ffff8880745582e0 (&type->i_mutex_dir_key#6){+.+.}-{3:3}, at: path_openat+0x7ba/0x3170 fs/namei.c:3788
 #2: ffff8880156f2090 (&sbi->lock){+.+.}-{3:3}, at: reiserfs_write_lock_nested+0x5f/0xd0 fs/reiserfs/lock.c:78

stack backtrace:
CPU: 1 PID: 4996 Comm: syz-executor363 Not tainted 6.3.0-syzkaller-13164-g78b421b6a7c6 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/14/2023
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0x1e7/0x2d0 lib/dump_stack.c:106
 check_noncircular+0x2fe/0x3b0 kernel/locking/lockdep.c:2188
 check_prev_add kernel/locking/lockdep.c:3108 [inline]
 check_prevs_add kernel/locking/lockdep.c:3227 [inline]
 validate_chain+0x166b/0x58e0 kernel/locking/lockdep.c:3842
 __lock_acquire+0x1295/0x2000 kernel/locking/lockdep.c:5074
 lock_acquire+0x1e3/0x520 kernel/locking/lockdep.c:5691
 down_write_nested+0x3d/0x50 kernel/locking/rwsem.c:1689
 inode_lock_nested include/linux/fs.h:810 [inline]
 open_xa_root fs/reiserfs/xattr.c:128 [inline]
 open_xa_dir+0x136/0x610 fs/reiserfs/xattr.c:153
 xattr_lookup+0x24/0x280 fs/reiserfs/xattr.c:396
 reiserfs_xattr_get+0xfd/0x570 fs/reiserfs/xattr.c:678
 __vfs_getxattr+0x436/0x470 fs/xattr.c:424
 smk_fetch+0xb1/0x140 security/smack/smack_lsm.c:295
 smack_d_instantiate+0x6d9/0xb40 security/smack/smack_lsm.c:3475
 security_d_instantiate+0x9b/0xf0 security/security.c:3760
 d_instantiate_new+0x65/0x120 fs/dcache.c:2053
 reiserfs_create+0x5cf/0x6e0 fs/reiserfs/namei.c:692
 lookup_open fs/namei.c:3492 [inline]
 open_last_lookups fs/namei.c:3560 [inline]
 path_openat+0x13df/0x3170 fs/namei.c:3788
 do_filp_open+0x234/0x490 fs/namei.c:3818
 do_sys_openat2+0x13f/0x500 fs/open.c:1356
 do_sys_open fs/open.c:1372 [inline]
 __do_sys_openat fs/open.c:1388 [inline]
 __se_sys_openat fs/open.c:1383 [inline]
 __x64_sys_openat+0x247/0x290 fs/open.c:1383
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7fdd867933d9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 b1 14 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fdd867402f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
RAX: ffffffffffffffda RBX: 00007fdd868196c0 RCX: 00007fdd867933d9
RDX: 000000000000275a RSI: 0000000020000080 RDI: 00000000ffffff9c


---
If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2022-12-31  6:35 [syzbot] [reiserfs?] possible deadlock in open_xa_dir syzbot
  2023-05-05  7:10 ` syzbot
@ 2023-05-05 20:51 ` syzbot
  2023-05-05 21:36     ` Paul Moore
                     ` (2 more replies)
  2024-03-09 22:20 ` syzbot
  2 siblings, 3 replies; 23+ messages in thread
From: syzbot @ 2023-05-05 20:51 UTC (permalink / raw)
  To: hdanton, linux-fsdevel, linux-kernel, paul, reiserfs-devel,
	roberto.sassu, syzkaller-bugs

syzbot has bisected this issue to:

commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
Author: Roberto Sassu <roberto.sassu@huawei.com>
Date:   Fri Mar 31 12:32:18 2023 +0000

    reiserfs: Add security prefix to xattr name in reiserfs_security_write()

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of https://githu..
git tree:       upstream
final oops:     https://syzkaller.appspot.com/x/report.txt?x=16403182280000
console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000

Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in reiserfs_security_write()")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-05-05 20:51 ` syzbot
@ 2023-05-05 21:36     ` Paul Moore
  2023-06-01 20:19   ` Roberto Sassu
  2023-06-01 20:30   ` Roberto Sassu
  2 siblings, 0 replies; 23+ messages in thread
From: Paul Moore @ 2023-05-05 21:36 UTC (permalink / raw)
  To: syzbot
  Cc: hdanton, linux-fsdevel, linux-kernel, reiserfs-devel,
	roberto.sassu, syzkaller-bugs

On Fri, May 5, 2023 at 4:51 PM syzbot
<syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com> wrote:
>
> syzbot has bisected this issue to:
>
> commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> Author: Roberto Sassu <roberto.sassu@huawei.com>
> Date:   Fri Mar 31 12:32:18 2023 +0000
>
>     reiserfs: Add security prefix to xattr name in reiserfs_security_write()
>
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
> start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of https://githu..
> git tree:       upstream
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=16403182280000
> console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
> dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
>
> Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
> Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in reiserfs_security_write()")
>
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection

I don't think Roberto's patch identified above is the actual root
cause of this problem as reiserfs_xattr_set_handle() is called in
reiserfs_security_write() both before and after the patch.  However,
due to some bad logic in reiserfs_security_write() which Roberto
corrected, I'm thinking that it is possible this code is being
exercised for the first time and syzbot is starting to trigger a
locking issue in the reiserfs code ... ?

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
@ 2023-05-05 21:36     ` Paul Moore
  0 siblings, 0 replies; 23+ messages in thread
From: Paul Moore @ 2023-05-05 21:36 UTC (permalink / raw)
  To: syzbot
  Cc: hdanton, linux-fsdevel, linux-kernel, reiserfs-devel,
	roberto.sassu, syzkaller-bugs

On Fri, May 5, 2023 at 4:51 PM syzbot
<syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com> wrote:
>
> syzbot has bisected this issue to:
>
> commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> Author: Roberto Sassu <roberto.sassu@huawei.com>
> Date:   Fri Mar 31 12:32:18 2023 +0000
>
>     reiserfs: Add security prefix to xattr name in reiserfs_security_write()
>
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
> start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of https://githu..
> git tree:       upstream
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=16403182280000
> console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
> dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
>
> Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
> Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in reiserfs_security_write()")
>
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection

I don't think Roberto's patch identified above is the actual root
cause of this problem as reiserfs_xattr_set_handle() is called in
reiserfs_security_write() both before and after the patch.  However,
due to some bad logic in reiserfs_security_write() which Roberto
corrected, I'm thinking that it is possible this code is being
exercised for the first time and syzbot is starting to trigger a
locking issue in the reiserfs code ... ?

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-05-05 21:36     ` Paul Moore
@ 2023-05-31  9:49       ` Roberto Sassu
  -1 siblings, 0 replies; 23+ messages in thread
From: Roberto Sassu @ 2023-05-31  9:49 UTC (permalink / raw)
  To: Paul Moore, syzbot
  Cc: hdanton, linux-fsdevel, linux-kernel, reiserfs-devel,
	roberto.sassu, syzkaller-bugs, peterz, mingo, will

On 5/5/2023 11:36 PM, Paul Moore wrote:
> On Fri, May 5, 2023 at 4:51 PM syzbot
> <syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com> wrote:
>>
>> syzbot has bisected this issue to:
>>
>> commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
>> Author: Roberto Sassu <roberto.sassu@huawei.com>
>> Date:   Fri Mar 31 12:32:18 2023 +0000
>>
>>      reiserfs: Add security prefix to xattr name in reiserfs_security_write()
>>
>> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
>> start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of https://githu..
>> git tree:       upstream
>> final oops:     https://syzkaller.appspot.com/x/report.txt?x=16403182280000
>> console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
>> kernel config:  https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
>> dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
>> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
>>
>> Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
>> Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in reiserfs_security_write()")
>>
>> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
> 
> I don't think Roberto's patch identified above is the actual root
> cause of this problem as reiserfs_xattr_set_handle() is called in
> reiserfs_security_write() both before and after the patch.  However,
> due to some bad logic in reiserfs_security_write() which Roberto
> corrected, I'm thinking that it is possible this code is being
> exercised for the first time and syzbot is starting to trigger a
> locking issue in the reiserfs code ... ?

+ Jan, Jeff (which basically restructured the lock)

+ Petr, Ingo, Will

I involve the lockdep experts, to get a bit of help on this.

First of all, the lockdep warning is trivial to reproduce:

# dd if=/dev/zero of=reiserfs.img bs=1M count=100
# losetup -f --show reiserfs.img
/dev/loop0
# mkfs.reiserfs /dev/loop0
# mount /dev/loop0 /mnt/
# touch file0

In the testing system, Smack is the major LSM.

Ok, so the warning here is clear:

https://syzkaller.appspot.com/x/log.txt?x=12403182280000

However, I was looking if that can really happen. From this:

[   77.746561][ T5418] -> #1 (&sbi->lock){+.+.}-{3:3}:
[   77.753772][ T5418]        lock_acquire+0x23e/0x630
[   77.758792][ T5418]        __mutex_lock_common+0x1d8/0x2530
[   77.764504][ T5418]        mutex_lock_nested+0x1b/0x20
[   77.769868][ T5418]        reiserfs_write_lock+0x70/0xc0
[   77.775321][ T5418]        reiserfs_mkdir+0x321/0x870

I see that the lock is taken in reiserfs_write_lock(), while lockdep says:

[   77.710227][ T5418] but task is already holding lock:
[   77.717587][ T5418] ffff88807568d090 (&sbi->lock){+.+.}-{3:3}, at: 
reiserfs_write_lock_nested+0x4a/0xb0

which is in a different place, I believe here:

int reiserfs_paste_into_item(struct reiserfs_transaction_handle *th,
                              /* Path to the pasted item. */
[...]

         depth = reiserfs_write_unlock_nested(sb);
         dquot_free_space_nodirty(inode, pasted_size);
         reiserfs_write_lock_nested(sb, depth);
         return retval;
}

This is called by reiserfs_add_entry(), which is called by 
reiserfs_create() (it is in the lockdep trace). After returning to 
reiserfs_create(), d_instantiate_new() is called.

I don't know exactly, I take the part that the lock is held. But if it 
is held, how d_instantiate_new() can be executed in another task?

static int reiserfs_create(struct mnt_idmap *idmap, struct inode *dir,
                         struct dentry *dentry, umode_t mode, bool excl)
{

[...]

         reiserfs_write_lock(dir->i_sb);

         retval = journal_begin(&th, dir->i_sb, jbegin_count);

[...]

         d_instantiate_new(dentry, inode);
         retval = journal_end(&th);

out_failed:
         reiserfs_write_unlock(dir->i_sb);

If the lock is held, the scenario lockdep describes cannot happen. Any 
thoughts?

Thanks

Roberto


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
@ 2023-05-31  9:49       ` Roberto Sassu
  0 siblings, 0 replies; 23+ messages in thread
From: Roberto Sassu @ 2023-05-31  9:49 UTC (permalink / raw)
  To: Paul Moore, syzbot
  Cc: hdanton, linux-fsdevel, linux-kernel, reiserfs-devel,
	roberto.sassu, syzkaller-bugs, peterz, mingo, will

On 5/5/2023 11:36 PM, Paul Moore wrote:
> On Fri, May 5, 2023 at 4:51 PM syzbot
> <syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com> wrote:
>>
>> syzbot has bisected this issue to:
>>
>> commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
>> Author: Roberto Sassu <roberto.sassu@huawei.com>
>> Date:   Fri Mar 31 12:32:18 2023 +0000
>>
>>      reiserfs: Add security prefix to xattr name in reiserfs_security_write()
>>
>> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
>> start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of https://githu..
>> git tree:       upstream
>> final oops:     https://syzkaller.appspot.com/x/report.txt?x=16403182280000
>> console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
>> kernel config:  https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
>> dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
>> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
>>
>> Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
>> Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in reiserfs_security_write()")
>>
>> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
> 
> I don't think Roberto's patch identified above is the actual root
> cause of this problem as reiserfs_xattr_set_handle() is called in
> reiserfs_security_write() both before and after the patch.  However,
> due to some bad logic in reiserfs_security_write() which Roberto
> corrected, I'm thinking that it is possible this code is being
> exercised for the first time and syzbot is starting to trigger a
> locking issue in the reiserfs code ... ?

+ Jan, Jeff (which basically restructured the lock)

+ Petr, Ingo, Will

I involve the lockdep experts, to get a bit of help on this.

First of all, the lockdep warning is trivial to reproduce:

# dd if=/dev/zero of=reiserfs.img bs=1M count=100
# losetup -f --show reiserfs.img
/dev/loop0
# mkfs.reiserfs /dev/loop0
# mount /dev/loop0 /mnt/
# touch file0

In the testing system, Smack is the major LSM.

Ok, so the warning here is clear:

https://syzkaller.appspot.com/x/log.txt?x=12403182280000

However, I was looking if that can really happen. From this:

[   77.746561][ T5418] -> #1 (&sbi->lock){+.+.}-{3:3}:
[   77.753772][ T5418]        lock_acquire+0x23e/0x630
[   77.758792][ T5418]        __mutex_lock_common+0x1d8/0x2530
[   77.764504][ T5418]        mutex_lock_nested+0x1b/0x20
[   77.769868][ T5418]        reiserfs_write_lock+0x70/0xc0
[   77.775321][ T5418]        reiserfs_mkdir+0x321/0x870

I see that the lock is taken in reiserfs_write_lock(), while lockdep says:

[   77.710227][ T5418] but task is already holding lock:
[   77.717587][ T5418] ffff88807568d090 (&sbi->lock){+.+.}-{3:3}, at: 
reiserfs_write_lock_nested+0x4a/0xb0

which is in a different place, I believe here:

int reiserfs_paste_into_item(struct reiserfs_transaction_handle *th,
                              /* Path to the pasted item. */
[...]

         depth = reiserfs_write_unlock_nested(sb);
         dquot_free_space_nodirty(inode, pasted_size);
         reiserfs_write_lock_nested(sb, depth);
         return retval;
}

This is called by reiserfs_add_entry(), which is called by 
reiserfs_create() (it is in the lockdep trace). After returning to 
reiserfs_create(), d_instantiate_new() is called.

I don't know exactly, I take the part that the lock is held. But if it 
is held, how d_instantiate_new() can be executed in another task?

static int reiserfs_create(struct mnt_idmap *idmap, struct inode *dir,
                         struct dentry *dentry, umode_t mode, bool excl)
{

[...]

         reiserfs_write_lock(dir->i_sb);

         retval = journal_begin(&th, dir->i_sb, jbegin_count);

[...]

         d_instantiate_new(dentry, inode);
         retval = journal_end(&th);

out_failed:
         reiserfs_write_unlock(dir->i_sb);

If the lock is held, the scenario lockdep describes cannot happen. Any 
thoughts?

Thanks

Roberto


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-05-31  9:49       ` Roberto Sassu
  (?)
@ 2023-05-31  9:52       ` Roberto Sassu
  -1 siblings, 0 replies; 23+ messages in thread
From: Roberto Sassu @ 2023-05-31  9:52 UTC (permalink / raw)
  To: Paul Moore, syzbot
  Cc: hdanton, linux-fsdevel, linux-kernel, reiserfs-devel,
	roberto.sassu, syzkaller-bugs, peterz, mingo, will, Jeff Mahoney,
	Jan Kara

On 5/31/2023 11:49 AM, Roberto Sassu wrote:
> On 5/5/2023 11:36 PM, Paul Moore wrote:
>> On Fri, May 5, 2023 at 4:51 PM syzbot
>> <syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com> wrote:
>>>
>>> syzbot has bisected this issue to:
>>>
>>> commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
>>> Author: Roberto Sassu <roberto.sassu@huawei.com>
>>> Date:   Fri Mar 31 12:32:18 2023 +0000
>>>
>>>      reiserfs: Add security prefix to xattr name in 
>>> reiserfs_security_write()
>>>
>>> bisection log:  
>>> https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
>>> start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of 
>>> https://githu..
>>> git tree:       upstream
>>> final oops:     
>>> https://syzkaller.appspot.com/x/report.txt?x=16403182280000
>>> console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
>>> kernel config:  
>>> https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
>>> dashboard link: 
>>> https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
>>> syz repro:      
>>> https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
>>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
>>>
>>> Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
>>> Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in 
>>> reiserfs_security_write()")
>>>
>>> For information about bisection process see: 
>>> https://goo.gl/tpsmEJ#bisection
>>
>> I don't think Roberto's patch identified above is the actual root
>> cause of this problem as reiserfs_xattr_set_handle() is called in
>> reiserfs_security_write() both before and after the patch.  However,
>> due to some bad logic in reiserfs_security_write() which Roberto
>> corrected, I'm thinking that it is possible this code is being
>> exercised for the first time and syzbot is starting to trigger a
>> locking issue in the reiserfs code ... ?
> 
> + Jan, Jeff (which basically restructured the lock)

Actually adding Jan and Jeff.

Roberto

> + Petr, Ingo, Will
> 
> I involve the lockdep experts, to get a bit of help on this.
> 
> First of all, the lockdep warning is trivial to reproduce:
> 
> # dd if=/dev/zero of=reiserfs.img bs=1M count=100
> # losetup -f --show reiserfs.img
> /dev/loop0
> # mkfs.reiserfs /dev/loop0
> # mount /dev/loop0 /mnt/
> # touch file0
> 
> In the testing system, Smack is the major LSM.
> 
> Ok, so the warning here is clear:
> 
> https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> 
> However, I was looking if that can really happen. From this:
> 
> [   77.746561][ T5418] -> #1 (&sbi->lock){+.+.}-{3:3}:
> [   77.753772][ T5418]        lock_acquire+0x23e/0x630
> [   77.758792][ T5418]        __mutex_lock_common+0x1d8/0x2530
> [   77.764504][ T5418]        mutex_lock_nested+0x1b/0x20
> [   77.769868][ T5418]        reiserfs_write_lock+0x70/0xc0
> [   77.775321][ T5418]        reiserfs_mkdir+0x321/0x870
> 
> I see that the lock is taken in reiserfs_write_lock(), while lockdep says:
> 
> [   77.710227][ T5418] but task is already holding lock:
> [   77.717587][ T5418] ffff88807568d090 (&sbi->lock){+.+.}-{3:3}, at: 
> reiserfs_write_lock_nested+0x4a/0xb0
> 
> which is in a different place, I believe here:
> 
> int reiserfs_paste_into_item(struct reiserfs_transaction_handle *th,
>                               /* Path to the pasted item. */
> [...]
> 
>          depth = reiserfs_write_unlock_nested(sb);
>          dquot_free_space_nodirty(inode, pasted_size);
>          reiserfs_write_lock_nested(sb, depth);
>          return retval;
> }
> 
> This is called by reiserfs_add_entry(), which is called by 
> reiserfs_create() (it is in the lockdep trace). After returning to 
> reiserfs_create(), d_instantiate_new() is called.
> 
> I don't know exactly, I take the part that the lock is held. But if it 
> is held, how d_instantiate_new() can be executed in another task?
> 
> static int reiserfs_create(struct mnt_idmap *idmap, struct inode *dir,
>                          struct dentry *dentry, umode_t mode, bool excl)
> {
> 
> [...]
> 
>          reiserfs_write_lock(dir->i_sb);
> 
>          retval = journal_begin(&th, dir->i_sb, jbegin_count);
> 
> [...]
> 
>          d_instantiate_new(dentry, inode);
>          retval = journal_end(&th);
> 
> out_failed:
>          reiserfs_write_unlock(dir->i_sb);
> 
> If the lock is held, the scenario lockdep describes cannot happen. Any 
> thoughts?
> 
> Thanks
> 
> Roberto


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-05-05 20:51 ` syzbot
  2023-05-05 21:36     ` Paul Moore
@ 2023-06-01 20:19   ` Roberto Sassu
  2023-06-01 20:38     ` syzbot
  2023-06-01 20:30   ` Roberto Sassu
  2 siblings, 1 reply; 23+ messages in thread
From: Roberto Sassu @ 2023-06-01 20:19 UTC (permalink / raw)
  To: syzbot, hdanton, linux-fsdevel, linux-kernel, paul,
	reiserfs-devel, roberto.sassu, syzkaller-bugs, Jan Kara,
	Jeff Mahoney

On 5/5/2023 10:51 PM, syzbot wrote:
> syzbot has bisected this issue to:
> 
> commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> Author: Roberto Sassu <roberto.sassu@huawei.com>
> Date:   Fri Mar 31 12:32:18 2023 +0000
> 
>      reiserfs: Add security prefix to xattr name in reiserfs_security_write()
> 
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
> start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of https://githu..
> git tree:       upstream
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=16403182280000
> console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
> dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
> 
> Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
> Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in reiserfs_security_write()")
> 
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

--- a/fs/reiserfs/namei.c
+++ b/fs/reiserfs/namei.c
@@ -689,7 +689,9 @@ static int reiserfs_create(struct mnt_idmap *idmap, struct inode *dir,
         reiserfs_update_inode_transaction(inode);
         reiserfs_update_inode_transaction(dir);

+       reiserfs_write_unlock(dir->i_sb);
         d_instantiate_new(dentry, inode);
+       reiserfs_write_lock(dir->i_sb);
         retval = journal_end(&th);

  out_failed:
@@ -773,7 +775,9 @@ static int reiserfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
                 goto out_failed;
         }

+       reiserfs_write_unlock(dir->i_sb);
         d_instantiate_new(dentry, inode);
+       reiserfs_write_lock(dir->i_sb);
         retval = journal_end(&th);

  out_failed:
@@ -874,7 +878,9 @@ static int reiserfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
         /* the above add_entry did not update dir's stat data */
         reiserfs_update_sd(&th, dir);

+       reiserfs_write_unlock(dir->i_sb);
         d_instantiate_new(dentry, inode);
+       reiserfs_write_lock(dir->i_sb);
         retval = journal_end(&th);
  out_failed:
         reiserfs_write_unlock(dir->i_sb);
@@ -1191,7 +1197,9 @@ static int reiserfs_symlink(struct mnt_idmap *idmap,
                 goto out_failed;
         }

+       reiserfs_write_unlock(parent_dir->i_sb);
         d_instantiate_new(dentry, inode);
+       reiserfs_write_lock(parent_dir->i_sb);
         retval = journal_end(&th);
  out_failed:
         reiserfs_write_unlock(parent_dir->i_sb);


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-05-05 20:51 ` syzbot
  2023-05-05 21:36     ` Paul Moore
  2023-06-01 20:19   ` Roberto Sassu
@ 2023-06-01 20:30   ` Roberto Sassu
  2023-06-01 20:57     ` syzbot
  2 siblings, 1 reply; 23+ messages in thread
From: Roberto Sassu @ 2023-06-01 20:30 UTC (permalink / raw)
  To: syzbot, hdanton, linux-fsdevel, linux-kernel, paul,
	reiserfs-devel, roberto.sassu, syzkaller-bugs

[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]

On 5/5/2023 10:51 PM, syzbot wrote:
> syzbot has bisected this issue to:
> 
> commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> Author: Roberto Sassu <roberto.sassu@huawei.com>
> Date:   Fri Mar 31 12:32:18 2023 +0000
> 
>      reiserfs: Add security prefix to xattr name in reiserfs_security_write()
> 
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
> start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of https://githu..
> git tree:       upstream
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=16403182280000
> console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
> dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
> 
> Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
> Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in reiserfs_security_write()")
> 
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

[-- Attachment #2: 0001-reiserfs-Move-d_instantiate_new-out-of-the-write-loc.patch --]
[-- Type: text/plain, Size: 2724 bytes --]

From cf5445afc351bbc55a0080f1bc408ff496aeb879 Mon Sep 17 00:00:00 2001
From: Roberto Sassu <roberto.sassu@huawei.com>
Date: Thu, 1 Jun 2023 20:36:37 +0200
Subject: [RFC][PATCH] reiserfs: Move d_instantiate_new() out of the write lock

Commit 4c05141df57f ("reiserfs: locking, push write lock out of xattr
code") moved xattr operations outside the write lock. The problem is that
not all xattr operations are outside that lock.  For example, the write
lock is not released when d_instantiate_new() is called. At that time,
active LSMs likely fetch the content from their xattrs.

Mixing the two cases (xattr operations without and with a write lock)
could cause a deadlock. For example, a deadlock could happen due to the
following circular dependencies:

write lock (task A) -> inode lock (task B) ->write lock (task B)
-> inode lock (task A)

Make sure that all xattr operations are outside the write lock, by
wrapping all d_instantiate_new() calls with reiserfs_write_unlock() and
reiserfs_write_lock().

Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in reiserfs_security_write()")
Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 fs/reiserfs/namei.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/reiserfs/namei.c b/fs/reiserfs/namei.c
index 52240cc891c..3508bf1a75e 100644
--- a/fs/reiserfs/namei.c
+++ b/fs/reiserfs/namei.c
@@ -689,7 +689,9 @@ static int reiserfs_create(struct mnt_idmap *idmap, struct inode *dir,
 	reiserfs_update_inode_transaction(inode);
 	reiserfs_update_inode_transaction(dir);
 
+	reiserfs_write_unlock(dir->i_sb);
 	d_instantiate_new(dentry, inode);
+	reiserfs_write_lock(dir->i_sb);
 	retval = journal_end(&th);
 
 out_failed:
@@ -773,7 +775,9 @@ static int reiserfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
 		goto out_failed;
 	}
 
+	reiserfs_write_unlock(dir->i_sb);
 	d_instantiate_new(dentry, inode);
+	reiserfs_write_lock(dir->i_sb);
 	retval = journal_end(&th);
 
 out_failed:
@@ -874,7 +878,9 @@ static int reiserfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	/* the above add_entry did not update dir's stat data */
 	reiserfs_update_sd(&th, dir);
 
+	reiserfs_write_unlock(dir->i_sb);
 	d_instantiate_new(dentry, inode);
+	reiserfs_write_lock(dir->i_sb);
 	retval = journal_end(&th);
 out_failed:
 	reiserfs_write_unlock(dir->i_sb);
@@ -1191,7 +1197,9 @@ static int reiserfs_symlink(struct mnt_idmap *idmap,
 		goto out_failed;
 	}
 
+	reiserfs_write_unlock(parent_dir->i_sb);
 	d_instantiate_new(dentry, inode);
+	reiserfs_write_lock(parent_dir->i_sb);
 	retval = journal_end(&th);
 out_failed:
 	reiserfs_write_unlock(parent_dir->i_sb);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-06-01 20:19   ` Roberto Sassu
@ 2023-06-01 20:38     ` syzbot
  0 siblings, 0 replies; 23+ messages in thread
From: syzbot @ 2023-06-01 20:38 UTC (permalink / raw)
  To: hdanton, jack, jeffm, linux-fsdevel, linux-kernel, paul,
	reiserfs-devel, roberto.sassu, roberto.sassu, syzkaller-bugs

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

failed to apply patch:
checking file fs/reiserfs/namei.c
patch: **** unexpected end of file in patch



Tested on:

commit:         929ed21d Merge tag '6.4-rc4-smb3-client-fixes' of git:..
git tree:       upstream
dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
compiler:       
patch:          https://syzkaller.appspot.com/x/patch.diff?x=10efc079280000


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-06-01 20:30   ` Roberto Sassu
@ 2023-06-01 20:57     ` syzbot
  0 siblings, 0 replies; 23+ messages in thread
From: syzbot @ 2023-06-01 20:57 UTC (permalink / raw)
  To: hdanton, linux-fsdevel, linux-kernel, paul, reiserfs-devel,
	roberto.sassu, roberto.sassu, syzkaller-bugs

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com

Tested on:

commit:         929ed21d Merge tag '6.4-rc4-smb3-client-fixes' of git:..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=16c69fed280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=162cf2103e4a7453
dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1159c149280000

Note: testing is done by a robot and is best-effort only.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-05-31  9:49       ` Roberto Sassu
  (?)
  (?)
@ 2023-06-01 21:22       ` Jeff Mahoney
  2023-06-02  7:20           ` Roberto Sassu
  -1 siblings, 1 reply; 23+ messages in thread
From: Jeff Mahoney @ 2023-06-01 21:22 UTC (permalink / raw)
  To: Roberto Sassu, Paul Moore, syzbot
  Cc: hdanton, linux-fsdevel, linux-kernel, reiserfs-devel,
	roberto.sassu, syzkaller-bugs, peterz, mingo, will

On 5/31/23 05:49, Roberto Sassu wrote:
> On 5/5/2023 11:36 PM, Paul Moore wrote:
>> On Fri, May 5, 2023 at 4:51 PM syzbot
>> <syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com> wrote:
>>>
>>> syzbot has bisected this issue to:
>>>
>>> commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
>>> Author: Roberto Sassu <roberto.sassu@huawei.com>
>>> Date:   Fri Mar 31 12:32:18 2023 +0000
>>>
>>>      reiserfs: Add security prefix to xattr name in 
>>> reiserfs_security_write()
>>>
>>> bisection log:  
>>> https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
>>> start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of 
>>> https://githu..
>>> git tree:       upstream
>>> final oops:     
>>> https://syzkaller.appspot.com/x/report.txt?x=16403182280000
>>> console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
>>> kernel config:  
>>> https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
>>> dashboard link: 
>>> https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
>>> syz repro:      
>>> https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
>>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
>>>
>>> Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
>>> Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in 
>>> reiserfs_security_write()")
>>>
>>> For information about bisection process see: 
>>> https://goo.gl/tpsmEJ#bisection
>>
>> I don't think Roberto's patch identified above is the actual root
>> cause of this problem as reiserfs_xattr_set_handle() is called in
>> reiserfs_security_write() both before and after the patch.  However,
>> due to some bad logic in reiserfs_security_write() which Roberto
>> corrected, I'm thinking that it is possible this code is being
>> exercised for the first time and syzbot is starting to trigger a
>> locking issue in the reiserfs code ... ?
> 
> + Jan, Jeff (which basically restructured the lock)
> 
> + Petr, Ingo, Will
> 
> I involve the lockdep experts, to get a bit of help on this.

Yep, looks like that's been broken since it was added in 2009.  Since 
there can't be any users of it, it'd make sense to drop the security 
xattr support from reiserfs entirely.

> First of all, the lockdep warning is trivial to reproduce:
> 
> # dd if=/dev/zero of=reiserfs.img bs=1M count=100
> # losetup -f --show reiserfs.img
> /dev/loop0
> # mkfs.reiserfs /dev/loop0
> # mount /dev/loop0 /mnt/
> # touch file0
> 
> In the testing system, Smack is the major LSM.
> 
> Ok, so the warning here is clear:
> 
> https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> 
> However, I was looking if that can really happen. From this:
> 
> [   77.746561][ T5418] -> #1 (&sbi->lock){+.+.}-{3:3}:
> [   77.753772][ T5418]        lock_acquire+0x23e/0x630
> [   77.758792][ T5418]        __mutex_lock_common+0x1d8/0x2530
> [   77.764504][ T5418]        mutex_lock_nested+0x1b/0x20
> [   77.769868][ T5418]        reiserfs_write_lock+0x70/0xc0
> [   77.775321][ T5418]        reiserfs_mkdir+0x321/0x870
> 
> I see that the lock is taken in reiserfs_write_lock(), while lockdep says:
> 
> [   77.710227][ T5418] but task is already holding lock:
> [   77.717587][ T5418] ffff88807568d090 (&sbi->lock){+.+.}-{3:3}, at: 
> reiserfs_write_lock_nested+0x4a/0xb0
> 
> which is in a different place, I believe here:
> 
> int reiserfs_paste_into_item(struct reiserfs_transaction_handle *th,
>                               /* Path to the pasted item. */
> [...]
> 
>          depth = reiserfs_write_unlock_nested(sb);
>          dquot_free_space_nodirty(inode, pasted_size);
>          reiserfs_write_lock_nested(sb, depth);
>          return retval;
> }
> 
> This is called by reiserfs_add_entry(), which is called by 
> reiserfs_create() (it is in the lockdep trace). After returning to 
> reiserfs_create(), d_instantiate_new() is called.
> 
> I don't know exactly, I take the part that the lock is held. But if it 
> is held, how d_instantiate_new() can be executed in another task?
> 
> static int reiserfs_create(struct mnt_idmap *idmap, struct inode *dir,
>                          struct dentry *dentry, umode_t mode, bool excl)
> {
> 
> [...]
> 
>          reiserfs_write_lock(dir->i_sb);
> 
>          retval = journal_begin(&th, dir->i_sb, jbegin_count);
> 
> [...]
> 
>          d_instantiate_new(dentry, inode);
>          retval = journal_end(&th);
> 
> out_failed:
>          reiserfs_write_unlock(dir->i_sb);
> 
> If the lock is held, the scenario lockdep describes cannot happen. Any 
> thoughts?

It's important to understand that the reiserfs write lock was added as a 
subsystem-specific replacement for the BKL.  Given that reiserfs was 
dying already back then, it made more sense from a time management 
perspective to emulate that behavior internally rather than use new 
locking when practically nobody cared anymore.

See reiserfs_write_unlock_nested and reiserfs_write_lock_nested paired 
throughout the code.  It drops the lock when it passes a point where 
it's likely to schedule, just like the BKL would have.

Yes, it's a mess.  Just let it die quietly.

-Jeff

-- 
Jeff Mahoney
VP Engineering, Linux Systems


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-06-01 21:22       ` Jeff Mahoney
@ 2023-06-02  7:20           ` Roberto Sassu
  0 siblings, 0 replies; 23+ messages in thread
From: Roberto Sassu @ 2023-06-02  7:20 UTC (permalink / raw)
  To: Jeff Mahoney, Paul Moore, syzbot
  Cc: hdanton, linux-fsdevel, linux-kernel, reiserfs-devel,
	roberto.sassu, syzkaller-bugs, peterz, mingo, will, Jan Kara

On Thu, 2023-06-01 at 17:22 -0400, Jeff Mahoney wrote:
> On 5/31/23 05:49, Roberto Sassu wrote:
> > On 5/5/2023 11:36 PM, Paul Moore wrote:
> > > On Fri, May 5, 2023 at 4:51 PM syzbot
> > > <syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com> wrote:
> > > > syzbot has bisected this issue to:
> > > > 
> > > > commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> > > > Author: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Date:   Fri Mar 31 12:32:18 2023 +0000
> > > > 
> > > >      reiserfs: Add security prefix to xattr name in 
> > > > reiserfs_security_write()
> > > > 
> > > > bisection log:  
> > > > https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
> > > > start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of 
> > > > https://githu..
> > > > git tree:       upstream
> > > > final oops:     
> > > > https://syzkaller.appspot.com/x/report.txt?x=16403182280000
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> > > > kernel config:  
> > > > https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
> > > > dashboard link: 
> > > > https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
> > > > syz repro:      
> > > > https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
> > > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
> > > > 
> > > > Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
> > > > Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in 
> > > > reiserfs_security_write()")
> > > > 
> > > > For information about bisection process see: 
> > > > https://goo.gl/tpsmEJ#bisection
> > > 
> > > I don't think Roberto's patch identified above is the actual root
> > > cause of this problem as reiserfs_xattr_set_handle() is called in
> > > reiserfs_security_write() both before and after the patch.  However,
> > > due to some bad logic in reiserfs_security_write() which Roberto
> > > corrected, I'm thinking that it is possible this code is being
> > > exercised for the first time and syzbot is starting to trigger a
> > > locking issue in the reiserfs code ... ?
> > 
> > + Jan, Jeff (which basically restructured the lock)
> > 
> > + Petr, Ingo, Will

Peter, clearly (sorry!)

> I involve the lockdep experts, to get a bit of help on this.
> 
> Yep, looks like that's been broken since it was added in 2009.  Since 
> there can't be any users of it, it'd make sense to drop the security 
> xattr support from reiserfs entirely.

Thanks, Jeff. Will make a patch to implement your suggestion.

Meanwhile, I learned how to read lockdep a bit better. The following
format could have helped me to understand it more quickly. The proposal
is simply to change #n to CPU#n at the top of the trace, define labels
L#n for the locks, and add them where effectively are held.

[   77.746561][ T5418] -> CPU1 (&sbi->lock){+.+.}-{3:3}:
[   77.753772][ T5418]        lock_acquire+0x23e/0x630
[   77.758792][ T5418]        __mutex_lock_common+0x1d8/0x2530
[   77.764504][ T5418]   (L3) mutex_lock_nested+0x1b/0x20
[   77.769868][ T5418]        reiserfs_write_lock+0x70/0xc0
[   77.775321][ T5418]        reiserfs_mkdir+0x321/0x870
[   77.780509][ T5418]   (L2) open_xa_dir+0x259/0x540
[   77.785440][ T5418]        xattr_lookup+0x17/0x210
[   77.790378][ T5418]        reiserfs_xattr_set_handle+0xda/0xc80
[   77.796448][ T5418]        reiserfs_security_write+0x134/0x190
[   77.802416][ T5418]        reiserfs_new_inode+0x13bf/0x1a90
[   77.808124][ T5418]        reiserfs_create+0x3b1/0x680
[   77.813399][ T5418]        path_openat+0xf1e/0x2c10
[   77.818415][ T5418]        do_filp_open+0x22a/0x440
[   77.823433][ T5418]        do_sys_openat2+0x10f/0x430
[   77.828624][ T5418]        __x64_sys_creat+0x11e/0x160
[   77.833905][ T5418]        do_syscall_64+0x41/0xc0
[   77.838926][ T5418]        entry_SYSCALL_64_after_hwframe+0x63/0xcd
[   77.845514][ T5418] 
[   77.845514][ T5418] -> CPU0 (&type->i_mutex_dir_key#8/3){+.+.}-{3:3}:
[   77.854118][ T5418]        validate_chain+0x166b/0x58e0
[   77.859488][ T5418]        __lock_acquire+0x125b/0x1f80
[   77.864853][ T5418]        lock_acquire+0x23e/0x630
[   77.869909][ T5418]   (L4) down_write_nested+0x3d/0x50
[   77.875186][ T5418]        open_xa_dir+0x134/0x540
[   77.880117][ T5418]        xattr_lookup+0x17/0x210
[   77.885050][ T5418]        reiserfs_xattr_get+0xe1/0x4a0
[   77.890501][ T5418]        __vfs_getxattr+0x2fe/0x350
[   77.895802][ T5418]        smk_fetch+0x98/0xf0
[   77.900382][ T5418]        smack_d_instantiate+0x5d5/0xa20
[   77.906018][ T5418]        security_d_instantiate+0x6b/0xb0
[   77.911736][ T5418]        d_instantiate_new+0x5e/0xe0
[   77.917013][ T5418]   (L1) reiserfs_create+0x5ee/0x680
[   77.922293][ T5418]        path_openat+0xf1e/0x2c10
[   77.927308][ T5418]        do_filp_open+0x22a/0x440
[   77.932330][ T5418]        do_sys_openat2+0x10f/0x430
[   77.937515][ T5418]        __x64_sys_creat+0x11e/0x160
[   77.942874][ T5418]        do_syscall_64+0x41/0xc0
[   77.947796][ T5418]        entry_SYSCALL_64_after_hwframe+0x63/0xcd
[   77.954200][ T5418] 
[   77.954200][ T5418] other info that might help us debug this:
[   77.954200][ T5418] 
[   77.964508][ T5418]  Possible unsafe locking scenario:
[   77.964508][ T5418] 
[   77.972034][ T5418]   CPU0                         CPU1
[   77.977394][ T5418]   ----                         ----
[   77.982748][ T5418]   L1: lock(&sbi->lock);
[   77.986726][ T5418]                                L2: lock(&type->i_mutex_dir_key#8/3);
[   77.994618][ T5418]                                L3: lock(&sbi->lock);
[   78.001118][ T5418]   L4: lock(&type->i_mutex_dir_key#8/3);

Thanks

Roberto

> > First of all, the lockdep warning is trivial to reproduce:
> > 
> > # dd if=/dev/zero of=reiserfs.img bs=1M count=100
> > # losetup -f --show reiserfs.img
> > /dev/loop0
> > # mkfs.reiserfs /dev/loop0
> > # mount /dev/loop0 /mnt/
> > # touch file0
> > 
> > In the testing system, Smack is the major LSM.
> > 
> > Ok, so the warning here is clear:
> > 
> > https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> > 
> > However, I was looking if that can really happen. From this:
> > 
> > [   77.746561][ T5418] -> #1 (&sbi->lock){+.+.}-{3:3}:
> > [   77.753772][ T5418]        lock_acquire+0x23e/0x630
> > [   77.758792][ T5418]        __mutex_lock_common+0x1d8/0x2530
> > [   77.764504][ T5418]        mutex_lock_nested+0x1b/0x20
> > [   77.769868][ T5418]        reiserfs_write_lock+0x70/0xc0
> > [   77.775321][ T5418]        reiserfs_mkdir+0x321/0x870
> > 
> > I see that the lock is taken in reiserfs_write_lock(), while lockdep says:
> > 
> > [   77.710227][ T5418] but task is already holding lock:
> > [   77.717587][ T5418] ffff88807568d090 (&sbi->lock){+.+.}-{3:3}, at: 
> > reiserfs_write_lock_nested+0x4a/0xb0
> > 
> > which is in a different place, I believe here:
> > 
> > int reiserfs_paste_into_item(struct reiserfs_transaction_handle *th,
> >                               /* Path to the pasted item. */
> > [...]
> > 
> >          depth = reiserfs_write_unlock_nested(sb);
> >          dquot_free_space_nodirty(inode, pasted_size);
> >          reiserfs_write_lock_nested(sb, depth);
> >          return retval;
> > }
> > 
> > This is called by reiserfs_add_entry(), which is called by 
> > reiserfs_create() (it is in the lockdep trace). After returning to 
> > reiserfs_create(), d_instantiate_new() is called.
> > 
> > I don't know exactly, I take the part that the lock is held. But if it 
> > is held, how d_instantiate_new() can be executed in another task?
> > 
> > static int reiserfs_create(struct mnt_idmap *idmap, struct inode *dir,
> >                          struct dentry *dentry, umode_t mode, bool excl)
> > {
> > 
> > [...]
> > 
> >          reiserfs_write_lock(dir->i_sb);
> > 
> >          retval = journal_begin(&th, dir->i_sb, jbegin_count);
> > 
> > [...]
> > 
> >          d_instantiate_new(dentry, inode);
> >          retval = journal_end(&th);
> > 
> > out_failed:
> >          reiserfs_write_unlock(dir->i_sb);
> > 
> > If the lock is held, the scenario lockdep describes cannot happen. Any 
> > thoughts?
> 
> It's important to understand that the reiserfs write lock was added as a 
> subsystem-specific replacement for the BKL.  Given that reiserfs was 
> dying already back then, it made more sense from a time management 
> perspective to emulate that behavior internally rather than use new 
> locking when practically nobody cared anymore.
> 
> See reiserfs_write_unlock_nested and reiserfs_write_lock_nested paired 
> throughout the code.  It drops the lock when it passes a point where 
> it's likely to schedule, just like the BKL would have.
> 
> Yes, it's a mess.  Just let it die quietly.
> 
> -Jeff
> 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
@ 2023-06-02  7:20           ` Roberto Sassu
  0 siblings, 0 replies; 23+ messages in thread
From: Roberto Sassu @ 2023-06-02  7:20 UTC (permalink / raw)
  To: Jeff Mahoney, Paul Moore, syzbot
  Cc: hdanton, linux-fsdevel, linux-kernel, reiserfs-devel,
	roberto.sassu, syzkaller-bugs, peterz, mingo, will, Jan Kara

On Thu, 2023-06-01 at 17:22 -0400, Jeff Mahoney wrote:
> On 5/31/23 05:49, Roberto Sassu wrote:
> > On 5/5/2023 11:36 PM, Paul Moore wrote:
> > > On Fri, May 5, 2023 at 4:51 PM syzbot
> > > <syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com> wrote:
> > > > syzbot has bisected this issue to:
> > > > 
> > > > commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> > > > Author: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Date:   Fri Mar 31 12:32:18 2023 +0000
> > > > 
> > > >      reiserfs: Add security prefix to xattr name in 
> > > > reiserfs_security_write()
> > > > 
> > > > bisection log:  
> > > > https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
> > > > start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of 
> > > > https://githu..
> > > > git tree:       upstream
> > > > final oops:     
> > > > https://syzkaller.appspot.com/x/report.txt?x=16403182280000
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> > > > kernel config:  
> > > > https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
> > > > dashboard link: 
> > > > https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
> > > > syz repro:      
> > > > https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
> > > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
> > > > 
> > > > Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
> > > > Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in 
> > > > reiserfs_security_write()")
> > > > 
> > > > For information about bisection process see: 
> > > > https://goo.gl/tpsmEJ#bisection
> > > 
> > > I don't think Roberto's patch identified above is the actual root
> > > cause of this problem as reiserfs_xattr_set_handle() is called in
> > > reiserfs_security_write() both before and after the patch.  However,
> > > due to some bad logic in reiserfs_security_write() which Roberto
> > > corrected, I'm thinking that it is possible this code is being
> > > exercised for the first time and syzbot is starting to trigger a
> > > locking issue in the reiserfs code ... ?
> > 
> > + Jan, Jeff (which basically restructured the lock)
> > 
> > + Petr, Ingo, Will

Peter, clearly (sorry!)

> I involve the lockdep experts, to get a bit of help on this.
> 
> Yep, looks like that's been broken since it was added in 2009.  Since 
> there can't be any users of it, it'd make sense to drop the security 
> xattr support from reiserfs entirely.

Thanks, Jeff. Will make a patch to implement your suggestion.

Meanwhile, I learned how to read lockdep a bit better. The following
format could have helped me to understand it more quickly. The proposal
is simply to change #n to CPU#n at the top of the trace, define labels
L#n for the locks, and add them where effectively are held.

[   77.746561][ T5418] -> CPU1 (&sbi->lock){+.+.}-{3:3}:
[   77.753772][ T5418]        lock_acquire+0x23e/0x630
[   77.758792][ T5418]        __mutex_lock_common+0x1d8/0x2530
[   77.764504][ T5418]   (L3) mutex_lock_nested+0x1b/0x20
[   77.769868][ T5418]        reiserfs_write_lock+0x70/0xc0
[   77.775321][ T5418]        reiserfs_mkdir+0x321/0x870
[   77.780509][ T5418]   (L2) open_xa_dir+0x259/0x540
[   77.785440][ T5418]        xattr_lookup+0x17/0x210
[   77.790378][ T5418]        reiserfs_xattr_set_handle+0xda/0xc80
[   77.796448][ T5418]        reiserfs_security_write+0x134/0x190
[   77.802416][ T5418]        reiserfs_new_inode+0x13bf/0x1a90
[   77.808124][ T5418]        reiserfs_create+0x3b1/0x680
[   77.813399][ T5418]        path_openat+0xf1e/0x2c10
[   77.818415][ T5418]        do_filp_open+0x22a/0x440
[   77.823433][ T5418]        do_sys_openat2+0x10f/0x430
[   77.828624][ T5418]        __x64_sys_creat+0x11e/0x160
[   77.833905][ T5418]        do_syscall_64+0x41/0xc0
[   77.838926][ T5418]        entry_SYSCALL_64_after_hwframe+0x63/0xcd
[   77.845514][ T5418] 
[   77.845514][ T5418] -> CPU0 (&type->i_mutex_dir_key#8/3){+.+.}-{3:3}:
[   77.854118][ T5418]        validate_chain+0x166b/0x58e0
[   77.859488][ T5418]        __lock_acquire+0x125b/0x1f80
[   77.864853][ T5418]        lock_acquire+0x23e/0x630
[   77.869909][ T5418]   (L4) down_write_nested+0x3d/0x50
[   77.875186][ T5418]        open_xa_dir+0x134/0x540
[   77.880117][ T5418]        xattr_lookup+0x17/0x210
[   77.885050][ T5418]        reiserfs_xattr_get+0xe1/0x4a0
[   77.890501][ T5418]        __vfs_getxattr+0x2fe/0x350
[   77.895802][ T5418]        smk_fetch+0x98/0xf0
[   77.900382][ T5418]        smack_d_instantiate+0x5d5/0xa20
[   77.906018][ T5418]        security_d_instantiate+0x6b/0xb0
[   77.911736][ T5418]        d_instantiate_new+0x5e/0xe0
[   77.917013][ T5418]   (L1) reiserfs_create+0x5ee/0x680
[   77.922293][ T5418]        path_openat+0xf1e/0x2c10
[   77.927308][ T5418]        do_filp_open+0x22a/0x440
[   77.932330][ T5418]        do_sys_openat2+0x10f/0x430
[   77.937515][ T5418]        __x64_sys_creat+0x11e/0x160
[   77.942874][ T5418]        do_syscall_64+0x41/0xc0
[   77.947796][ T5418]        entry_SYSCALL_64_after_hwframe+0x63/0xcd
[   77.954200][ T5418] 
[   77.954200][ T5418] other info that might help us debug this:
[   77.954200][ T5418] 
[   77.964508][ T5418]  Possible unsafe locking scenario:
[   77.964508][ T5418] 
[   77.972034][ T5418]   CPU0                         CPU1
[   77.977394][ T5418]   ----                         ----
[   77.982748][ T5418]   L1: lock(&sbi->lock);
[   77.986726][ T5418]                                L2: lock(&type->i_mutex_dir_key#8/3);
[   77.994618][ T5418]                                L3: lock(&sbi->lock);
[   78.001118][ T5418]   L4: lock(&type->i_mutex_dir_key#8/3);

Thanks

Roberto

> > First of all, the lockdep warning is trivial to reproduce:
> > 
> > # dd if=/dev/zero of=reiserfs.img bs=1M count=100
> > # losetup -f --show reiserfs.img
> > /dev/loop0
> > # mkfs.reiserfs /dev/loop0
> > # mount /dev/loop0 /mnt/
> > # touch file0
> > 
> > In the testing system, Smack is the major LSM.
> > 
> > Ok, so the warning here is clear:
> > 
> > https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> > 
> > However, I was looking if that can really happen. From this:
> > 
> > [   77.746561][ T5418] -> #1 (&sbi->lock){+.+.}-{3:3}:
> > [   77.753772][ T5418]        lock_acquire+0x23e/0x630
> > [   77.758792][ T5418]        __mutex_lock_common+0x1d8/0x2530
> > [   77.764504][ T5418]        mutex_lock_nested+0x1b/0x20
> > [   77.769868][ T5418]        reiserfs_write_lock+0x70/0xc0
> > [   77.775321][ T5418]        reiserfs_mkdir+0x321/0x870
> > 
> > I see that the lock is taken in reiserfs_write_lock(), while lockdep says:
> > 
> > [   77.710227][ T5418] but task is already holding lock:
> > [   77.717587][ T5418] ffff88807568d090 (&sbi->lock){+.+.}-{3:3}, at: 
> > reiserfs_write_lock_nested+0x4a/0xb0
> > 
> > which is in a different place, I believe here:
> > 
> > int reiserfs_paste_into_item(struct reiserfs_transaction_handle *th,
> >                               /* Path to the pasted item. */
> > [...]
> > 
> >          depth = reiserfs_write_unlock_nested(sb);
> >          dquot_free_space_nodirty(inode, pasted_size);
> >          reiserfs_write_lock_nested(sb, depth);
> >          return retval;
> > }
> > 
> > This is called by reiserfs_add_entry(), which is called by 
> > reiserfs_create() (it is in the lockdep trace). After returning to 
> > reiserfs_create(), d_instantiate_new() is called.
> > 
> > I don't know exactly, I take the part that the lock is held. But if it 
> > is held, how d_instantiate_new() can be executed in another task?
> > 
> > static int reiserfs_create(struct mnt_idmap *idmap, struct inode *dir,
> >                          struct dentry *dentry, umode_t mode, bool excl)
> > {
> > 
> > [...]
> > 
> >          reiserfs_write_lock(dir->i_sb);
> > 
> >          retval = journal_begin(&th, dir->i_sb, jbegin_count);
> > 
> > [...]
> > 
> >          d_instantiate_new(dentry, inode);
> >          retval = journal_end(&th);
> > 
> > out_failed:
> >          reiserfs_write_unlock(dir->i_sb);
> > 
> > If the lock is held, the scenario lockdep describes cannot happen. Any 
> > thoughts?
> 
> It's important to understand that the reiserfs write lock was added as a 
> subsystem-specific replacement for the BKL.  Given that reiserfs was 
> dying already back then, it made more sense from a time management 
> perspective to emulate that behavior internally rather than use new 
> locking when practically nobody cared anymore.
> 
> See reiserfs_write_unlock_nested and reiserfs_write_lock_nested paired 
> throughout the code.  It drops the lock when it passes a point where 
> it's likely to schedule, just like the BKL would have.
> 
> Yes, it's a mess.  Just let it die quietly.
> 
> -Jeff
> 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-06-02  7:20           ` Roberto Sassu
  (?)
@ 2023-06-02  8:56           ` Roberto Sassu
  2023-06-02  9:17             ` syzbot
  -1 siblings, 1 reply; 23+ messages in thread
From: Roberto Sassu @ 2023-06-02  8:56 UTC (permalink / raw)
  To: Jeff Mahoney, Paul Moore, syzbot
  Cc: hdanton, linux-fsdevel, linux-kernel, reiserfs-devel,
	roberto.sassu, syzkaller-bugs, peterz, mingo, will, Jan Kara

[-- Attachment #1: Type: text/plain, Size: 2798 bytes --]

On Fri, 2023-06-02 at 09:20 +0200, Roberto Sassu wrote:
> On Thu, 2023-06-01 at 17:22 -0400, Jeff Mahoney wrote:
> > On 5/31/23 05:49, Roberto Sassu wrote:
> > > On 5/5/2023 11:36 PM, Paul Moore wrote:
> > > > On Fri, May 5, 2023 at 4:51 PM syzbot
> > > > <syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com> wrote:
> > > > > syzbot has bisected this issue to:
> > > > > 
> > > > > commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> > > > > Author: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Date:   Fri Mar 31 12:32:18 2023 +0000
> > > > > 
> > > > >      reiserfs: Add security prefix to xattr name in 
> > > > > reiserfs_security_write()
> > > > > 
> > > > > bisection log:  
> > > > > https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
> > > > > start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of 
> > > > > https://githu..
> > > > > git tree:       upstream
> > > > > final oops:     
> > > > > https://syzkaller.appspot.com/x/report.txt?x=16403182280000
> > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> > > > > kernel config:  
> > > > > https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
> > > > > dashboard link: 
> > > > > https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
> > > > > syz repro:      
> > > > > https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
> > > > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
> > > > > 
> > > > > Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
> > > > > Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in 
> > > > > reiserfs_security_write()")
> > > > > 
> > > > > For information about bisection process see: 
> > > > > https://goo.gl/tpsmEJ#bisection
> > > > 
> > > > I don't think Roberto's patch identified above is the actual root
> > > > cause of this problem as reiserfs_xattr_set_handle() is called in
> > > > reiserfs_security_write() both before and after the patch.  However,
> > > > due to some bad logic in reiserfs_security_write() which Roberto
> > > > corrected, I'm thinking that it is possible this code is being
> > > > exercised for the first time and syzbot is starting to trigger a
> > > > locking issue in the reiserfs code ... ?
> > > 
> > > + Jan, Jeff (which basically restructured the lock)
> > > 
> > > + Petr, Ingo, Will
> 
> Peter, clearly (sorry!)
> 
> > I involve the lockdep experts, to get a bit of help on this.
> > 
> > Yep, looks like that's been broken since it was added in 2009.  Since 
> > there can't be any users of it, it'd make sense to drop the security 
> > xattr support from reiserfs entirely.
> 
> Thanks, Jeff. Will make a patch to implement your suggestion.

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git next

[-- Attachment #2: 0001-reiserfs-Disable-security-xattr-initialization-since.patch --]
[-- Type: text/x-patch, Size: 2210 bytes --]

From a31f5b09e39e5a964457b0a52aa9c437a0bf7f63 Mon Sep 17 00:00:00 2001
From: Roberto Sassu <roberto.sassu@huawei.com>
Date: Fri, 2 Jun 2023 10:10:28 +0200
Subject: [PATCH] reiserfs: Disable security xattr initialization since it
 never worked

Commit d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in
reiserfs_security_write()"), while fixed the security xattr initialization,
it also revealed a circular locking dependency between the reiserfs write
lock and the inode lock.

Since the bug in security xattr initialization was introduced since the
beginning, there cannot be any user of this feature. Instead of trying to
fix the locking dependency, which was already challenging to convert from
BLK, just disable the feature.

However, still keep the security xattr handler, since it was introduced
earlier, and users might have manually added xattrs.

Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
Suggested-by: Jeff Mahoney <jeffm@suse.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 fs/reiserfs/super.c          | 2 ++
 fs/reiserfs/xattr_security.c | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index 929acce6e73..262041b87cd 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -1654,6 +1654,8 @@ static int read_super_block(struct super_block *s, int offset)
 
 	reiserfs_warning(NULL, "", "reiserfs filesystem is deprecated and "
 		"scheduled to be removed from the kernel in 2025");
+	reiserfs_warning(NULL, "", "initializing security xattrs never worked, disable it");
+
 	SB_BUFFER_WITH_SB(s) = bh;
 	SB_DISK_SUPER_BLOCK(s) = rs;
 
diff --git a/fs/reiserfs/xattr_security.c b/fs/reiserfs/xattr_security.c
index 078dd8cc312..4037a998dbf 100644
--- a/fs/reiserfs/xattr_security.c
+++ b/fs/reiserfs/xattr_security.c
@@ -69,6 +69,9 @@ int reiserfs_security_init(struct inode *dir, struct inode *inode,
 	sec->value = NULL;
 	sec->length = 0;
 
+	/* See warning in read_super_block(). */
+	return 0;
+
 	/* Don't add selinux attributes on xattrs - they'll never get used */
 	if (IS_PRIVATE(dir))
 		return 0;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-06-02  8:56           ` Roberto Sassu
@ 2023-06-02  9:17             ` syzbot
  2023-06-02 16:18               ` Roberto Sassu
  0 siblings, 1 reply; 23+ messages in thread
From: syzbot @ 2023-06-02  9:17 UTC (permalink / raw)
  To: hdanton, jack, jeffm, linux-fsdevel, linux-kernel, mingo, paul,
	peterz, reiserfs-devel, roberto.sassu, roberto.sassu,
	syzkaller-bugs, will

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com

Tested on:

commit:         4432b507 lsm: fix a number of misspellings
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git next
console output: https://syzkaller.appspot.com/x/log.txt?x=166c541d280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=38526bf24c8d961b
dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1095cd79280000

Note: testing is done by a robot and is best-effort only.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-06-02  9:17             ` syzbot
@ 2023-06-02 16:18               ` Roberto Sassu
  2023-06-02 16:39                 ` syzbot
  0 siblings, 1 reply; 23+ messages in thread
From: Roberto Sassu @ 2023-06-02 16:18 UTC (permalink / raw)
  To: syzbot, hdanton, jack, jeffm, linux-fsdevel, linux-kernel, mingo,
	paul, peterz, reiserfs-devel, roberto.sassu, syzkaller-bugs,
	will

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

On Fri, 2023-06-02 at 02:17 -0700, syzbot wrote:
> Hello,
> 
> syzbot has tested the proposed patch and the reproducer did not trigger any issue:
> 
> Reported-and-tested-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
> 
> Tested on:
> 
> commit:         4432b507 lsm: fix a number of misspellings
> git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git next
> console output: https://syzkaller.appspot.com/x/log.txt?x=166c541d280000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=38526bf24c8d961b
> dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
> compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
> patch:          https://syzkaller.appspot.com/x/patch.diff?x=1095cd79280000
> 
> Note: testing is done by a robot and is best-effort only.

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git next

[-- Attachment #2: 0001-reiserfs-Disable-by-default-security-xattr-init-sinc.patch --]
[-- Type: text/x-patch, Size: 2884 bytes --]

From 73bb02eb7a751c447af43d7cac7c191329b6dd55 Mon Sep 17 00:00:00 2001
From: Roberto Sassu <roberto.sassu@huawei.com>
Date: Fri, 2 Jun 2023 10:10:28 +0200
Subject: [PATCH] reiserfs: Disable by default security xattr init since it
 never worked

Commit d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in
reiserfs_security_write()"), while fixed the security xattr initialization,
it also revealed a circular locking dependency between the reiserfs write
lock and the inode lock.

Add the new config option CONFIG_REISERFS_FS_SECURITY_INIT to
enable/disable the feature. Also, since the bug in security xattr
initialization was introduced since the beginning, disable it by default.

Reported-and-tested-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
Suggested-by: Jeff Mahoney <jeffm@suse.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 fs/reiserfs/Kconfig          | 15 +++++++++++++++
 fs/reiserfs/super.c          |  3 +++
 fs/reiserfs/xattr_security.c |  3 +++
 3 files changed, 21 insertions(+)

diff --git a/fs/reiserfs/Kconfig b/fs/reiserfs/Kconfig
index 4d22ecfe0fa..a618d0bda7b 100644
--- a/fs/reiserfs/Kconfig
+++ b/fs/reiserfs/Kconfig
@@ -88,3 +88,18 @@ config REISERFS_FS_SECURITY
 
 	  If you are not using a security module that requires using
 	  extended attributes for file security labels, say N.
+
+config REISERFS_FS_SECURITY_INIT
+	bool "ReiserFS Security Labels initialization"
+	depends on REISERFS_FS_XATTR
+	default false
+	help
+	  Init new inodes with security labels provided by LSMs.
+
+	  It was broken from the beginning, since the xattr name was
+	  missing the 'security.' prefix.
+
+	  Enabling this option might cause lockdep warnings and
+	  ultimately deadlocks.
+
+	  If unsure, say N.
diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index 929acce6e73..b427d03d0ea 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -1654,6 +1654,9 @@ static int read_super_block(struct super_block *s, int offset)
 
 	reiserfs_warning(NULL, "", "reiserfs filesystem is deprecated and "
 		"scheduled to be removed from the kernel in 2025");
+	if (IS_ENABLED(CONFIG_REISERFS_FS_SECURITY_INIT))
+		reiserfs_warning(NULL, "", "initializing security xattrs can cause deadlocks");
+
 	SB_BUFFER_WITH_SB(s) = bh;
 	SB_DISK_SUPER_BLOCK(s) = rs;
 
diff --git a/fs/reiserfs/xattr_security.c b/fs/reiserfs/xattr_security.c
index 078dd8cc312..d82c4507803 100644
--- a/fs/reiserfs/xattr_security.c
+++ b/fs/reiserfs/xattr_security.c
@@ -69,6 +69,9 @@ int reiserfs_security_init(struct inode *dir, struct inode *inode,
 	sec->value = NULL;
 	sec->length = 0;
 
+	if (!IS_ENABLED(CONFIG_REISERFS_FS_SECURITY_INIT))
+		return 0;
+
 	/* Don't add selinux attributes on xattrs - they'll never get used */
 	if (IS_PRIVATE(dir))
 		return 0;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-06-02 16:18               ` Roberto Sassu
@ 2023-06-02 16:39                 ` syzbot
  0 siblings, 0 replies; 23+ messages in thread
From: syzbot @ 2023-06-02 16:39 UTC (permalink / raw)
  To: hdanton, jack, jeffm, linux-fsdevel, linux-kernel, mingo, paul,
	peterz, reiserfs-devel, roberto.sassu, roberto.sassu,
	syzkaller-bugs, will

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com

Tested on:

commit:         4432b507 lsm: fix a number of misspellings
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git next
console output: https://syzkaller.appspot.com/x/log.txt?x=16b47dd1280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=178a8c28652084e1
dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=102f6ab6280000

Note: testing is done by a robot and is best-effort only.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2023-06-02  7:20           ` Roberto Sassu
@ 2023-06-02 16:46             ` Roberto Sassu
  -1 siblings, 0 replies; 23+ messages in thread
From: Roberto Sassu @ 2023-06-02 16:46 UTC (permalink / raw)
  To: Jeff Mahoney, Paul Moore, syzbot
  Cc: hdanton, linux-fsdevel, linux-kernel, reiserfs-devel,
	roberto.sassu, syzkaller-bugs, peterz, mingo, will, Jan Kara

On Fri, 2023-06-02 at 09:20 +0200, Roberto Sassu wrote:
> On Thu, 2023-06-01 at 17:22 -0400, Jeff Mahoney wrote:
> > On 5/31/23 05:49, Roberto Sassu wrote:
> > > On 5/5/2023 11:36 PM, Paul Moore wrote:
> > > > On Fri, May 5, 2023 at 4:51 PM syzbot
> > > > <syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com> wrote:
> > > > > syzbot has bisected this issue to:
> > > > > 
> > > > > commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> > > > > Author: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Date:   Fri Mar 31 12:32:18 2023 +0000
> > > > > 
> > > > >      reiserfs: Add security prefix to xattr name in 
> > > > > reiserfs_security_write()
> > > > > 
> > > > > bisection log:  
> > > > > https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
> > > > > start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of 
> > > > > https://githu..
> > > > > git tree:       upstream
> > > > > final oops:     
> > > > > https://syzkaller.appspot.com/x/report.txt?x=16403182280000
> > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> > > > > kernel config:  
> > > > > https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
> > > > > dashboard link: 
> > > > > https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
> > > > > syz repro:      
> > > > > https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
> > > > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
> > > > > 
> > > > > Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
> > > > > Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in 
> > > > > reiserfs_security_write()")
> > > > > 
> > > > > For information about bisection process see: 
> > > > > https://goo.gl/tpsmEJ#bisection
> > > > 
> > > > I don't think Roberto's patch identified above is the actual root
> > > > cause of this problem as reiserfs_xattr_set_handle() is called in
> > > > reiserfs_security_write() both before and after the patch.  However,
> > > > due to some bad logic in reiserfs_security_write() which Roberto
> > > > corrected, I'm thinking that it is possible this code is being
> > > > exercised for the first time and syzbot is starting to trigger a
> > > > locking issue in the reiserfs code ... ?
> > > 
> > > + Jan, Jeff (which basically restructured the lock)
> > > 
> > > + Petr, Ingo, Will
> 
> Peter, clearly (sorry!)
> 
> > I involve the lockdep experts, to get a bit of help on this.
> > 
> > Yep, looks like that's been broken since it was added in 2009.  Since 
> > there can't be any users of it, it'd make sense to drop the security 
> > xattr support from reiserfs entirely.
> 
> Thanks, Jeff. Will make a patch to implement your suggestion.

Ok, I tried first to disable security xattr initialization and keep the
xattr handler.

Setting the security xattr manually triggers a lockdep warning. Even
worse, setting a trusted xattr manually triggers that too. So, not sure
how we should proceed.

Have you looked at:

https://lore.kernel.org/linux-kernel/8a48ede1-3a45-7c3c-39e9-36001ac09283@huaweicloud.com/

That silences the lockdep warning, but I'm far from saying that it
won't have any side effect...

Thanks

Roberto

> Meanwhile, I learned how to read lockdep a bit better. The following
> format could have helped me to understand it more quickly. The proposal
> is simply to change #n to CPU#n at the top of the trace, define labels
> L#n for the locks, and add them where effectively are held.
> 
> [   77.746561][ T5418] -> CPU1 (&sbi->lock){+.+.}-{3:3}:
> [   77.753772][ T5418]        lock_acquire+0x23e/0x630
> [   77.758792][ T5418]        __mutex_lock_common+0x1d8/0x2530
> [   77.764504][ T5418]   (L3) mutex_lock_nested+0x1b/0x20
> [   77.769868][ T5418]        reiserfs_write_lock+0x70/0xc0
> [   77.775321][ T5418]        reiserfs_mkdir+0x321/0x870
> [   77.780509][ T5418]   (L2) open_xa_dir+0x259/0x540
> [   77.785440][ T5418]        xattr_lookup+0x17/0x210
> [   77.790378][ T5418]        reiserfs_xattr_set_handle+0xda/0xc80
> [   77.796448][ T5418]        reiserfs_security_write+0x134/0x190
> [   77.802416][ T5418]        reiserfs_new_inode+0x13bf/0x1a90
> [   77.808124][ T5418]        reiserfs_create+0x3b1/0x680
> [   77.813399][ T5418]        path_openat+0xf1e/0x2c10
> [   77.818415][ T5418]        do_filp_open+0x22a/0x440
> [   77.823433][ T5418]        do_sys_openat2+0x10f/0x430
> [   77.828624][ T5418]        __x64_sys_creat+0x11e/0x160
> [   77.833905][ T5418]        do_syscall_64+0x41/0xc0
> [   77.838926][ T5418]        entry_SYSCALL_64_after_hwframe+0x63/0xcd
> [   77.845514][ T5418] 
> [   77.845514][ T5418] -> CPU0 (&type->i_mutex_dir_key#8/3){+.+.}-{3:3}:
> [   77.854118][ T5418]        validate_chain+0x166b/0x58e0
> [   77.859488][ T5418]        __lock_acquire+0x125b/0x1f80
> [   77.864853][ T5418]        lock_acquire+0x23e/0x630
> [   77.869909][ T5418]   (L4) down_write_nested+0x3d/0x50
> [   77.875186][ T5418]        open_xa_dir+0x134/0x540
> [   77.880117][ T5418]        xattr_lookup+0x17/0x210
> [   77.885050][ T5418]        reiserfs_xattr_get+0xe1/0x4a0
> [   77.890501][ T5418]        __vfs_getxattr+0x2fe/0x350
> [   77.895802][ T5418]        smk_fetch+0x98/0xf0
> [   77.900382][ T5418]        smack_d_instantiate+0x5d5/0xa20
> [   77.906018][ T5418]        security_d_instantiate+0x6b/0xb0
> [   77.911736][ T5418]        d_instantiate_new+0x5e/0xe0
> [   77.917013][ T5418]   (L1) reiserfs_create+0x5ee/0x680
> [   77.922293][ T5418]        path_openat+0xf1e/0x2c10
> [   77.927308][ T5418]        do_filp_open+0x22a/0x440
> [   77.932330][ T5418]        do_sys_openat2+0x10f/0x430
> [   77.937515][ T5418]        __x64_sys_creat+0x11e/0x160
> [   77.942874][ T5418]        do_syscall_64+0x41/0xc0
> [   77.947796][ T5418]        entry_SYSCALL_64_after_hwframe+0x63/0xcd
> [   77.954200][ T5418] 
> [   77.954200][ T5418] other info that might help us debug this:
> [   77.954200][ T5418] 
> [   77.964508][ T5418]  Possible unsafe locking scenario:
> [   77.964508][ T5418] 
> [   77.972034][ T5418]   CPU0                         CPU1
> [   77.977394][ T5418]   ----                         ----
> [   77.982748][ T5418]   L1: lock(&sbi->lock);
> [   77.986726][ T5418]                                L2: lock(&type->i_mutex_dir_key#8/3);
> [   77.994618][ T5418]                                L3: lock(&sbi->lock);
> [   78.001118][ T5418]   L4: lock(&type->i_mutex_dir_key#8/3);
> 
> Thanks
> 
> Roberto
> 
> > > First of all, the lockdep warning is trivial to reproduce:
> > > 
> > > # dd if=/dev/zero of=reiserfs.img bs=1M count=100
> > > # losetup -f --show reiserfs.img
> > > /dev/loop0
> > > # mkfs.reiserfs /dev/loop0
> > > # mount /dev/loop0 /mnt/
> > > # touch file0
> > > 
> > > In the testing system, Smack is the major LSM.
> > > 
> > > Ok, so the warning here is clear:
> > > 
> > > https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> > > 
> > > However, I was looking if that can really happen. From this:
> > > 
> > > [   77.746561][ T5418] -> #1 (&sbi->lock){+.+.}-{3:3}:
> > > [   77.753772][ T5418]        lock_acquire+0x23e/0x630
> > > [   77.758792][ T5418]        __mutex_lock_common+0x1d8/0x2530
> > > [   77.764504][ T5418]        mutex_lock_nested+0x1b/0x20
> > > [   77.769868][ T5418]        reiserfs_write_lock+0x70/0xc0
> > > [   77.775321][ T5418]        reiserfs_mkdir+0x321/0x870
> > > 
> > > I see that the lock is taken in reiserfs_write_lock(), while lockdep says:
> > > 
> > > [   77.710227][ T5418] but task is already holding lock:
> > > [   77.717587][ T5418] ffff88807568d090 (&sbi->lock){+.+.}-{3:3}, at: 
> > > reiserfs_write_lock_nested+0x4a/0xb0
> > > 
> > > which is in a different place, I believe here:
> > > 
> > > int reiserfs_paste_into_item(struct reiserfs_transaction_handle *th,
> > >                               /* Path to the pasted item. */
> > > [...]
> > > 
> > >          depth = reiserfs_write_unlock_nested(sb);
> > >          dquot_free_space_nodirty(inode, pasted_size);
> > >          reiserfs_write_lock_nested(sb, depth);
> > >          return retval;
> > > }
> > > 
> > > This is called by reiserfs_add_entry(), which is called by 
> > > reiserfs_create() (it is in the lockdep trace). After returning to 
> > > reiserfs_create(), d_instantiate_new() is called.
> > > 
> > > I don't know exactly, I take the part that the lock is held. But if it 
> > > is held, how d_instantiate_new() can be executed in another task?
> > > 
> > > static int reiserfs_create(struct mnt_idmap *idmap, struct inode *dir,
> > >                          struct dentry *dentry, umode_t mode, bool excl)
> > > {
> > > 
> > > [...]
> > > 
> > >          reiserfs_write_lock(dir->i_sb);
> > > 
> > >          retval = journal_begin(&th, dir->i_sb, jbegin_count);
> > > 
> > > [...]
> > > 
> > >          d_instantiate_new(dentry, inode);
> > >          retval = journal_end(&th);
> > > 
> > > out_failed:
> > >          reiserfs_write_unlock(dir->i_sb);
> > > 
> > > If the lock is held, the scenario lockdep describes cannot happen. Any 
> > > thoughts?
> > 
> > It's important to understand that the reiserfs write lock was added as a 
> > subsystem-specific replacement for the BKL.  Given that reiserfs was 
> > dying already back then, it made more sense from a time management 
> > perspective to emulate that behavior internally rather than use new 
> > locking when practically nobody cared anymore.
> > 
> > See reiserfs_write_unlock_nested and reiserfs_write_lock_nested paired 
> > throughout the code.  It drops the lock when it passes a point where 
> > it's likely to schedule, just like the BKL would have.
> > 
> > Yes, it's a mess.  Just let it die quietly.
> > 
> > -Jeff
> > 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
@ 2023-06-02 16:46             ` Roberto Sassu
  0 siblings, 0 replies; 23+ messages in thread
From: Roberto Sassu @ 2023-06-02 16:46 UTC (permalink / raw)
  To: Jeff Mahoney, Paul Moore, syzbot
  Cc: hdanton, linux-fsdevel, linux-kernel, reiserfs-devel,
	roberto.sassu, syzkaller-bugs, peterz, mingo, will, Jan Kara

On Fri, 2023-06-02 at 09:20 +0200, Roberto Sassu wrote:
> On Thu, 2023-06-01 at 17:22 -0400, Jeff Mahoney wrote:
> > On 5/31/23 05:49, Roberto Sassu wrote:
> > > On 5/5/2023 11:36 PM, Paul Moore wrote:
> > > > On Fri, May 5, 2023 at 4:51 PM syzbot
> > > > <syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com> wrote:
> > > > > syzbot has bisected this issue to:
> > > > > 
> > > > > commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> > > > > Author: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Date:   Fri Mar 31 12:32:18 2023 +0000
> > > > > 
> > > > >      reiserfs: Add security prefix to xattr name in 
> > > > > reiserfs_security_write()
> > > > > 
> > > > > bisection log:  
> > > > > https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
> > > > > start commit:   3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of 
> > > > > https://githu..
> > > > > git tree:       upstream
> > > > > final oops:     
> > > > > https://syzkaller.appspot.com/x/report.txt?x=16403182280000
> > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> > > > > kernel config:  
> > > > > https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
> > > > > dashboard link: 
> > > > > https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
> > > > > syz repro:      
> > > > > https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
> > > > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
> > > > > 
> > > > > Reported-by: syzbot+8fb64a61fdd96b50f3b8@syzkaller.appspotmail.com
> > > > > Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in 
> > > > > reiserfs_security_write()")
> > > > > 
> > > > > For information about bisection process see: 
> > > > > https://goo.gl/tpsmEJ#bisection
> > > > 
> > > > I don't think Roberto's patch identified above is the actual root
> > > > cause of this problem as reiserfs_xattr_set_handle() is called in
> > > > reiserfs_security_write() both before and after the patch.  However,
> > > > due to some bad logic in reiserfs_security_write() which Roberto
> > > > corrected, I'm thinking that it is possible this code is being
> > > > exercised for the first time and syzbot is starting to trigger a
> > > > locking issue in the reiserfs code ... ?
> > > 
> > > + Jan, Jeff (which basically restructured the lock)
> > > 
> > > + Petr, Ingo, Will
> 
> Peter, clearly (sorry!)
> 
> > I involve the lockdep experts, to get a bit of help on this.
> > 
> > Yep, looks like that's been broken since it was added in 2009.  Since 
> > there can't be any users of it, it'd make sense to drop the security 
> > xattr support from reiserfs entirely.
> 
> Thanks, Jeff. Will make a patch to implement your suggestion.

Ok, I tried first to disable security xattr initialization and keep the
xattr handler.

Setting the security xattr manually triggers a lockdep warning. Even
worse, setting a trusted xattr manually triggers that too. So, not sure
how we should proceed.

Have you looked at:

https://lore.kernel.org/linux-kernel/8a48ede1-3a45-7c3c-39e9-36001ac09283@huaweicloud.com/

That silences the lockdep warning, but I'm far from saying that it
won't have any side effect...

Thanks

Roberto

> Meanwhile, I learned how to read lockdep a bit better. The following
> format could have helped me to understand it more quickly. The proposal
> is simply to change #n to CPU#n at the top of the trace, define labels
> L#n for the locks, and add them where effectively are held.
> 
> [   77.746561][ T5418] -> CPU1 (&sbi->lock){+.+.}-{3:3}:
> [   77.753772][ T5418]        lock_acquire+0x23e/0x630
> [   77.758792][ T5418]        __mutex_lock_common+0x1d8/0x2530
> [   77.764504][ T5418]   (L3) mutex_lock_nested+0x1b/0x20
> [   77.769868][ T5418]        reiserfs_write_lock+0x70/0xc0
> [   77.775321][ T5418]        reiserfs_mkdir+0x321/0x870
> [   77.780509][ T5418]   (L2) open_xa_dir+0x259/0x540
> [   77.785440][ T5418]        xattr_lookup+0x17/0x210
> [   77.790378][ T5418]        reiserfs_xattr_set_handle+0xda/0xc80
> [   77.796448][ T5418]        reiserfs_security_write+0x134/0x190
> [   77.802416][ T5418]        reiserfs_new_inode+0x13bf/0x1a90
> [   77.808124][ T5418]        reiserfs_create+0x3b1/0x680
> [   77.813399][ T5418]        path_openat+0xf1e/0x2c10
> [   77.818415][ T5418]        do_filp_open+0x22a/0x440
> [   77.823433][ T5418]        do_sys_openat2+0x10f/0x430
> [   77.828624][ T5418]        __x64_sys_creat+0x11e/0x160
> [   77.833905][ T5418]        do_syscall_64+0x41/0xc0
> [   77.838926][ T5418]        entry_SYSCALL_64_after_hwframe+0x63/0xcd
> [   77.845514][ T5418] 
> [   77.845514][ T5418] -> CPU0 (&type->i_mutex_dir_key#8/3){+.+.}-{3:3}:
> [   77.854118][ T5418]        validate_chain+0x166b/0x58e0
> [   77.859488][ T5418]        __lock_acquire+0x125b/0x1f80
> [   77.864853][ T5418]        lock_acquire+0x23e/0x630
> [   77.869909][ T5418]   (L4) down_write_nested+0x3d/0x50
> [   77.875186][ T5418]        open_xa_dir+0x134/0x540
> [   77.880117][ T5418]        xattr_lookup+0x17/0x210
> [   77.885050][ T5418]        reiserfs_xattr_get+0xe1/0x4a0
> [   77.890501][ T5418]        __vfs_getxattr+0x2fe/0x350
> [   77.895802][ T5418]        smk_fetch+0x98/0xf0
> [   77.900382][ T5418]        smack_d_instantiate+0x5d5/0xa20
> [   77.906018][ T5418]        security_d_instantiate+0x6b/0xb0
> [   77.911736][ T5418]        d_instantiate_new+0x5e/0xe0
> [   77.917013][ T5418]   (L1) reiserfs_create+0x5ee/0x680
> [   77.922293][ T5418]        path_openat+0xf1e/0x2c10
> [   77.927308][ T5418]        do_filp_open+0x22a/0x440
> [   77.932330][ T5418]        do_sys_openat2+0x10f/0x430
> [   77.937515][ T5418]        __x64_sys_creat+0x11e/0x160
> [   77.942874][ T5418]        do_syscall_64+0x41/0xc0
> [   77.947796][ T5418]        entry_SYSCALL_64_after_hwframe+0x63/0xcd
> [   77.954200][ T5418] 
> [   77.954200][ T5418] other info that might help us debug this:
> [   77.954200][ T5418] 
> [   77.964508][ T5418]  Possible unsafe locking scenario:
> [   77.964508][ T5418] 
> [   77.972034][ T5418]   CPU0                         CPU1
> [   77.977394][ T5418]   ----                         ----
> [   77.982748][ T5418]   L1: lock(&sbi->lock);
> [   77.986726][ T5418]                                L2: lock(&type->i_mutex_dir_key#8/3);
> [   77.994618][ T5418]                                L3: lock(&sbi->lock);
> [   78.001118][ T5418]   L4: lock(&type->i_mutex_dir_key#8/3);
> 
> Thanks
> 
> Roberto
> 
> > > First of all, the lockdep warning is trivial to reproduce:
> > > 
> > > # dd if=/dev/zero of=reiserfs.img bs=1M count=100
> > > # losetup -f --show reiserfs.img
> > > /dev/loop0
> > > # mkfs.reiserfs /dev/loop0
> > > # mount /dev/loop0 /mnt/
> > > # touch file0
> > > 
> > > In the testing system, Smack is the major LSM.
> > > 
> > > Ok, so the warning here is clear:
> > > 
> > > https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> > > 
> > > However, I was looking if that can really happen. From this:
> > > 
> > > [   77.746561][ T5418] -> #1 (&sbi->lock){+.+.}-{3:3}:
> > > [   77.753772][ T5418]        lock_acquire+0x23e/0x630
> > > [   77.758792][ T5418]        __mutex_lock_common+0x1d8/0x2530
> > > [   77.764504][ T5418]        mutex_lock_nested+0x1b/0x20
> > > [   77.769868][ T5418]        reiserfs_write_lock+0x70/0xc0
> > > [   77.775321][ T5418]        reiserfs_mkdir+0x321/0x870
> > > 
> > > I see that the lock is taken in reiserfs_write_lock(), while lockdep says:
> > > 
> > > [   77.710227][ T5418] but task is already holding lock:
> > > [   77.717587][ T5418] ffff88807568d090 (&sbi->lock){+.+.}-{3:3}, at: 
> > > reiserfs_write_lock_nested+0x4a/0xb0
> > > 
> > > which is in a different place, I believe here:
> > > 
> > > int reiserfs_paste_into_item(struct reiserfs_transaction_handle *th,
> > >                               /* Path to the pasted item. */
> > > [...]
> > > 
> > >          depth = reiserfs_write_unlock_nested(sb);
> > >          dquot_free_space_nodirty(inode, pasted_size);
> > >          reiserfs_write_lock_nested(sb, depth);
> > >          return retval;
> > > }
> > > 
> > > This is called by reiserfs_add_entry(), which is called by 
> > > reiserfs_create() (it is in the lockdep trace). After returning to 
> > > reiserfs_create(), d_instantiate_new() is called.
> > > 
> > > I don't know exactly, I take the part that the lock is held. But if it 
> > > is held, how d_instantiate_new() can be executed in another task?
> > > 
> > > static int reiserfs_create(struct mnt_idmap *idmap, struct inode *dir,
> > >                          struct dentry *dentry, umode_t mode, bool excl)
> > > {
> > > 
> > > [...]
> > > 
> > >          reiserfs_write_lock(dir->i_sb);
> > > 
> > >          retval = journal_begin(&th, dir->i_sb, jbegin_count);
> > > 
> > > [...]
> > > 
> > >          d_instantiate_new(dentry, inode);
> > >          retval = journal_end(&th);
> > > 
> > > out_failed:
> > >          reiserfs_write_unlock(dir->i_sb);
> > > 
> > > If the lock is held, the scenario lockdep describes cannot happen. Any 
> > > thoughts?
> > 
> > It's important to understand that the reiserfs write lock was added as a 
> > subsystem-specific replacement for the BKL.  Given that reiserfs was 
> > dying already back then, it made more sense from a time management 
> > perspective to emulate that behavior internally rather than use new 
> > locking when practically nobody cared anymore.
> > 
> > See reiserfs_write_unlock_nested and reiserfs_write_lock_nested paired 
> > throughout the code.  It drops the lock when it passes a point where 
> > it's likely to schedule, just like the BKL would have.
> > 
> > Yes, it's a mess.  Just let it die quietly.
> > 
> > -Jeff
> > 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
  2022-12-31  6:35 [syzbot] [reiserfs?] possible deadlock in open_xa_dir syzbot
  2023-05-05  7:10 ` syzbot
  2023-05-05 20:51 ` syzbot
@ 2024-03-09 22:20 ` syzbot
  2 siblings, 0 replies; 23+ messages in thread
From: syzbot @ 2024-03-09 22:20 UTC (permalink / raw)
  To: axboe, brauner, hdanton, jack, jeffm, linux-fsdevel,
	linux-kernel, mingo, paul, peterz, reiserfs-devel, roberto.sassu,
	roberto.sassu, syzkaller-bugs, will

syzbot suspects this issue was fixed by commit:

commit 6f861765464f43a71462d52026fbddfc858239a5
Author: Jan Kara <jack@suse.cz>
Date:   Wed Nov 1 17:43:10 2023 +0000

    fs: Block writes to mounted block devices

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=11c28556180000
start commit:   5eff55d725a4 Merge tag 'platform-drivers-x86-v6.7-7' of gi..
git tree:       upstream
kernel config:  https://syzkaller.appspot.com/x/.config?x=655f8abe9fe69b3b
dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12d80b99e80000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=148cccdee80000

If the result looks correct, please mark the issue as fixed by replying with:

#syz fix: fs: Block writes to mounted block devices

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [syzbot] [reiserfs?] possible deadlock in open_xa_dir
       [not found] <20230505075628.4150-1-hdanton@sina.com>
@ 2023-05-05  8:32 ` syzbot
  0 siblings, 0 replies; 23+ messages in thread
From: syzbot @ 2023-05-05  8:32 UTC (permalink / raw)
  To: hdanton, linux-kernel, syzkaller-bugs

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
WARNING in reiserfs_check_lock_depth

REISERFS (device loop0): checking transaction log (loop0)
REISERFS (device loop0): Using r5 hash to sort names
REISERFS (device loop0): Created .reiserfs_priv - reserved for xattr storage.
------------[ cut here ]------------
WARNING: CPU: 1 PID: 5415 at fs/reiserfs/lock.c:91 reiserfs_check_lock_depth+0x71/0x90
Modules linked in:
CPU: 1 PID: 5415 Comm: syz-executor.0 Not tainted 6.3.0-syzkaller-13164-g78b421b6a7c6-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/14/2023
RIP: 0010:reiserfs_check_lock_depth+0x71/0x90 fs/reiserfs/lock.c:91
Code: 03 42 0f b6 04 38 84 c0 75 24 41 8b 1e 31 ff 89 de e8 a3 4f 58 ff 85 db 78 0b e8 ea 4b 58 ff 5b 41 5e 41 5f c3 e8 df 4b 58 ff <0f> 0b eb f1 44 89 f1 80 e1 07 80 c1 03 38 c1 7c cf 4c 89 f7 e8 86
RSP: 0018:ffffc9000581f520 EFLAGS: 00010293
RAX: ffffffff82334431 RBX: 00000000ffffffff RCX: ffff888025ed0000
RDX: 0000000000000000 RSI: 00000000ffffffff RDI: 0000000000000000
RBP: ffffc9000581f7b0 R08: ffffffff8233441d R09: ffffed100e877044
R10: 0000000000000000 R11: dffffc0000000001 R12: 1ffff92000b03f05
R13: dffffc0000000000 R14: ffff88807783b0c0 R15: dffffc0000000000
FS:  00007f9491058700(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f62321d1ff8 CR3: 000000002a3c7000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 do_journal_end+0x20f/0x4770 fs/reiserfs/journal.c:4018
 reiserfs_create+0x68e/0x710 fs/reiserfs/namei.c:694
 lookup_open fs/namei.c:3492 [inline]
 open_last_lookups fs/namei.c:3560 [inline]
 path_openat+0x13df/0x3170 fs/namei.c:3788
 do_filp_open+0x234/0x490 fs/namei.c:3818
 do_sys_openat2+0x13f/0x500 fs/open.c:1356
 do_sys_open fs/open.c:1372 [inline]
 __do_sys_openat fs/open.c:1388 [inline]
 __se_sys_openat fs/open.c:1383 [inline]
 __x64_sys_openat+0x247/0x290 fs/open.c:1383
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f949028c169
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 19 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f9491058168 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
RAX: ffffffffffffffda RBX: 00007f94903abf80 RCX: 00007f949028c169
RDX: 000000000000275a RSI: 0000000020000080 RDI: ffffffffffffff9c
RBP: 00007f94902e7ca1 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007fff4aaf8b4f R14: 00007f9491058300 R15: 0000000000022000
 </TASK>


Tested on:

commit:         78b421b6 Merge tag 'linux-watchdog-6.4-rc1' of git://w..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=1271004c280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
dashboard link: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1194d2f4280000


^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2024-03-09 22:20 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-31  6:35 [syzbot] [reiserfs?] possible deadlock in open_xa_dir syzbot
2023-05-05  7:10 ` syzbot
2023-05-05 20:51 ` syzbot
2023-05-05 21:36   ` Paul Moore
2023-05-05 21:36     ` Paul Moore
2023-05-31  9:49     ` Roberto Sassu
2023-05-31  9:49       ` Roberto Sassu
2023-05-31  9:52       ` Roberto Sassu
2023-06-01 21:22       ` Jeff Mahoney
2023-06-02  7:20         ` Roberto Sassu
2023-06-02  7:20           ` Roberto Sassu
2023-06-02  8:56           ` Roberto Sassu
2023-06-02  9:17             ` syzbot
2023-06-02 16:18               ` Roberto Sassu
2023-06-02 16:39                 ` syzbot
2023-06-02 16:46           ` Roberto Sassu
2023-06-02 16:46             ` Roberto Sassu
2023-06-01 20:19   ` Roberto Sassu
2023-06-01 20:38     ` syzbot
2023-06-01 20:30   ` Roberto Sassu
2023-06-01 20:57     ` syzbot
2024-03-09 22:20 ` syzbot
     [not found] <20230505075628.4150-1-hdanton@sina.com>
2023-05-05  8:32 ` syzbot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.