Re: [PATCH 1/3] ima: Remove inode lock

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2024-10-09 at 18:25 +0200, Roberto Sassu wrote:
> On Wed, 2024-10-09 at 11:37 -0400, Paul Moore wrote:
> > On Wed, Oct 9, 2024 at 11:36 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > On Tue, Oct 8, 2024 at 12:57 PM Roberto Sassu
> > > <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
> > > > 
> > > > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > > 
> > > > Move out the mutex in the ima_iint_cache structure to a new structure
> > > > called ima_iint_cache_lock, so that a lock can be taken regardless of
> > > > whether or not inode integrity metadata are stored in the inode.
> > > > 
> > > > Introduce ima_inode_security() to simplify accessing the new structure in
> > > > the inode security blob.
> > > > 
> > > > Move the mutex initialization and annotation in the new function
> > > > ima_inode_alloc_security() and introduce ima_iint_lock() and
> > > > ima_iint_unlock() to respectively lock and unlock the mutex.
> > > > 
> > > > Finally, expand the critical region in process_measurement() guarded by
> > > > iint->mutex up to where the inode was locked, use only one iint lock in
> > > > __ima_inode_hash(), since the mutex is now in the inode security blob, and
> > > > replace the inode_lock()/inode_unlock() calls in ima_check_last_writer().
> > > > 
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > > ---
> > > >  security/integrity/ima/ima.h      | 26 ++++++++---
> > > >  security/integrity/ima/ima_api.c  |  4 +-
> > > >  security/integrity/ima/ima_iint.c | 77 ++++++++++++++++++++++++++-----
> > > >  security/integrity/ima/ima_main.c | 39 +++++++---------
> > > >  4 files changed, 104 insertions(+), 42 deletions(-)
> > > 
> > > I'm not an IMA expert, but it looks reasonable to me, although
> > > shouldn't this carry a stable CC in the patch metadata?
> > > 
> > > Reviewed-by: Paul Moore <paul@xxxxxxxxxxxxxx>
> > 
> > Sorry, one more thing ... did you verify this patchset resolves the
> > syzbot problem?  I saw at least one reproducer.
> 
> Uhm, could not reproduce the deadlock with the reproducer. However,
> without the patch I have a lockdep warning, and with I don't.
> 
> I asked syzbot to try the patches. Let's see.

I actually got a different lockdep warning:

