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

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

>