[PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

[PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

[Baokun Li via Linux-erofs]

When erofs_kill_sb() is called in block dev based mode, s_bdev may not have
been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it will
be mistaken for fscache mode, and then attempt to free an anon_dev that has
never been allocated, triggering the following warning:

============================================
ida_free called for id=0 which is not allocated.
WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140
Modules linked in:
CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630
RIP: 0010:ida_free+0x134/0x140
Call Trace:
 <TASK>
 erofs_kill_sb+0x81/0x90
 deactivate_locked_super+0x35/0x80
 get_tree_bdev+0x136/0x1e0
 vfs_get_tree+0x2c/0xf0
 do_new_mount+0x190/0x2f0
 [...]
============================================

To avoid this problem, add SB_NODEV to fc->sb_flags after successfully
parsing the fsid, and then the superblock inherits this flag when it is
allocated, so that the sb_flags can be used to distinguish whether it is
in block dev based mode when calling erofs_kill_sb().

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/erofs/super.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index b21bd8f78dc1..7539ce7d64bc 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -520,6 +520,7 @@ static int erofs_fc_parse_param(struct fs_context *fc,
 		ctx->fsid = kstrdup(param->string, GFP_KERNEL);
 		if (!ctx->fsid)
 			return -ENOMEM;
+		fc->sb_flags |= SB_NODEV;
 		break;
 	case Opt_domain_id:
 		kfree(ctx->domain_id);
@@ -706,9 +707,7 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
 
 static int erofs_fc_get_tree(struct fs_context *fc)
 {
-	struct erofs_fs_context *ctx = fc->fs_private;
-
-	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && ctx->fsid)
+	if (fc->sb_flags & SB_NODEV)
 		return get_tree_nodev(fc, erofs_fc_fill_super);
 
 	return get_tree_bdev(fc, erofs_fc_fill_super);
@@ -801,7 +800,7 @@ static void erofs_kill_sb(struct super_block *sb)
 {
 	struct erofs_sb_info *sbi;
 
-	if (erofs_is_fscache_mode(sb))
+	if (sb->s_flags & SB_NODEV)
 		kill_anon_super(sb);
 	else
 		kill_block_super(sb);
-- 
2.31.1

Re: [PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

[Christian Brauner]

On Mon, Apr 15, 2024 at 08:17:46PM +0800, Baokun Li wrote:
> When erofs_kill_sb() is called in block dev based mode, s_bdev may not have
> been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it will
> be mistaken for fscache mode, and then attempt to free an anon_dev that has
> never been allocated, triggering the following warning:
> 
> ============================================
> ida_free called for id=0 which is not allocated.
> WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140
> Modules linked in:
> CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630
> RIP: 0010:ida_free+0x134/0x140
> Call Trace:
>  <TASK>
>  erofs_kill_sb+0x81/0x90
>  deactivate_locked_super+0x35/0x80
>  get_tree_bdev+0x136/0x1e0
>  vfs_get_tree+0x2c/0xf0
>  do_new_mount+0x190/0x2f0
>  [...]
> ============================================
> 
> To avoid this problem, add SB_NODEV to fc->sb_flags after successfully
> parsing the fsid, and then the superblock inherits this flag when it is
> allocated, so that the sb_flags can be used to distinguish whether it is
> in block dev based mode when calling erofs_kill_sb().
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
>  fs/erofs/super.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index b21bd8f78dc1..7539ce7d64bc 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -520,6 +520,7 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>  		ctx->fsid = kstrdup(param->string, GFP_KERNEL);
>  		if (!ctx->fsid)
>  			return -ENOMEM;
> +		fc->sb_flags |= SB_NODEV;

Hm, I wouldn't do it this way. That's an abuse of that flag imho.
Record the information in the erofs_fs_context if you need to.

Re: [PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

[Baokun Li via Linux-erofs]

On 2024/4/15 21:38, Christian Brauner wrote:
> On Mon, Apr 15, 2024 at 08:17:46PM +0800, Baokun Li wrote:
>> When erofs_kill_sb() is called in block dev based mode, s_bdev may not have
>> been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it will
>> be mistaken for fscache mode, and then attempt to free an anon_dev that has
>> never been allocated, triggering the following warning:
>>
>> ============================================
>> ida_free called for id=0 which is not allocated.
>> WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140
>> Modules linked in:
>> CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630
>> RIP: 0010:ida_free+0x134/0x140
>> Call Trace:
>>   <TASK>
>>   erofs_kill_sb+0x81/0x90
>>   deactivate_locked_super+0x35/0x80
>>   get_tree_bdev+0x136/0x1e0
>>   vfs_get_tree+0x2c/0xf0
>>   do_new_mount+0x190/0x2f0
>>   [...]
>> ============================================
>>
>> To avoid this problem, add SB_NODEV to fc->sb_flags after successfully
>> parsing the fsid, and then the superblock inherits this flag when it is
>> allocated, so that the sb_flags can be used to distinguish whether it is
>> in block dev based mode when calling erofs_kill_sb().
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> ---
>>   fs/erofs/super.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index b21bd8f78dc1..7539ce7d64bc 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -520,6 +520,7 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>>   		ctx->fsid = kstrdup(param->string, GFP_KERNEL);
>>   		if (!ctx->fsid)
>>   			return -ENOMEM;
>> +		fc->sb_flags |= SB_NODEV;
> Hm, I wouldn't do it this way. That's an abuse of that flag imho.
> Record the information in the erofs_fs_context if you need to.
Hi Christian!

The problem here is that when mounting erofs, if we have an fsid
then it is not block device based, if we don't have an fsid it is block
device based. So only after we confirmed whether we have an fsid
or not, we can confirm whether we need SB_NODEV or not.

-- 
With Best Regards,
Baokun Li
.

Re: [PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

[Baokun Li via Linux-erofs]

On 2024/4/15 21:38, Christian Brauner wrote:
> On Mon, Apr 15, 2024 at 08:17:46PM +0800, Baokun Li wrote:
>> When erofs_kill_sb() is called in block dev based mode, s_bdev may not have
>> been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it will
>> be mistaken for fscache mode, and then attempt to free an anon_dev that has
>> never been allocated, triggering the following warning:
>>
>> ============================================
>> ida_free called for id=0 which is not allocated.
>> WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140
>> Modules linked in:
>> CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630
>> RIP: 0010:ida_free+0x134/0x140
>> Call Trace:
>>   <TASK>
>>   erofs_kill_sb+0x81/0x90
>>   deactivate_locked_super+0x35/0x80
>>   get_tree_bdev+0x136/0x1e0
>>   vfs_get_tree+0x2c/0xf0
>>   do_new_mount+0x190/0x2f0
>>   [...]
>> ============================================
>>
>> To avoid this problem, add SB_NODEV to fc->sb_flags after successfully
>> parsing the fsid, and then the superblock inherits this flag when it is
>> allocated, so that the sb_flags can be used to distinguish whether it is
>> in block dev based mode when calling erofs_kill_sb().
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> ---
>>   fs/erofs/super.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index b21bd8f78dc1..7539ce7d64bc 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -520,6 +520,7 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>>   		ctx->fsid = kstrdup(param->string, GFP_KERNEL);
>>   		if (!ctx->fsid)
>>   			return -ENOMEM;
>> +		fc->sb_flags |= SB_NODEV;
> Hm, I wouldn't do it this way. That's an abuse of that flag imho.
> Record the information in the erofs_fs_context if you need to.
The stack diagram that triggers the problem is as follows, the call to
erofs_kill_sb() fails before fill_super() has been executed, and we can
only use super_block to determine whether it is currently in nodev
fscahe mode or block device based mode. So if it is recorded in
erofs_fs_context (aka fc->fs_private), we can't access the recorded data
unless we pass fc into erofs_kill_sb() as well.

do_new_mount
  fs_context_for_mount
   alloc_fs_context
    erofs_init_fs_context
     // Mount options have not been parsed yet,
     // it is not clear if there is an fsid
  parse_monolithic_mount_data
   generic_parse_monolithic
    erofs_fc_parse_param
     case Opt_fsid:
       // fc->sb_flags |= SB_NODEV;
  vfs_get_tree
   erofs_fc_get_tree
    get_tree_bdev
     s = sget_dev(fc, dev)
       s = alloc_super()
         s->s_flags = fc->sb_flags;
     setup_bdev_super
       // return error
       sb->s_bdev = bdev // s_bdev is NULL
     deactivate_locked_super
       erofs_kill_sb
         // Block device based mode is considered fscahe mode for nodev
         erofs_is_fscache_mode(sb)
           IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && !sb->s_bdev
          kill_anon_super(sb)
            free_anon_bdev(dev)
              ida_free(&unnamed_dev_ida, MINOR(dev))
              ((int)id < 0)
              WARN(1, "ida_free called for id=%d which is not 
allocated.\n", id);


Thanks!
-- 
With Best Regards,
Baokun Li

Re: [PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

[Gao Xiang]

Hi Christian, Baokun,

On Mon, Apr 15, 2024 at 11:23:58PM +0800, Baokun Li wrote:
> On 2024/4/15 21:38, Christian Brauner wrote:
> > On Mon, Apr 15, 2024 at 08:17:46PM +0800, Baokun Li wrote:
> > > When erofs_kill_sb() is called in block dev based mode, s_bdev may not have
> > > been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it will
> > > be mistaken for fscache mode, and then attempt to free an anon_dev that has
> > > never been allocated, triggering the following warning:
> > > 
> > > ============================================
> > > ida_free called for id=0 which is not allocated.
> > > WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140
> > > Modules linked in:
> > > CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630
> > > RIP: 0010:ida_free+0x134/0x140
> > > Call Trace:
> > >   <TASK>
> > >   erofs_kill_sb+0x81/0x90
> > >   deactivate_locked_super+0x35/0x80
> > >   get_tree_bdev+0x136/0x1e0
> > >   vfs_get_tree+0x2c/0xf0
> > >   do_new_mount+0x190/0x2f0
> > >   [...]
> > > ============================================
> > > 
> > > To avoid this problem, add SB_NODEV to fc->sb_flags after successfully
> > > parsing the fsid, and then the superblock inherits this flag when it is
> > > allocated, so that the sb_flags can be used to distinguish whether it is
> > > in block dev based mode when calling erofs_kill_sb().
> > > 
> > > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> > > ---
> > >   fs/erofs/super.c | 7 +++----
> > >   1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> > > index b21bd8f78dc1..7539ce7d64bc 100644
> > > --- a/fs/erofs/super.c
> > > +++ b/fs/erofs/super.c
> > > @@ -520,6 +520,7 @@ static int erofs_fc_parse_param(struct fs_context *fc,
> > >   		ctx->fsid = kstrdup(param->string, GFP_KERNEL);
> > >   		if (!ctx->fsid)
> > >   			return -ENOMEM;
> > > +		fc->sb_flags |= SB_NODEV;
> > Hm, I wouldn't do it this way. That's an abuse of that flag imho.
> > Record the information in the erofs_fs_context if you need to.
> The stack diagram that triggers the problem is as follows, the call to
> erofs_kill_sb() fails before fill_super() has been executed, and we can
> only use super_block to determine whether it is currently in nodev
> fscahe mode or block device based mode. So if it is recorded in
> erofs_fs_context (aka fc->fs_private), we can't access the recorded data
> unless we pass fc into erofs_kill_sb() as well.
> 

If I understand correctly, from the discussion above, I think
there exists a gap between alloc_super() and sb->s_bdev is set.
But .kill_sb() can be called between them and fc is not passed
into .kill_sb().

I'm not sure how to resolve it in EROFS itself, anyway...

Thanks,
Gao Xiang

Re: [PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

[Baokun Li via Linux-erofs]

On 2024/4/16 8:57, Gao Xiang wrote:
> Hi Christian, Baokun,
>
> On Mon, Apr 15, 2024 at 11:23:58PM +0800, Baokun Li wrote:
>> On 2024/4/15 21:38, Christian Brauner wrote:
>>> On Mon, Apr 15, 2024 at 08:17:46PM +0800, Baokun Li wrote:
>>>> When erofs_kill_sb() is called in block dev based mode, s_bdev may not have
>>>> been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it will
>>>> be mistaken for fscache mode, and then attempt to free an anon_dev that has
>>>> never been allocated, triggering the following warning:
>>>>
>>>> ============================================
>>>> ida_free called for id=0 which is not allocated.
>>>> WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140
>>>> Modules linked in:
>>>> CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630
>>>> RIP: 0010:ida_free+0x134/0x140
>>>> Call Trace:
>>>>    <TASK>
>>>>    erofs_kill_sb+0x81/0x90
>>>>    deactivate_locked_super+0x35/0x80
>>>>    get_tree_bdev+0x136/0x1e0
>>>>    vfs_get_tree+0x2c/0xf0
>>>>    do_new_mount+0x190/0x2f0
>>>>    [...]
>>>> ============================================
>>>>
>>>> To avoid this problem, add SB_NODEV to fc->sb_flags after successfully
>>>> parsing the fsid, and then the superblock inherits this flag when it is
>>>> allocated, so that the sb_flags can be used to distinguish whether it is
>>>> in block dev based mode when calling erofs_kill_sb().
>>>>
>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>>> ---
>>>>    fs/erofs/super.c | 7 +++----
>>>>    1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>>>> index b21bd8f78dc1..7539ce7d64bc 100644
>>>> --- a/fs/erofs/super.c
>>>> +++ b/fs/erofs/super.c
>>>> @@ -520,6 +520,7 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>>>>    		ctx->fsid = kstrdup(param->string, GFP_KERNEL);
>>>>    		if (!ctx->fsid)
>>>>    			return -ENOMEM;
>>>> +		fc->sb_flags |= SB_NODEV;
>>> Hm, I wouldn't do it this way. That's an abuse of that flag imho.
>>> Record the information in the erofs_fs_context if you need to.
>> The stack diagram that triggers the problem is as follows, the call to
>> erofs_kill_sb() fails before fill_super() has been executed, and we can
>> only use super_block to determine whether it is currently in nodev
>> fscahe mode or block device based mode. So if it is recorded in
>> erofs_fs_context (aka fc->fs_private), we can't access the recorded data
>> unless we pass fc into erofs_kill_sb() as well.
>>
> If I understand correctly, from the discussion above, I think
> there exists a gap between alloc_super() and sb->s_bdev is set.
> But .kill_sb() can be called between them and fc is not passed
> into .kill_sb().
>
> I'm not sure how to resolve it in EROFS itself, anyway...
>
> Thanks,
> Gao Xiang
Yes, exactly!

There is no fill_super between alloc_super() and kill_sb(), so erofs has
no way to set a flag for the superblock directly. The only way I can
think of now is to modify fc->sb_flags in erofs_fc_parse_param()
to indirectly set a flag for the superblock.

Cheers,
Baokun

Re: [PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 287 bytes --]

> > I'm not sure how to resolve it in EROFS itself, anyway...

Instead of allocating the erofs_sb_info in fill_super() allocate it
during erofs_get_tree() and then you can ensure that you always have the
info you need available during erofs_kill_sb(). See the appended
(untested) patch.

[-- Attachment #2: 0001-erofs-reliably-distinguish-block-based-and-fscache-m.patch --]
[-- Type: text/x-diff, Size: 4053 bytes --]

From e4f586a41748b6edc05aca36d49b7b39e55def81 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Mon, 15 Apr 2024 20:17:46 +0800
Subject: [PATCH] erofs: reliably distinguish block based and fscache mode

When erofs_kill_sb() is called in block dev based mode, s_bdev may not have
been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it will
be mistaken for fscache mode, and then attempt to free an anon_dev that has
never been allocated, triggering the following warning:

============================================
ida_free called for id=0 which is not allocated.
WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140
Modules linked in:
CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630
RIP: 0010:ida_free+0x134/0x140
Call Trace:
 <TASK>
 erofs_kill_sb+0x81/0x90
 deactivate_locked_super+0x35/0x80
 get_tree_bdev+0x136/0x1e0
 vfs_get_tree+0x2c/0xf0
 do_new_mount+0x190/0x2f0
 [...]
============================================

Instead of allocating the erofs_sb_info in fill_super() allocate it
during erofs_get_tree() and ensure that erofs can always have the info
available during erofs_kill_sb().

Signed-off-by: Baokun Li <libaokun1@huawei.com>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/erofs/super.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index c0eb139adb07..4ed80154edf8 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -581,7 +581,7 @@ static const struct export_operations erofs_export_ops = {
 static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	struct inode *inode;
-	struct erofs_sb_info *sbi;
+	struct erofs_sb_info *sbi = EROFS_SB(sb);
 	struct erofs_fs_context *ctx = fc->fs_private;
 	int err;
 
@@ -590,15 +590,10 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
 	sb->s_op = &erofs_sops;
 
-	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
-	if (!sbi)
-		return -ENOMEM;
-
 	sb->s_fs_info = sbi;
 	sbi->opt = ctx->opt;
 	sbi->devs = ctx->devs;
 	ctx->devs = NULL;
-	sbi->fsid = ctx->fsid;
 	ctx->fsid = NULL;
 	sbi->domain_id = ctx->domain_id;
 	ctx->domain_id = NULL;
@@ -707,8 +702,15 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
 static int erofs_fc_get_tree(struct fs_context *fc)
 {
 	struct erofs_fs_context *ctx = fc->fs_private;
+	struct erofs_sb_info *sbi;
+
+	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
+	if (!sbi)
+		return -ENOMEM;
 
-	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && ctx->fsid)
+	fc->s_fs_info = sbi;
+	sbi->fsid = ctx->fsid;
+	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && sbi->fsid)
 		return get_tree_nodev(fc, erofs_fc_fill_super);
 
 	return get_tree_bdev(fc, erofs_fc_fill_super);
@@ -762,11 +764,15 @@ static void erofs_free_dev_context(struct erofs_dev_context *devs)
 static void erofs_fc_free(struct fs_context *fc)
 {
 	struct erofs_fs_context *ctx = fc->fs_private;
+	struct erofs_sb_info *sbi = fc->s_fs_info;
 
 	erofs_free_dev_context(ctx->devs);
 	kfree(ctx->fsid);
 	kfree(ctx->domain_id);
 	kfree(ctx);
+
+	if (sbi)
+		kfree(sbi);
 }
 
 static const struct fs_context_operations erofs_context_ops = {
@@ -783,6 +789,7 @@ static int erofs_init_fs_context(struct fs_context *fc)
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
+
 	ctx->devs = kzalloc(sizeof(struct erofs_dev_context), GFP_KERNEL);
 	if (!ctx->devs) {
 		kfree(ctx);
@@ -799,17 +806,13 @@ static int erofs_init_fs_context(struct fs_context *fc)
 
 static void erofs_kill_sb(struct super_block *sb)
 {
-	struct erofs_sb_info *sbi;
+	struct erofs_sb_info *sbi = EROFS_SB(sb);
 
-	if (erofs_is_fscache_mode(sb))
+	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && sbi->fsid)
 		kill_anon_super(sb);
 	else
 		kill_block_super(sb);
 
-	sbi = EROFS_SB(sb);
-	if (!sbi)
-		return;
-
 	erofs_free_dev_context(sbi->devs);
 	fs_put_dax(sbi->dax_dev, NULL);
 	erofs_fscache_unregister_fs(sb);
-- 
2.43.0

Re: [PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

[Gao Xiang]

On Tue, Apr 16, 2024 at 02:35:08PM +0200, Christian Brauner wrote:
> > > I'm not sure how to resolve it in EROFS itself, anyway...
> 
> Instead of allocating the erofs_sb_info in fill_super() allocate it
> during erofs_get_tree() and then you can ensure that you always have the
> info you need available during erofs_kill_sb(). See the appended
> (untested) patch.

Hi Christian,

Yeah, that is a good way I think.  Although sbi will be allocated
unconditionally instead but that is minor.

I'm on OSSNA this week, will test this patch more when returning.

Hi Baokun,

Could you also check this on your side?

Thanks,
Gao Xiang


> From e4f586a41748b6edc05aca36d49b7b39e55def81 Mon Sep 17 00:00:00 2001
> From: Christian Brauner <brauner@kernel.org>
> Date: Mon, 15 Apr 2024 20:17:46 +0800
> Subject: [PATCH] erofs: reliably distinguish block based and fscache mode
> 
> When erofs_kill_sb() is called in block dev based mode, s_bdev may not have
> been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it will
> be mistaken for fscache mode, and then attempt to free an anon_dev that has
> never been allocated, triggering the following warning:
> 
> ============================================
> ida_free called for id=0 which is not allocated.
> WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140
> Modules linked in:
> CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630
> RIP: 0010:ida_free+0x134/0x140
> Call Trace:
>  <TASK>
>  erofs_kill_sb+0x81/0x90
>  deactivate_locked_super+0x35/0x80
>  get_tree_bdev+0x136/0x1e0
>  vfs_get_tree+0x2c/0xf0
>  do_new_mount+0x190/0x2f0
>  [...]
> ============================================
> 
> Instead of allocating the erofs_sb_info in fill_super() allocate it
> during erofs_get_tree() and ensure that erofs can always have the info
> available during erofs_kill_sb().
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/erofs/super.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index c0eb139adb07..4ed80154edf8 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -581,7 +581,7 @@ static const struct export_operations erofs_export_ops = {
>  static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>  	struct inode *inode;
> -	struct erofs_sb_info *sbi;
> +	struct erofs_sb_info *sbi = EROFS_SB(sb);
>  	struct erofs_fs_context *ctx = fc->fs_private;
>  	int err;
>  
> @@ -590,15 +590,10 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
>  	sb->s_maxbytes = MAX_LFS_FILESIZE;
>  	sb->s_op = &erofs_sops;
>  
> -	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> -	if (!sbi)
> -		return -ENOMEM;
> -
>  	sb->s_fs_info = sbi;
>  	sbi->opt = ctx->opt;
>  	sbi->devs = ctx->devs;
>  	ctx->devs = NULL;
> -	sbi->fsid = ctx->fsid;
>  	ctx->fsid = NULL;
>  	sbi->domain_id = ctx->domain_id;
>  	ctx->domain_id = NULL;
> @@ -707,8 +702,15 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
>  static int erofs_fc_get_tree(struct fs_context *fc)
>  {
>  	struct erofs_fs_context *ctx = fc->fs_private;
> +	struct erofs_sb_info *sbi;
> +
> +	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> +	if (!sbi)
> +		return -ENOMEM;
>  
> -	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && ctx->fsid)
> +	fc->s_fs_info = sbi;
> +	sbi->fsid = ctx->fsid;
> +	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && sbi->fsid)
>  		return get_tree_nodev(fc, erofs_fc_fill_super);
>  
>  	return get_tree_bdev(fc, erofs_fc_fill_super);
> @@ -762,11 +764,15 @@ static void erofs_free_dev_context(struct erofs_dev_context *devs)
>  static void erofs_fc_free(struct fs_context *fc)
>  {
>  	struct erofs_fs_context *ctx = fc->fs_private;
> +	struct erofs_sb_info *sbi = fc->s_fs_info;
>  
>  	erofs_free_dev_context(ctx->devs);
>  	kfree(ctx->fsid);
>  	kfree(ctx->domain_id);
>  	kfree(ctx);
> +
> +	if (sbi)
> +		kfree(sbi);
>  }
>  
>  static const struct fs_context_operations erofs_context_ops = {
> @@ -783,6 +789,7 @@ static int erofs_init_fs_context(struct fs_context *fc)
>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>  	if (!ctx)
>  		return -ENOMEM;
> +
>  	ctx->devs = kzalloc(sizeof(struct erofs_dev_context), GFP_KERNEL);
>  	if (!ctx->devs) {
>  		kfree(ctx);
> @@ -799,17 +806,13 @@ static int erofs_init_fs_context(struct fs_context *fc)
>  
>  static void erofs_kill_sb(struct super_block *sb)
>  {
> -	struct erofs_sb_info *sbi;
> +	struct erofs_sb_info *sbi = EROFS_SB(sb);
>  
> -	if (erofs_is_fscache_mode(sb))
> +	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && sbi->fsid)
>  		kill_anon_super(sb);
>  	else
>  		kill_block_super(sb);
>  
> -	sbi = EROFS_SB(sb);
> -	if (!sbi)
> -		return;
> -
>  	erofs_free_dev_context(sbi->devs);
>  	fs_put_dax(sbi->dax_dev, NULL);
>  	erofs_fscache_unregister_fs(sb);
> -- 
> 2.43.0
> 

Re: [PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

[Baokun Li via Linux-erofs]

On 2024/4/16 22:49, Gao Xiang wrote:
> On Tue, Apr 16, 2024 at 02:35:08PM +0200, Christian Brauner wrote:
>>>> I'm not sure how to resolve it in EROFS itself, anyway...
>> Instead of allocating the erofs_sb_info in fill_super() allocate it
>> during erofs_get_tree() and then you can ensure that you always have the
>> info you need available during erofs_kill_sb(). See the appended
>> (untested) patch.
> Hi Christian,
>
> Yeah, that is a good way I think.  Although sbi will be allocated
> unconditionally instead but that is minor.
>
> I'm on OSSNA this week, will test this patch more when returning.
>
> Hi Baokun,
>
> Could you also check this on your side?
>
> Thanks,
> Gao Xiang
Hi Xiang,

This patch does fix the initial problem.


Hi Christian,

Thanks for the patch, this is a good idea. Just with nits below.
Otherwise feel free to add.

Reviewed-and-tested-by: Baokun Li <libaokun1@huawei.com>
>
>>  From e4f586a41748b6edc05aca36d49b7b39e55def81 Mon Sep 17 00:00:00 2001
>> From: Christian Brauner <brauner@kernel.org>
>> Date: Mon, 15 Apr 2024 20:17:46 +0800
>> Subject: [PATCH] erofs: reliably distinguish block based and fscache mode
>>
SNIP

>>
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index c0eb139adb07..4ed80154edf8 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -581,7 +581,7 @@ static const struct export_operations erofs_export_ops = {
>>   static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
>>   {
>>   	struct inode *inode;
>> -	struct erofs_sb_info *sbi;
>> +	struct erofs_sb_info *sbi = EROFS_SB(sb);
>>   	struct erofs_fs_context *ctx = fc->fs_private;
>>   	int err;
>>   
>> @@ -590,15 +590,10 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
>>   	sb->s_maxbytes = MAX_LFS_FILESIZE;
>>   	sb->s_op = &erofs_sops;
>>   
>> -	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
>> -	if (!sbi)
>> -		return -ENOMEM;
>> -
>>   	sb->s_fs_info = sbi;
This line is no longer needed.
>>   	sbi->opt = ctx->opt;
>>   	sbi->devs = ctx->devs;
>>   	ctx->devs = NULL;
>> -	sbi->fsid = ctx->fsid;
>>   	ctx->fsid = NULL;
>>   	sbi->domain_id = ctx->domain_id;
>>   	ctx->domain_id = NULL;
Since erofs_sb_info is now allocated in erofs_fc_get_tree(), why not
encapsulate the above lines as erofs_ctx_to_info() helper function
to be called in erofs_fc_get_tree()？Then erofs_fc_fill_super() wouldn't
have to use erofs_fs_context and would prevent the fsid from being
freed twice.
>> @@ -707,8 +702,15 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
>>   static int erofs_fc_get_tree(struct fs_context *fc)
>>   {
>>   	struct erofs_fs_context *ctx = fc->fs_private;
>> +	struct erofs_sb_info *sbi;
>> +
>> +	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
>> +	if (!sbi)
>> +		return -ENOMEM;
>>   
>> -	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && ctx->fsid)
>> +	fc->s_fs_info = sbi;
>> +	sbi->fsid = ctx->fsid;
Here ctx->fsid is not set to null, so fsid may be freed twice.
>> +	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && sbi->fsid)
>>   		return get_tree_nodev(fc, erofs_fc_fill_super);
>>   
>>   	return get_tree_bdev(fc, erofs_fc_fill_super);
>> @@ -762,11 +764,15 @@ static void erofs_free_dev_context(struct erofs_dev_context *devs)
>>   static void erofs_fc_free(struct fs_context *fc)
>>   {
>>   	struct erofs_fs_context *ctx = fc->fs_private;
>> +	struct erofs_sb_info *sbi = fc->s_fs_info;
>>   
>>   	erofs_free_dev_context(ctx->devs);
>>   	kfree(ctx->fsid);
>>   	kfree(ctx->domain_id);
>>   	kfree(ctx);
>> +
>> +	if (sbi)
>> +		kfree(sbi);
There's no need to check sbi, kfree does.
>>   }
>>   
>>   static const struct fs_context_operations erofs_context_ops = {
>> @@ -783,6 +789,7 @@ static int erofs_init_fs_context(struct fs_context *fc)
>>   	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>>   	if (!ctx)
>>   		return -ENOMEM;
>> +
>>   	ctx->devs = kzalloc(sizeof(struct erofs_dev_context), GFP_KERNEL);
>>   	if (!ctx->devs) {
>>   		kfree(ctx);
>> @@ -799,17 +806,13 @@ static int erofs_init_fs_context(struct fs_context *fc)
>>   
>>   static void erofs_kill_sb(struct super_block *sb)
>>   {
>> -	struct erofs_sb_info *sbi;
>> +	struct erofs_sb_info *sbi = EROFS_SB(sb);
>>   
>> -	if (erofs_is_fscache_mode(sb))
>> +	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && sbi->fsid)
>>   		kill_anon_super(sb);
>>   	else
>>   		kill_block_super(sb);
>>   
>> -	sbi = EROFS_SB(sb);
>> -	if (!sbi)
>> -		return;
>> -
>>   	erofs_free_dev_context(sbi->devs);
>>   	fs_put_dax(sbi->dax_dev, NULL);
>>   	erofs_fscache_unregister_fs(sb);
>> -- 
>> 2.43.0
>>

Re: [PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

[Gao Xiang]

On Wed, Apr 17, 2024 at 10:59:53AM +0800, Baokun Li wrote:
> On 2024/4/16 22:49, Gao Xiang wrote:
> > On Tue, Apr 16, 2024 at 02:35:08PM +0200, Christian Brauner wrote:
> > > > > I'm not sure how to resolve it in EROFS itself, anyway...
> > > Instead of allocating the erofs_sb_info in fill_super() allocate it
> > > during erofs_get_tree() and then you can ensure that you always have the
> > > info you need available during erofs_kill_sb(). See the appended
> > > (untested) patch.
> > Hi Christian,
> > 
> > Yeah, that is a good way I think.  Although sbi will be allocated
> > unconditionally instead but that is minor.
> > 
> > I'm on OSSNA this week, will test this patch more when returning.
> > 
> > Hi Baokun,
> > 
> > Could you also check this on your side?
> > 
> > Thanks,
> > Gao Xiang
> Hi Xiang,
> 
> This patch does fix the initial problem.
> 
> 
> Hi Christian,
> 
> Thanks for the patch, this is a good idea. Just with nits below.
> Otherwise feel free to add.
> 
> Reviewed-and-tested-by: Baokun Li <libaokun1@huawei.com>
> > 
> > >  From e4f586a41748b6edc05aca36d49b7b39e55def81 Mon Sep 17 00:00:00 2001
> > > From: Christian Brauner <brauner@kernel.org>
> > > Date: Mon, 15 Apr 2024 20:17:46 +0800
> > > Subject: [PATCH] erofs: reliably distinguish block based and fscache mode
> > > 
> SNIP
> 
> > > 
> > > diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> > > index c0eb139adb07..4ed80154edf8 100644
> > > --- a/fs/erofs/super.c
> > > +++ b/fs/erofs/super.c
> > > @@ -581,7 +581,7 @@ static const struct export_operations erofs_export_ops = {
> > >   static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
> > >   {
> > >   	struct inode *inode;
> > > -	struct erofs_sb_info *sbi;
> > > +	struct erofs_sb_info *sbi = EROFS_SB(sb);
> > >   	struct erofs_fs_context *ctx = fc->fs_private;
> > >   	int err;
> > > @@ -590,15 +590,10 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
> > >   	sb->s_maxbytes = MAX_LFS_FILESIZE;
> > >   	sb->s_op = &erofs_sops;
> > > -	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> > > -	if (!sbi)
> > > -		return -ENOMEM;
> > > -
> > >   	sb->s_fs_info = sbi;
> This line is no longer needed.
> > >   	sbi->opt = ctx->opt;
> > >   	sbi->devs = ctx->devs;
> > >   	ctx->devs = NULL;
> > > -	sbi->fsid = ctx->fsid;
> > >   	ctx->fsid = NULL;
> > >   	sbi->domain_id = ctx->domain_id;
> > >   	ctx->domain_id = NULL;
> Since erofs_sb_info is now allocated in erofs_fc_get_tree(), why not
> encapsulate the above lines as erofs_ctx_to_info() helper function
> to be called in erofs_fc_get_tree()？Then erofs_fc_fill_super() wouldn't
> have to use erofs_fs_context and would prevent the fsid from being
> freed twice.

Hi Baokun,

I'm not sure if Christian has enough time to polish the whole
codebase (I'm happy if do so).  Basically, that is just a hint
to the issue, if you have more time, I guess you could also help
revive this patch together (also because you also have a real
EROFS test environment).

Let me also check this next week after OSSNA travelling.

Thanks,
Gao Xiang

Re: [PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

[Baokun Li via Linux-erofs]

On 2024/4/17 11:11, Gao Xiang wrote:
> On Wed, Apr 17, 2024 at 10:59:53AM +0800, Baokun Li wrote:
>> On 2024/4/16 22:49, Gao Xiang wrote:
>>> On Tue, Apr 16, 2024 at 02:35:08PM +0200, Christian Brauner wrote:
>>>>>> I'm not sure how to resolve it in EROFS itself, anyway...
>>>> Instead of allocating the erofs_sb_info in fill_super() allocate it
>>>> during erofs_get_tree() and then you can ensure that you always have the
>>>> info you need available during erofs_kill_sb(). See the appended
>>>> (untested) patch.
>>> Hi Christian,
>>>
>>> Yeah, that is a good way I think.  Although sbi will be allocated
>>> unconditionally instead but that is minor.
>>>
>>> I'm on OSSNA this week, will test this patch more when returning.
>>>
>>> Hi Baokun,
>>>
>>> Could you also check this on your side?
>>>
>>> Thanks,
>>> Gao Xiang
>> Hi Xiang,
>>
>> This patch does fix the initial problem.
>>
>>
>> Hi Christian,
>>
>> Thanks for the patch, this is a good idea. Just with nits below.
>> Otherwise feel free to add.
>>
>> Reviewed-and-tested-by: Baokun Li <libaokun1@huawei.com>
>>>>   From e4f586a41748b6edc05aca36d49b7b39e55def81 Mon Sep 17 00:00:00 2001
>>>> From: Christian Brauner <brauner@kernel.org>
>>>> Date: Mon, 15 Apr 2024 20:17:46 +0800
>>>> Subject: [PATCH] erofs: reliably distinguish block based and fscache mode
>>>>
>> SNIP
>>
>>>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>>>> index c0eb139adb07..4ed80154edf8 100644
>>>> --- a/fs/erofs/super.c
>>>> +++ b/fs/erofs/super.c
>>>> @@ -581,7 +581,7 @@ static const struct export_operations erofs_export_ops = {
>>>>    static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
>>>>    {
>>>>    	struct inode *inode;
>>>> -	struct erofs_sb_info *sbi;
>>>> +	struct erofs_sb_info *sbi = EROFS_SB(sb);
>>>>    	struct erofs_fs_context *ctx = fc->fs_private;
>>>>    	int err;
>>>> @@ -590,15 +590,10 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
>>>>    	sb->s_maxbytes = MAX_LFS_FILESIZE;
>>>>    	sb->s_op = &erofs_sops;
>>>> -	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
>>>> -	if (!sbi)
>>>> -		return -ENOMEM;
>>>> -
>>>>    	sb->s_fs_info = sbi;
>> This line is no longer needed.
>>>>    	sbi->opt = ctx->opt;
>>>>    	sbi->devs = ctx->devs;
>>>>    	ctx->devs = NULL;
>>>> -	sbi->fsid = ctx->fsid;
>>>>    	ctx->fsid = NULL;
>>>>    	sbi->domain_id = ctx->domain_id;
>>>>    	ctx->domain_id = NULL;
>> Since erofs_sb_info is now allocated in erofs_fc_get_tree(), why not
>> encapsulate the above lines as erofs_ctx_to_info() helper function
>> to be called in erofs_fc_get_tree()？Then erofs_fc_fill_super() wouldn't
>> have to use erofs_fs_context and would prevent the fsid from being
>> freed twice.
> Hi Baokun,
>
> I'm not sure if Christian has enough time to polish the whole
> codebase (I'm happy if do so).  Basically, that is just a hint
> to the issue, if you have more time, I guess you could also help
> revive this patch together (also because you also have a real
> EROFS test environment).
>
> Let me also check this next week after OSSNA travelling.
>
> Thanks,
> Gao Xiang

Hi Xiang,

Ok, then I will polish the patch and send it out as a v2.

Thanks,
Baokun