[  904.603365] ======================================================
[  904.604264] WARNING: possible circular locking dependency detected
[  904.605141] 6.12.0-rc2+ #20 Not tainted
[  904.605697] ------------------------------------------------------
[  904.606577] systemd/1 is trying to acquire lock:
[  904.607227] ffff88810e5c2580 (&p->lock){+.+.}-{4:4}, at: seq_read_iter+0x62/0x6b0
[  904.608290] 
[  904.608290] but task is already holding lock:
[  904.609105] ffff88810f4abf20 (&ima_iint_lock_mutex_key[depth]){+.+.}-{4:4}, at: ima_iint_lock+0x24/0x40
[  904.610429] 
[  904.610429] which lock already depends on the new lock.
[  904.610429] 
[  904.611574] 
[  904.611574] the existing dependency chain (in reverse order) is:
[  904.612628] 
[  904.612628] -> #2 (&ima_iint_lock_mutex_key[depth]){+.+.}-{4:4}:
[  904.613681]        __mutex_lock+0xaf/0x760
[  904.614266]        mutex_lock_nested+0x27/0x40
[  904.614897]        ima_iint_lock+0x24/0x40
[  904.615490]        process_measurement+0x176/0xef0
[  904.616168]        ima_file_mmap+0x98/0x120
[  904.616767]        security_mmap_file+0x408/0x560
[  904.617444]        __do_sys_remap_file_pages+0x2fa/0x4c0
[  904.618194]        __x64_sys_remap_file_pages+0x29/0x40
[  904.618937]        x64_sys_call+0x6e8/0x4550
[  904.619546]        do_syscall_64+0x71/0x180
[  904.620155]        entry_SYSCALL_64_after_hwframe+0x76/0x7e
[  904.620952] 
[  904.620952] -> #1 (&mm->mmap_lock){++++}-{4:4}:
[  904.621813]        __might_fault+0x6f/0xb0
[  904.622400]        _copy_to_iter+0x12e/0xa80
[  904.623009]        seq_read_iter+0x593/0x6b0
[  904.623629]        proc_reg_read_iter+0x31/0xe0
[  904.624276]        vfs_read+0x256/0x3d0
[  904.624822]        ksys_read+0x6d/0x160
[  904.625372]        __x64_sys_read+0x1d/0x30
[  904.625964]        x64_sys_call+0x1068/0x4550
[  904.626594]        do_syscall_64+0x71/0x180
[  904.627188]        entry_SYSCALL_64_after_hwframe+0x76/0x7e
[  904.627975] 
[  904.627975] -> #0 (&p->lock){+.+.}-{4:4}:
[  904.628787]        __lock_acquire+0x17f3/0x2320
[  904.629432]        lock_acquire+0xf2/0x420
[  904.630013]        __mutex_lock+0xaf/0x760
[  904.630596]        mutex_lock_nested+0x27/0x40
[  904.631225]        seq_read_iter+0x62/0x6b0
[  904.631831]        kernfs_fop_read_iter+0x1ef/0x2c0
[  904.632599]        __kernel_read+0x113/0x350
[  904.633206]        integrity_kernel_read+0x23/0x40
[  904.633902]        ima_calc_file_hash_tfm+0x14e/0x230
[  904.634621]        ima_calc_file_hash+0x97/0x250
[  904.635281]        ima_collect_measurement+0x4be/0x530
[  904.636008]        process_measurement+0x7c0/0xef0
[  904.636689]        ima_file_check+0x65/0x80
[  904.637295]        security_file_post_open+0xb1/0x1b0
[  904.638008]        path_openat+0x216/0x1280
[  904.638605]        do_filp_open+0xab/0x140
[  904.639185]        do_sys_openat2+0xba/0x120
[  904.639805]        do_sys_open+0x4c/0x80
[  904.640366]        __x64_sys_openat+0x23/0x30
[  904.640992]        x64_sys_call+0x2575/0x4550
[  904.641616]        do_syscall_64+0x71/0x180
[  904.642207]        entry_SYSCALL_64_after_hwframe+0x76/0x7e
[  904.643003] 
[  904.643003] other info that might help us debug this:
[  904.643003] 
[  904.644149] Chain exists of:
[  904.644149]   &p->lock --> &mm->mmap_lock --> &ima_iint_lock_mutex_key[depth]
[  904.644149] 
[  904.645763]  Possible unsafe locking scenario:
[  904.645763] 
[  904.646614]        CPU0                    CPU1
[  904.647264]        ----                    ----
[  904.647909]   lock(&ima_iint_lock_mutex_key[depth]);
[  904.648617]                                lock(&mm->mmap_lock);
[  904.649479]                                lock(&ima_iint_lock_mutex_key[depth]);
[  904.650543]   lock(&p->lock);
[  904.650974] 
[  904.650974]  *** DEADLOCK ***
[  904.650974] 
[  904.651826] 1 lock held by systemd/1:
[  904.652376]  #0: ffff88810f4abf20 (&ima_iint_lock_mutex_key[depth]){+.+.}-{4:4}, at: ima_iint_lock+0x24/0x40
[  904.653759] 
[  904.653759] stack backtrace:
[  904.654391] CPU: 2 UID: 0 PID: 1 Comm: systemd Not tainted 6.12.0-rc2+ #20
[  904.655360] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
[  904.656497] Call Trace:
[  904.656856]  <TASK>
[  904.657166]  dump_stack_lvl+0x134/0x1a0
[  904.657728]  dump_stack+0x14/0x30
[  904.658206]  print_circular_bug+0x38d/0x450
[  904.658812]  check_noncircular+0xed/0x120
[  904.659396]  ? srso_return_thunk+0x5/0x5f
[  904.659972]  ? srso_return_thunk+0x5/0x5f
[  904.660569]  __lock_acquire+0x17f3/0x2320
[  904.661145]  lock_acquire+0xf2/0x420
[  904.661664]  ? seq_read_iter+0x62/0x6b0
[  904.662217]  ? srso_return_thunk+0x5/0x5f
[  904.662886]  __mutex_lock+0xaf/0x760
[  904.663408]  ? seq_read_iter+0x62/0x6b0
[  904.663961]  ? seq_read_iter+0x62/0x6b0
[  904.664525]  ? srso_return_thunk+0x5/0x5f
[  904.665098]  ? mark_lock+0x4e/0x750
[  904.665610]  ? mutex_lock_nested+0x27/0x40
[  904.666194]  ? find_held_lock+0x3a/0x100
[  904.666770]  mutex_lock_nested+0x27/0x40
[  904.667337]  seq_read_iter+0x62/0x6b0
[  904.667869]  kernfs_fop_read_iter+0x1ef/0x2c0
[  904.668536]  __kernel_read+0x113/0x350
[  904.669079]  integrity_kernel_read+0x23/0x40
[  904.669698]  ima_calc_file_hash_tfm+0x14e/0x230
[  904.670349]  ? __lock_acquire+0xc32/0x2320
[  904.670937]  ? srso_return_thunk+0x5/0x5f
[  904.671525]  ? __lock_acquire+0xfbb/0x2320
[  904.672113]  ? srso_return_thunk+0x5/0x5f
[  904.672693]  ? srso_return_thunk+0x5/0x5f
[  904.673280]  ? lock_acquire+0xf2/0x420
[  904.673818]  ? kernfs_iop_getattr+0x4a/0xb0
[  904.674424]  ? srso_return_thunk+0x5/0x5f
[  904.674997]  ? find_held_lock+0x3a/0x100
[  904.675564]  ? srso_return_thunk+0x5/0x5f
[  904.676156]  ? srso_return_thunk+0x5/0x5f
[  904.676740]  ? srso_return_thunk+0x5/0x5f
[  904.677322]  ? local_clock_noinstr+0x9/0xb0
[  904.677923]  ? srso_return_thunk+0x5/0x5f
[  904.678502]  ? srso_return_thunk+0x5/0x5f
[  904.679075]  ? lock_release+0x4e2/0x570
[  904.679639]  ima_calc_file_hash+0x97/0x250
[  904.680227]  ima_collect_measurement+0x4be/0x530
[  904.680901]  ? srso_return_thunk+0x5/0x5f
[  904.681496]  ? srso_return_thunk+0x5/0x5f
[  904.682070]  ? __kernfs_iattrs+0x4a/0x140
[  904.682658]  ? srso_return_thunk+0x5/0x5f
[  904.683242]  ? process_measurement+0x7c0/0xef0
[  904.683876]  ? srso_return_thunk+0x5/0x5f
[  904.684462]  process_measurement+0x7c0/0xef0
[  904.685078]  ? srso_return_thunk+0x5/0x5f
[  904.685654]  ? srso_return_thunk+0x5/0x5f
[  904.686228]  ? _raw_spin_unlock_irqrestore+0x5d/0xd0
[  904.686938]  ? srso_return_thunk+0x5/0x5f
[  904.687523]  ? srso_return_thunk+0x5/0x5f
[  904.688098]  ? srso_return_thunk+0x5/0x5f
[  904.688672]  ? local_clock_noinstr+0x9/0xb0
[  904.689273]  ? srso_return_thunk+0x5/0x5f
[  904.689846]  ? srso_return_thunk+0x5/0x5f
[  904.690430]  ? srso_return_thunk+0x5/0x5f
[  904.691005]  ? srso_return_thunk+0x5/0x5f
[  904.691583]  ? srso_return_thunk+0x5/0x5f
[  904.692180]  ? local_clock_noinstr+0x9/0xb0
[  904.692841]  ? srso_return_thunk+0x5/0x5f
[  904.693419]  ? srso_return_thunk+0x5/0x5f
[  904.693990]  ? lock_release+0x4e2/0x570
[  904.694544]  ? srso_return_thunk+0x5/0x5f
[  904.695115]  ? kernfs_put_active+0x5d/0xc0
[  904.695708]  ? srso_return_thunk+0x5/0x5f
[  904.696286]  ? kernfs_fop_open+0x376/0x6b0
[  904.696872]  ima_file_check+0x65/0x80
[  904.697409]  security_file_post_open+0xb1/0x1b0
[  904.698058]  path_openat+0x216/0x1280
[  904.698589]  do_filp_open+0xab/0x140
[  904.699106]  ? srso_return_thunk+0x5/0x5f
[  904.699693]  ? lock_release+0x554/0x570
[  904.700264]  ? srso_return_thunk+0x5/0x5f
[  904.700836]  ? do_raw_spin_unlock+0x76/0x140
[  904.701450]  ? srso_return_thunk+0x5/0x5f
[  904.702021]  ? _raw_spin_unlock+0x3f/0xa0
[  904.702606]  ? srso_return_thunk+0x5/0x5f
[  904.703178]  ? alloc_fd+0x1ca/0x3b0
[  904.703685]  do_sys_openat2+0xba/0x120
[  904.704223]  ? file_free+0x8d/0x110
[  904.704729]  do_sys_open+0x4c/0x80
[  904.705221]  __x64_sys_openat+0x23/0x30
[  904.705784]  x64_sys_call+0x2575/0x4550
[  904.706337]  do_syscall_64+0x71/0x180
[  904.706863]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[  904.707587] RIP: 0033:0x7f3462123037
[  904.708120] Code: 55 9c 48 89 75 a0 89 7d a8 44 89 55 ac e8 a1 7a f8 ff 44 8b 55 ac 8b 55 9c 41 89 c0 48 8b 75 a0 8b 7d a8 b8 01 01 00 00 0f 05 <48> 3d 00 f0 ff ff 77 31 44 89 c7 89 45 ac e8 f6 7a f8 ff 8b 45 ac
[  904.710744] RSP: 002b:00007ffdd1a79370 EFLAGS: 00000293 ORIG_RAX: 0000000000000101
[  904.711821] RAX: ffffffffffffffda RBX: ffffffffffffffff RCX: 00007f3462123037
[  904.712829] RDX: 0000000000080100 RSI: 00007ffdd1a79420 RDI: 00000000ffffff9c
[  904.713839] RBP: 00007ffdd1a793e0 R08: 0000000000000000 R09: 0000000000000000
[  904.714848] R10: 0000000000000000 R11: 0000000000000293 R12: 00007ffdd1a794c8
[  904.715855] R13: 00007ffdd1a794b8 R14: 00007ffdd1a79690 R15: 00007ffdd1a79690
[  904.716877]  </TASK>

Roberto






[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux