linux-kernel - Re: [External] [PATCH] erofs: check the uniqueness of fsid in shared domain in advance

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 for Android: free password hash cracker in your pocket

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

Message-ID: <86bf6991-d46b-be95-06a7-87829e8c5e33@huaweicloud.com>
Date:   Fri, 25 Nov 2022 16:39:47 +0800
From:   Hou Tao <houtao@...weicloud.com>
To:     Jia Zhu <zhujia.zj@...edance.com>, linux-erofs@...ts.ozlabs.org
Cc:     Gao Xiang <xiang@...nel.org>, Chao Yu <chao@...nel.org>,
        Yue Hu <huyue2@...lpad.com>,
        Jeffle Xu <jefflexu@...ux.alibaba.com>, houtao1@...wei.com,
        linux-kernel@...r.kernel.org
Subject: Re: [External] [PATCH] erofs: check the uniqueness of fsid in shared
 domain in advance

Hi,

On 11/25/2022 4:25 PM, Jia Zhu wrote:
> Hi Tao,
> We've noticed this warning during development. It seems not a bug, so
> I ignored that.
> Many thanks for cacthing and clearing up the annoying warning.
Yes, it is not a bug. The duplicated mount will fail with -EEXIST even without
the patch. But it is a bit scary to see such warning in dmesg, so just fix it.
>
> 在 2022/11/25 15:40, Hou Tao 写道:
>> From: Hou Tao <houtao1@...wei.com>
>>
>> When shared domain is enabled, doing mount twice with the same fsid and
>> domain_id will trigger sysfs warning as shown below:
>>
>>   sysfs: cannot create duplicate filename '/fs/erofs/d0,meta.bin'
>>   CPU: 15 PID: 1051 Comm: mount Not tainted 6.1.0-rc6+ #1
>>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>>   Call Trace:
>>    <TASK>
>>    dump_stack_lvl+0x38/0x49
>>    dump_stack+0x10/0x12
>>    sysfs_warn_dup.cold+0x17/0x27
>>    sysfs_create_dir_ns+0xb8/0xd0
>>    kobject_add_internal+0xb1/0x240
>>    kobject_init_and_add+0x71/0xa0
>>    erofs_register_sysfs+0x89/0x110
>>    erofs_fc_fill_super+0x98c/0xaf0
>>    vfs_get_super+0x7d/0x100
>>    get_tree_nodev+0x16/0x20
>>    erofs_fc_get_tree+0x20/0x30
>>    vfs_get_tree+0x24/0xb0
>>    path_mount+0x2fa/0xa90
>>    do_mount+0x7c/0xa0
>>    __x64_sys_mount+0x8b/0xe0
>>    do_syscall_64+0x30/0x60
>>    entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>
>> The reason is erofs_fscache_register_cookie() doesn't guarantee the primary
>> data blob (aka fsid) is unique in the shared domain and
>> erofs_register_sysfs() invoked by the second mount will fail due to the
>> duplicated fsid in the shared domain and report warning.
>>
>> It would be better to check the uniqueness of fsid before doing
>> erofs_register_sysfs(), so adding a new flags parameter for
>> erofs_fscache_register_cookie() and doing the uniqueness check if
>> EROFS_REG_COOKIE_NEED_NOEXIST is enabled.
>>
>> After the patch, the error in dmesg for the duplicated mount would be:
>>
>>   erofs: ...: erofs_domain_register_cookie: XX already exists in domain YY
>>
>> Signed-off-by: Hou Tao <houtao1@...wei.com>
SNIP
>>   diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
>> index 05dc68627722..a5f8c8fcdd17 100644
>> --- a/fs/erofs/internal.h
>> +++ b/fs/erofs/internal.h
>> @@ -604,13 +604,18 @@ static inline int z_erofs_load_lzma_config(struct
>> super_block *sb,
>>   }
>>   #endif    /* !CONFIG_EROFS_FS_ZIP */
>>   +/* flags for erofs_fscache_register_cookie() */
>> +#define EROFS_REG_COOKIE_NEED_INODE 1
>> +#define EROFS_REG_COOKIE_NEED_NOEXIST 2
> How about using TAB to align the columns?
> Apart from this, LGTM.
> Reviewed-by: Jia Zhu <zhujia.zj@...edance.com>
> Thanks.
Thanks for the review. Will do the alignment in v2.
>> +
>>   /* fscache.c */
>>   #ifdef CONFIG_EROFS_FS_ONDEMAND
>>   int erofs_fscache_register_fs(struct super_block *sb);
>>   void erofs_fscache_unregister_fs(struct super_block *sb);
>>     struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
>> -                             char *name, bool need_inode);
>> +                            char *name,
>> +                            unsigned int flags);
>>   void erofs_fscache_unregister_cookie(struct erofs_fscache *fscache);
>>     extern const struct address_space_operations erofs_fscache_access_aops;
>> @@ -623,7 +628,8 @@ static inline void erofs_fscache_unregister_fs(struct
>> super_block *sb) {}
>>     static inline
>>   struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
>> -                             char *name, bool need_inode)
>> +                             char *name,
>> +                             unsigned int flags)
>>   {
>>       return ERR_PTR(-EOPNOTSUPP);
>>   }
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index 1c7dcca702b3..481788c24a68 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -245,7 +245,7 @@ static int erofs_init_device(struct erofs_buf *buf,
>> struct super_block *sb,
>>       }
>>         if (erofs_is_fscache_mode(sb)) {
>> -        fscache = erofs_fscache_register_cookie(sb, dif->path, false);
>> +        fscache = erofs_fscache_register_cookie(sb, dif->path, 0);
>>           if (IS_ERR(fscache))
>>               return PTR_ERR(fscache);
>>           dif->fscache = fscache;
> .