linux-kernel - Re: [PATCH] erofs: set SB_NODEV sb

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: <Zh6QC0++kpUUL5nf@debian>
Date: Tue, 16 Apr 2024 22:49:47 +0800
From: Gao Xiang <xiang@...nel.org>
To: Christian Brauner <brauner@...nel.org>
Cc: Baokun Li <libaokun1@...wei.com>, xiang@...nel.org,
	linux-erofs@...ts.ozlabs.org, chao@...nel.org, huyue2@...lpad.com,
	jefflexu@...ux.alibaba.com, viro@...iv.linux.org.uk,
	linux-kernel@...r.kernel.org, yangerkun@...wei.com,
	houtao1@...wei.com
Subject: Re: [PATCH] erofs: set SB_NODEV sb_flags when mounting with fsid

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@...nel.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@...wei.com>
> Signed-off-by: Christian Brauner <brauner@...nel.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
>