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

From: Baokun Li
Date: Tue Apr 16 2024 - 23:00:06 EST


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@xxxxxxxxxx>

From e4f586a41748b6edc05aca36d49b7b39e55def81 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@xxxxxxxxxx>
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