linux-kernel - Re: [PATCH] erofs: fix lockdep false positives on initializing erofs_pseudo

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <65d09bbe-9389-4502-1504-8c1557fe5e52@huawei.com>
Date: Thu, 7 Mar 2024 14:46:08 +0800
From: Baokun Li <libaokun1@...wei.com>
To: Jingbo Xu <jefflexu@...ux.alibaba.com>, Gao Xiang
	<hsiangkao@...ux.alibaba.com>, <linux-erofs@...ts.ozlabs.org>
CC: <xiang@...nel.org>, <chao@...nel.org>, <huyue2@...lpad.com>,
	<linux-kernel@...r.kernel.org>, <yangerkun@...wei.com>, <houtao1@...wei.com>,
	<yukuai3@...wei.com>, <chengzhihao1@...wei.com>, Baokun Li
	<libaokun1@...wei.com>
Subject: Re: [PATCH] erofs: fix lockdep false positives on initializing
 erofs_pseudo_mnt

On 2024/3/7 11:41, Jingbo Xu wrote:
> Hi Baokun,
>
> Thanks for catching this!
>
>
> On 3/7/24 10:52 AM, Gao Xiang wrote:
>> Hi Baokun,
>>
>> On 2024/3/7 10:44, Baokun Li wrote:
>>> Lockdep reported the following issue when mounting erofs with a
>>> domain_id:
>>>
>>> ============================================
>>> WARNING: possible recursive locking detected
>>> 6.8.0-rc7-xfstests #521 Not tainted
>>> --------------------------------------------
>>> mount/396 is trying to acquire lock:
>>> ffff907a8aaaa0e0 (&type->s_umount_key#50/1){+.+.}-{3:3},
>>>                          at: alloc_super+0xe3/0x3d0
>>>
>>> but task is already holding lock:
>>> ffff907a8aaa90e0 (&type->s_umount_key#50/1){+.+.}-{3:3},
>>>                          at: alloc_super+0xe3/0x3d0
>>>
>>> other info that might help us debug this:
>>>    Possible unsafe locking scenario:
>>>
>>>          CPU0
>>>          ----
>>>     lock(&type->s_umount_key#50/1);
>>>     lock(&type->s_umount_key#50/1);
>>>
>>>    *** DEADLOCK ***
>>>
>>>    May be due to missing lock nesting notation
>>>
>>> 2 locks held by mount/396:
>>>    #0: ffff907a8aaa90e0 (&type->s_umount_key#50/1){+.+.}-{3:3},
>>>              at: alloc_super+0xe3/0x3d0
>>>    #1: ffffffffc00e6f28 (erofs_domain_list_lock){+.+.}-{3:3},
>>>              at: erofs_fscache_register_fs+0x3d/0x270 [erofs]
>>>
>>> stack backtrace:
>>> CPU: 1 PID: 396 Comm: mount Not tainted 6.8.0-rc7-xfstests #521
>>> Call Trace:
>>>    <TASK>
>>>    dump_stack_lvl+0x64/0xb0
>>>    validate_chain+0x5c4/0xa00
>>>    __lock_acquire+0x6a9/0xd50
>>>    lock_acquire+0xcd/0x2b0
>>>    down_write_nested+0x45/0xd0
>>>    alloc_super+0xe3/0x3d0
>>>    sget_fc+0x62/0x2f0
>>>    vfs_get_super+0x21/0x90
>>>    vfs_get_tree+0x2c/0xf0
>>>    fc_mount+0x12/0x40
>>>    vfs_kern_mount.part.0+0x75/0x90
>>>    kern_mount+0x24/0x40
>>>    erofs_fscache_register_fs+0x1ef/0x270 [erofs]
>>>    erofs_fc_fill_super+0x213/0x380 [erofs]
>>>
>>> This is because the file_system_type of both erofs and the pseudo-mount
>>> point of domain_id is erofs_fs_type, so two successive calls to
>>> alloc_super() are considered to be using the same lock and trigger the
>>> warning above.
>>>
>>> Therefore add a nodev file_system_type named erofs_anon_fs_type to
>>> silence this complaint. In addition, to reduce code coupling, refactor
>>> out the erofs_anon_init_fs_context() and erofs_kill_pseudo_sb() functions
>>> and move the erofs_pseudo_mnt related code to fscache.c.
>>>
>>> Signed-off-by: Baokun Li <libaokun1@...wei.com>
>> IMHO, in the beginning, I'd like to avoid introducing another fs type
>> for erofs to share (meta)data between filesystems since it will cause
>> churn, could we use some alternative way to resolve this?
> Yeah as Gao Xiang said, this is initially intended to avoid introducing
> anothoer file_system_type, say erofs_anon_fs_type.
>
> What we need is actually a method of allocating anonymous inode as a
> sentinel identifying each blob.  There is indeed a global mount, i.e.
> anon_inode_mnt, for allocating anonymous inode/file specifically.  At
> the time the share domain feature is introduced, there's only one
> anonymous inode, i.e. anon_inode_inode, and all the allocated anonymous
> files are bound to this single anon_inode_inode.  Thus we decided to
> implement a erofs internal pseudo mount for this usage.
>
> But I noticed that we can now allocate unique anonymous inodes from
> anon_inode_mnt since commit e7e832c ("fs: add LSM-supporting anon-inode
> interface"), though the new interface is initially for LSM usage.
>
Thank you for your feedback!

If I understand you correctly, you mean to remove erofs_pseudo_mnt
directly to avoid this false positive, and use anon_inode_create_getfile()
to create the required anonymous inode.

-- 
With Best Regards,
Baokun Li
.