Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [External] [PATCH] erofs: check the uniqueness of fsid in shared
 domain in advance
To:     Jia Zhu <zhujia.zj@bytedance.com>, linux-erofs@lists.ozlabs.org
Cc:     Gao Xiang <xiang@kernel.org>, Chao Yu <chao@kernel.org>,
        Yue Hu <huyue2@coolpad.com>,
        Jeffle Xu <jefflexu@linux.alibaba.com>, houtao1@huawei.com,
        linux-kernel@vger.kernel.org
References: <20221125074057.2229083-1-houtao@huaweicloud.com>
 <49b2b8e4-39d0-983c-23c6-f18232a7dff3@bytedance.com>
From:   Hou Tao <houtao@huaweicloud.com>
Message-ID: <86bf6991-d46b-be95-06a7-87829e8c5e33@huaweicloud.com>
Date:   Fri, 25 Nov 2022 16:39:47 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <49b2b8e4-39d0-983c-23c6-f18232a7dff3@bytedance.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

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@huawei.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@huawei.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@bytedance.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;
> .