Hi Christian, On 2026/7/23 00:11, Christian Brauner wrote:
Allow userspace to pass an already-opened file descriptor as the mount source instead of a path string. This is useful for tools that already hold an fd to the image, such as composefs reusing an existing erofs backing file.Signed-off-by: Giuseppe Scrivano <[email protected]> diff --git a/fs/erofs/super.c b/fs/erofs/super.c index 9d8f862f309f..bc55be84d945 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -386,6 +386,7 @@ static void erofs_default_options(struct erofs_sb_info *sbi) enum { Opt_user_xattr, Opt_acl, Opt_cache_strategy, Opt_dax, Opt_dax_enum, Opt_device, Opt_domain_id, Opt_directio, Opt_fsoffset, Opt_inode_share, + Opt_source, };static const struct constant_table erofs_param_cache_strategy[] = {@@ -402,17 +403,18 @@ static const struct constant_table erofs_dax_param_enums[] = { };static const struct fs_parameter_spec erofs_fs_parameters[] = {- fsparam_flag_no("user_xattr", Opt_user_xattr), - fsparam_flag_no("acl", Opt_acl), - fsparam_enum("cache_strategy", Opt_cache_strategy, + fsparam_flag_no("user_xattr", Opt_user_xattr), + fsparam_flag_no("acl", Opt_acl), + fsparam_enum("cache_strategy", Opt_cache_strategy, erofs_param_cache_strategy), - fsparam_flag("dax", Opt_dax), - fsparam_enum("dax", Opt_dax_enum, erofs_dax_param_enums), - fsparam_string("device", Opt_device), - fsparam_string("domain_id", Opt_domain_id), - fsparam_flag_no("directio", Opt_directio), - fsparam_u64("fsoffset", Opt_fsoffset), - fsparam_flag("inode_share", Opt_inode_share), + fsparam_flag("dax", Opt_dax), + fsparam_enum("dax", Opt_dax_enum, erofs_dax_param_enums), + fsparam_string("device", Opt_device), + fsparam_string("domain_id", Opt_domain_id), + fsparam_flag_no("directio", Opt_directio), + fsparam_u64("fsoffset", Opt_fsoffset), + fsparam_flag("inode_share", Opt_inode_share), + fsparam_file_or_string("source", Opt_source), {} };@@ -437,6 +439,40 @@ static bool erofs_fc_set_dax_mode(struct fs_context *fc, unsigned int mode)return false; }+static int erofs_fc_parse_source(struct fs_context *fc,+ struct fs_parameter *param) +{ + struct erofs_sb_info *sbi = fc->s_fs_info; + + if (fc->source || sbi->dif0.file) + return invalf(fc, "Multiple sources"); + + switch (param->type) { + case fs_value_is_string: + fc->source = param->string; + param->string = NULL; + return 0; + case fs_value_is_file: {Afaict this is just fsparam_file_or_string()?+ char *buf __free(kfree) = kmalloc(PATH_MAX, GFP_KERNEL); + char *p; + + if (!buf) + return -ENOMEM; + p = file_path(param->file, buf, PATH_MAX); + if (IS_ERR(p)) + return PTR_ERR(p); + fc->source = kstrdup(p, GFP_KERNEL); + if (!fc->source) + return -ENOMEM;Hm, hm, hm... How is that reliable? So iirc this is then passed to: static int erofs_fc_get_tree(struct fs_context *fc) { int ret; ret = get_tree_bdev_flags(fc, erofs_fc_fill_super, IS_ENABLED(CONFIG_EROFS_FS_BACKED_BY_FILE) ? GET_TREE_BDEV_QUIET_LOOKUP : 0); if (IS_ENABLED(CONFIG_EROFS_FS_BACKED_BY_FILE) && ret == -ENOTBLK) { struct erofs_sb_info *sbi = fc->s_fs_info; struct file *file; if (!fc->source) return invalf(fc, "No source specified"); file = filp_open(fc->source, O_RDONLY | O_LARGEFILE, 0); if (IS_ERR(file)) return PTR_ERR(file); sbi->dif0.file = file; if (S_ISREG(file_inode(sbi->dif0.file)->i_mode) && sbi->dif0.file->f_mapping->a_ops->read_folio) return get_tree_nodev(fc, erofs_fc_fill_super); } return ret; } which then reopens the file or looks up the block device. So the only way this is _vaguely_ (and really _very vaguely_) safe is if userspace keeps at least the file descriptor open until the filesystem has been mounted.
I'm not quite sure if I catched the point, I think Giuseppe's patch here tried to record `file` into `sbi->dif0.file` (which indicates the primary "device" later.) And if `sbi->dif0.file` is set up by erofs_fc_parse_source(), erofs_fc_get_tree() will just use `sbi->dif0.file` instead of `fc->source` according to this patch. The reason why `fc->source` is set was discussed in the thread of the previous version suggested by Aleksa.
If they close it before this means you can mount something completely different. The other thing is even if they keep the fd open someone could just rename the damn thing and fc->source ends up pointing somwhere completely different. The could switch namespaces as well in some circumstances and then it points again into wherever. I've played with that fd idea before. The only way to make this work correctly is if you plumb this down into get_tree_nodev()
fc->source in this case has no use in erofs_fc_get_tree() (`fc->source` is just used for mountinfo for example), `sbi->dif0.file` works instead I hope I don't misunderstand something. Thanks, Gao Xiang
