Re: [PATCH v4] erofs: accept source file descriptor via fsconfig

> 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.

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()

--