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

[Giuseppe Scrivano <gscrivan@redhat.com>]

Aleksa Sarai <cyphar@cyphar.com> writes:

> On 2026-07-11, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index 86fa5c6a0c70..3040d4cf9b85 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_fd,
>>  };
>>  
>>  static const struct constant_table erofs_param_cache_strategy[] = {
>> @@ -413,6 +414,7 @@ static const struct fs_parameter_spec erofs_fs_parameters[] = {
>>  	fsparam_flag_no("directio",	Opt_directio),
>>  	fsparam_u64("fsoffset",		Opt_fsoffset),
>>  	fsparam_flag("inode_share",	Opt_inode_share),
>> +	fsparam_fd("source",		Opt_source_fd),
>>  	{}
>>  };
>>  
>> @@ -524,6 +526,11 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>>  		else
>>  			set_opt(&sbi->opt, INODE_SHARE);
>>  		break;
>> +	case Opt_source_fd:
>> +		if (sbi->dif0.file)
>> +			return -EINVAL;
>> +		sbi->dif0.file = get_file(param->file);
>> +		break;
>
> I don't think this handling is right for a few reasons:
>
>  1. AFAICS this shadows the default "source" handling logic (because
>     -ENOPARAM is not returned for the non-fd case), which means that
>     this regresses existing erofs users -- everyone already uses
>     "source" today. I must really be missing something if this worked
>     when you tested it.
>
>     Additionally, fsparam_fd unfortunately permits strings (where the
>     string is the numerical value of the fd number), meaning that this
>     will call get_file(<garbage>) if someone uses FSCONFIG_SET_STRING.
>     You will need to check param->type at least to avoid that.
>
>     I meant to send a patch for this earlier this year, but a nicer
>     solution would be to have a custom helper similar to fs_lookup_param
>     except that it permits FSCONFIG_SET_FD, FSCONFIG_SET_PATH,
>     FSCONFIG_SET_PATH_EMPTY, and FSCONFIG_SET_STRING. This is sorely
>     missing and people keep accidentally creating unusable interfaces as
>     a result. I mentioned this in an LPC talk last year[1].
>
>     proc_parse_pidns_param was my minimal version that only accepts
>     FSCONFIG_SET_FD and FSCONFIG_SET_STRING, and if you don't want to
>     add dirfd support yet then you should use something more like that.
>
>  2. On a slightly less critical note, fc->source has special handling in
>     the VFS in a few places and AFAICS this is the first example of
>     someone adding an implementation of "source" that does not set
>     fc->source to a proper value, which deserves some additional review.
>
>     (At at quick glance it seems this just means that some stuff in
>     procfs will show as "none" rather than fc->source debugging, but
>     again it probably needs a closer look.)
>
> [1]: https://youtu.be/NX5IzF6JXp0?t=72

sorry, I underestimated the blast of the patch.  I didn't realize it
changes the default handling and that is why I've limited my testing to
the new case only.

Would you be fine if I amend the following fixup?

It addresses both the issues you've reported.  An EROFS image file
mounted via its path still works and shows correctly in mountinfo and
"source" is also populated when mounting from a file descriptor.

# /root/erofs-test/erofs-mount-via-fd /root/erofs.blob /mnt
# grep /mnt /proc/self/mountinfo
671 42 0:83 / /mnt ro,relatime shared:666 - erofs /root/erofs.blob ro,seclabel,user_xattr,acl,cache_strategy=readaround

# mount /root/erofs.blob /mnt/
# grep /mnt /proc/self/mountinfo
671 42 0:83 / /mnt rw,relatime shared:666 - erofs /root/erofs.blob ro,seclabel,user_xattr,acl,cache_strategy=readaround

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 3040d4cf9b85..7818872ab1e5 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -386,7 +386,6 @@ 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_fd,
 };
 
 static const struct constant_table erofs_param_cache_strategy[] = {
@@ -414,7 +413,6 @@ static const struct fs_parameter_spec erofs_fs_parameters[] = {
 	fsparam_flag_no("directio",	Opt_directio),
 	fsparam_u64("fsoffset",		Opt_fsoffset),
 	fsparam_flag("inode_share",	Opt_inode_share),
-	fsparam_fd("source",		Opt_source_fd),
 	{}
 };
 
@@ -447,6 +445,14 @@ static int erofs_fc_parse_param(struct fs_context *fc,
 	struct erofs_device_info *dif;
 	int opt, ret;
 
+	if (strcmp(param->key, "source") == 0 &&
+	    param->type == fs_value_is_file) {
+		if (sbi->dif0.file || fc->source)
+			return -EINVAL;
+		sbi->dif0.file = get_file(param->file);
+		return 0;
+	}
+
 	opt = fs_parse(fc, erofs_fs_parameters, param, &result);
 	if (opt < 0)
 		return opt;
@@ -526,11 +532,6 @@ static int erofs_fc_parse_param(struct fs_context *fc,
 		else
 			set_opt(&sbi->opt, INODE_SHARE);
 		break;
-	case Opt_source_fd:
-		if (sbi->dif0.file)
-			return -EINVAL;
-		sbi->dif0.file = get_file(param->file);
-		break;
 	}
 	return 0;
 }
@@ -779,6 +780,18 @@ static int erofs_fc_get_tree(struct fs_context *fc)
 			return PTR_ERR(file);
 		sbi->dif0.file = file;
 	}
+	if (!fc->source) {
+		char *buf, *p;
+
+		buf = kmalloc(PATH_MAX, GFP_KERNEL);
+		if (!buf)
+			return -ENOMEM;
+		p = file_path(file, buf, PATH_MAX);
+		fc->source = kstrdup(IS_ERR(p) ? "(fd)" : p, GFP_KERNEL);
+		kfree(buf);
+		if (!fc->source)
+			return -ENOMEM;
+	}
 	if (!S_ISREG(file_inode(file)->i_mode) ||
 	    !file->f_mapping->a_ops->read_folio) {
 		errorfc(fc, "source is unsupported");

Regards,
Giuseppe