Re: [PATCH] jffs2: silence lockdep warning on evict path

[Richard Weinberger <richard@nod.at>]

----- Ursprüngliche Mail -----
> Von: "Ilya Shchipletsov" <rabbelkin@mail.ru>
> An: "linux-mtd" <linux-mtd@lists.infradead.org>
> CC: "Ilya Shchipletsov" <rabbelkin@mail.ru>, "David Woodhouse" <dwmw2@infradead.org>, "richard" <richard@nod.at>,
> "chengzhihao1" <chengzhihao1@huawei.com>, "Nikita Marushkin" <hfggklm@gmail.com>, "linux-kernel"
> <linux-kernel@vger.kernel.org>, "lvc-project" <lvc-project@linuxtesting.org>
> Gesendet: Freitag, 4. April 2025 10:00:18
> Betreff: [PATCH] jffs2: silence lockdep warning on evict path

> Syzkaller detected a possible deadlock in jffs2_do_clear_inode that happens
> in kswapd's evict path. This is however a false positive because in
> jffs2_evict_inode we are the only holder of inode and nobody else should be
> touching any locks of such inode.
> 
> WARNING: possible circular locking dependency detected
> 6.1.128-syzkaller-00157-gf31f96bd278e #0 Not tainted
> ------------------------------------------------------
> kswapd0/72 is trying to acquire lock:
> ffff8880945d6998 (&f->sem){+.+.}-{3:3}, at: jffs2_do_clear_inode+0x56/0x570
> fs/jffs2/readinode.c:1419
> 
> but task is already holding lock:
> ffffffff8a68b100 (fs_reclaim){+.+.}-{0:0}, at: balance_pgdat+0xa15/0x1510
> mm/vmscan.c:7173
> 
> which lock already depends on the new lock.
> 
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #1 (fs_reclaim){+.+.}-{0:0}:
>       __fs_reclaim_acquire mm/page_alloc.c:4719 [inline]
>       fs_reclaim_acquire+0x100/0x150 mm/page_alloc.c:4733
>       might_alloc include/linux/sched/mm.h:271 [inline]
>       slab_pre_alloc_hook mm/slab.h:710 [inline]
>       slab_alloc_node mm/slub.c:3318 [inline]
>       slab_alloc mm/slub.c:3406 [inline]
>       __kmem_cache_alloc_lru mm/slub.c:3413 [inline]
>       kmem_cache_alloc+0x43/0x360 mm/slub.c:3422
>       jffs2_do_read_inode+0x300/0x510 fs/jffs2/readinode.c:1372
>       jffs2_iget+0x1bb/0xcb0 fs/jffs2/fs.c:276
>       jffs2_do_fill_super+0x449/0xa60 fs/jffs2/fs.c:575
>       jffs2_fill_super+0x27e/0x370 fs/jffs2/super.c:290
>       mtd_get_sb+0x16f/0x220 drivers/mtd/mtdsuper.c:80
>       mtd_get_sb_by_nr drivers/mtd/mtdsuper.c:111 [inline]
>       get_tree_mtd+0x5ff/0x750 drivers/mtd/mtdsuper.c:164
>       vfs_get_tree+0x8e/0x300 fs/super.c:1573
>       do_new_mount fs/namespace.c:3056 [inline]
>       path_mount+0x6a6/0x1e90 fs/namespace.c:3386
>       do_mount fs/namespace.c:3399 [inline]
>       __do_sys_mount fs/namespace.c:3607 [inline]
>       __se_sys_mount fs/namespace.c:3584 [inline]
>       __x64_sys_mount+0x283/0x300 fs/namespace.c:3584
>       do_syscall_x64 arch/x86/entry/common.c:51 [inline]
>       do_syscall_64+0x35/0x80 arch/x86/entry/common.c:81
>       entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> 
> -> #0 (&f->sem){+.+.}-{3:3}:
>       check_prev_add kernel/locking/lockdep.c:3090 [inline]
>       check_prevs_add kernel/locking/lockdep.c:3209 [inline]
>       validate_chain kernel/locking/lockdep.c:3825 [inline]
>       __lock_acquire+0x2a29/0x5320 kernel/locking/lockdep.c:5049
>       lock_acquire kernel/locking/lockdep.c:5662 [inline]
>       lock_acquire+0x194/0x4b0 kernel/locking/lockdep.c:5627
>       __mutex_lock_common kernel/locking/mutex.c:603 [inline]
>       __mutex_lock+0x14c/0x19f0 kernel/locking/mutex.c:747
>       jffs2_do_clear_inode+0x56/0x570 fs/jffs2/readinode.c:1419
>       evict+0x32c/0x810 fs/inode.c:705
>       dispose_list+0xd7/0x1a0 fs/inode.c:738
>       prune_icache_sb+0xe7/0x150 fs/inode.c:941
>       super_cache_scan+0x38a/0x590 fs/super.c:106
>       do_shrink_slab+0x412/0xa00 mm/vmscan.c:853
>       shrink_slab+0x178/0x670 mm/vmscan.c:1013
>       shrink_node_memcgs mm/vmscan.c:6147 [inline]
>       shrink_node+0x957/0x1fb0 mm/vmscan.c:6176
>       kswapd_shrink_node mm/vmscan.c:6968 [inline]
>       balance_pgdat+0x8ed/0x1510 mm/vmscan.c:7158
>       kswapd+0x5d4/0xb80 mm/vmscan.c:7418
>       kthread+0x2e1/0x3a0 kernel/kthread.c:376
>       ret_from_fork+0x22/0x30 arch/x86/entry/entry_64.S:295
> 
> other info that might help us debug this:
> 
> Possible unsafe locking scenario:
> 
>       CPU0                    CPU1
>       ----                    ----
>  lock(fs_reclaim);
>                               lock(&f->sem);
>                               lock(fs_reclaim);
>  lock(&f->sem);
> 
> *** DEADLOCK ***
> 
> Fix this false positive by using mutex_trylock instead of mutex_lock to
> avoid creating a false locking dependency.
> 
> jffs2_do_crccheck_inode also calls this function, with local mutex,
> which should be safe, but to be extremely sure and to make code more
> future-proof WARN_ON_ONCE was used.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Co-developed-by: Nikita Marushkin <hfggklm@gmail.com>
> Signed-off-by: Nikita Marushkin <hfggklm@gmail.com>
> Signed-off-by: Ilya Shchipletsov <rabbelkin@mail.ru>
> ---
> fs/jffs2/readinode.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c
> index 03b4f99614be..3d2b2e5fc2c5 100644
> --- a/fs/jffs2/readinode.c
> +++ b/fs/jffs2/readinode.c
> @@ -1416,7 +1416,17 @@ void jffs2_do_clear_inode(struct jffs2_sb_info *c, struct
> jffs2_inode_info *f)
> 	int deleted;
> 
> 	jffs2_xattr_delete_inode(c, f->inocache);
> -	mutex_lock(&f->sem);
> +
> +	/*
> +	 * We should be the only ones having a reference to this struct
> +	 * jffs2_inode_info. So the locking is actually unnecessary. Besides,
> +	 * lockdep triggers a false-positive warning on &f->sem here about
> +	 * reclaim circular dependency. Play it safe and bump a warning if
> +	 * this doesn't hold true.
> +	 */
> +	if (WARN_ON_ONCE(!mutex_trylock(&f->sem)))
> +		return;
> +

Hmm, I thought we have lockdep classes or other annotations to fix such issues?
Adding a mutex_trylock/return here feels odd.

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/