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

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

[Giuseppe Scrivano]

Add fsparam_fd("source") so that userspace can pass an already-opened
file descriptor instead of a path string.  When the fd is provided via
fsconfig(FSCONFIG_SET_FD, "source", NULL, fd), it is stored directly
in sbi->dif0.file and erofs_fc_get_tree() skips the filp_open() call.

This is useful for mount namespaces where the backing file may not be
reachable by path, and for tools that already hold an fd to the image
(e.g. composefs reusing an erofs mount's backing file).

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
---
I've moved this out of the previous series, as the second patch is on
hold for a design discussion.  Since the two patches were independent,
they do not need to be held together.

v1: https://lore.kernel.org/linux-fsdevel/ak5GfvVfWLJU1EwK@debian/#r

 fs/erofs/super.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

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;
 	}
 	return 0;
 }
@@ -752,14 +759,18 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
 
 static int erofs_fc_get_tree(struct fs_context *fc)
 {
-	int ret;
+	struct erofs_sb_info *sbi = fc->s_fs_info;
+	struct file *file = sbi->dif0.file;
 
-	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 (!IS_ENABLED(CONFIG_EROFS_FS_BACKED_BY_FILE) || !file) {
+		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)
+			return ret;
 
 		if (!fc->source)
 			return invalf(fc, "No source specified");
@@ -767,12 +778,13 @@ static int erofs_fc_get_tree(struct fs_context *fc)
 		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;
+	if (!S_ISREG(file_inode(file)->i_mode) ||
+	    !file->f_mapping->a_ops->read_folio) {
+		errorfc(fc, "source is unsupported");
+		return -EINVAL;
+	}
+	return get_tree_nodev(fc, erofs_fc_fill_super);
 }
 
 static int erofs_fc_reconfigure(struct fs_context *fc)
-- 
2.55.0

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

[Gao Xiang]

On 2026/7/11 15:10, Giuseppe Scrivano wrote:
> Add fsparam_fd("source") so that userspace can pass an already-opened
> file descriptor instead of a path string.  When the fd is provided via
> fsconfig(FSCONFIG_SET_FD, "source", NULL, fd), it is stored directly
> in sbi->dif0.file and erofs_fc_get_tree() skips the filp_open() call.
> 
> This is useful for mount namespaces where the backing file may not be
> reachable by path, and for tools that already hold an fd to the image
> (e.g. composefs reusing an erofs mount's backing file).
> 
> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang

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

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 2994 bytes --]

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

-- 
Aleksa Sarai
Founding Engineer at Amutable
https://www.cyphar.com/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

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

[Gao Xiang]

Hi Aleksa,

On 2026/7/13 12:52, Aleksa Sarai wrote:
> 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.

Yes, I tried this patch just now, it seems it regresses the default
"source" as you said.  I have to withdraw my rvb.

Just check the codebase, it seems at least the minimal change is that
it needs a way for the default "source" to fall back to
vfs_parse_fs_param_source().

My initial rough thought for source_fd support for a filesystem was
not simple as this too, but I never tried to seek time to implement
myself according to my priority list.  As you said, I think in order
to better parse both source or source_fd cases, it should be better
to have better vfs helpers to parse both cases in a clean way;
I think then that should also enable bdev-backed source fds [1].

> 
>   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.)
  
Yes, "none" is useless for source fd cases, it'd be better to have
a path instead.

[1] https://github.com/composefs/composefs-rs/issues/346#issuecomment-4903974902

Thanks,
Gao Xiang

> 
> [1]: https://youtu.be/NX5IzF6JXp0?t=72
> 

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

[Giuseppe Scrivano]

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

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

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 2853 bytes --]

On 2026-07-13, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> 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;

Shortcutting parsing this way is not really idiomatic, the better way is
to create a helper -- in this case you can almost certainly just use
very similar logic to proc_parse_pidns_param() to get something minimal
working.

Defining your own version of "source" in fs_parameter_spec is fine, you
just need to make sure you handle FSCONFIG_SET_STRING properly -- there
are some other examples in the tree you can look at for inspiration
(mostly remote filesystems AFAICS). You could even return -ENOPARAM to
fallback to the basic implementation if that makes it easier for you,
but it would probably be better to handle it all in one place.

> @@ -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;
> +	}

And this would also live in the parser helper.

-- 
Aleksa Sarai
Founding Engineer at Amutable
https://www.cyphar.com/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

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

[Giuseppe Scrivano]

Aleksa Sarai <cyphar@cyphar.com> writes:

> On 2026-07-13, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>> 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;
>
> Shortcutting parsing this way is not really idiomatic, the better way is
> to create a helper -- in this case you can almost certainly just use
> very similar logic to proc_parse_pidns_param() to get something minimal
> working.
>
> Defining your own version of "source" in fs_parameter_spec is fine, you
> just need to make sure you handle FSCONFIG_SET_STRING properly -- there
> are some other examples in the tree you can look at for inspiration
> (mostly remote filesystems AFAICS). You could even return -ENOPARAM to
> fallback to the basic implementation if that makes it easier for you,
> but it would probably be better to handle it all in one place.
>
>> @@ -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;
>> +	}
>
> And this would also live in the parser helper.

thanks for the hints.

I'll prepare a v3 if you are fine with the version below:

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 86fa5c6a0c70..72c85cc53085 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,38 @@ 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: {
+		char *buf, *p;
+
+		sbi->dif0.file = get_file(param->file);
+		buf = kmalloc(PATH_MAX, GFP_KERNEL);
+		if (!buf)
+			return -ENOMEM;
+		p = file_path(param->file, buf, PATH_MAX);
+		fc->source = kstrdup(IS_ERR(p) ? "(fd)" : p, GFP_KERNEL);
+		kfree(buf);
+		if (!fc->source)
+			return -ENOMEM;
+		return 0;
+	}
+	default:
+		return invalf(fc, "Invalid source type");
+	}
+}
+
 static int erofs_fc_parse_param(struct fs_context *fc,
 				struct fs_parameter *param)
 {
@@ -524,6 +558,8 @@ static int erofs_fc_parse_param(struct fs_context *fc,
 		else
 			set_opt(&sbi->opt, INODE_SHARE);
 		break;
+	case Opt_source:
+		return erofs_fc_parse_source(fc, param);
 	}
 	return 0;
 }
@@ -752,14 +788,18 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
 
 static int erofs_fc_get_tree(struct fs_context *fc)
 {
-	int ret;
+	struct erofs_sb_info *sbi = fc->s_fs_info;
+	struct file *file = sbi->dif0.file;
 
-	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 (!IS_ENABLED(CONFIG_EROFS_FS_BACKED_BY_FILE) || !file) {
+		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)
+			return ret;
 
 		if (!fc->source)
 			return invalf(fc, "No source specified");
@@ -767,12 +807,13 @@ static int erofs_fc_get_tree(struct fs_context *fc)
 		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;
+	if (!S_ISREG(file_inode(file)->i_mode) ||
+	    !file->f_mapping->a_ops->read_folio) {
+		errorfc(fc, "source is unsupported");
+		return -EINVAL;
+	}
+	return get_tree_nodev(fc, erofs_fc_fill_super);
 }
 
 static int erofs_fc_reconfigure(struct fs_context *fc)

Regards,
Giuseppe

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

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]

On 2026-07-13, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> thanks for the hints.
> 
> I'll prepare a v3 if you are fine with the version below:

No worries, and this seems more reasonable at a first glance.

> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 86fa5c6a0c70..72c85cc53085 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
...
> @@ -437,6 +439,38 @@ 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: {
> +		char *buf, *p;
> +
> +		sbi->dif0.file = get_file(param->file);

A very minor nit, but you can actually steal the file reference here
with

		sbi->dif0.file = no_free_ptr(param->file);

A few other places do this. (You'll also need to change the param->file
reference below.)

> +		buf = kmalloc(PATH_MAX, GFP_KERNEL);
> +		if (!buf)
> +			return -ENOMEM;
> +		p = file_path(param->file, buf, PATH_MAX);
> +		fc->source = kstrdup(IS_ERR(p) ? "(fd)" : p, GFP_KERNEL);

I think that /proc/self/fd/%d would be a more useful name for debugging
if file_path() fails (not that it is really possible here AFAICS). But
I'm not really too fussed.

-- 
Aleksa Sarai
Founding Engineer at Amutable
https://www.cyphar.com/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

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

[Gao Xiang]

On 2026/7/14 08:49, Aleksa Sarai wrote:
> On 2026-07-13, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>> thanks for the hints.
>>
>> I'll prepare a v3 if you are fine with the version below:
> 
> No worries, and this seems more reasonable at a first glance.
> 
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index 86fa5c6a0c70..72c85cc53085 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
> ...
>> @@ -437,6 +439,38 @@ 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: {
>> +		char *buf, *p;
>> +
>> +		sbi->dif0.file = get_file(param->file);
> 
> A very minor nit, but you can actually steal the file reference here
> with
> 
> 		sbi->dif0.file = no_free_ptr(param->file);
> 
> A few other places do this. (You'll also need to change the param->file
> reference below.)
> 
>> +		buf = kmalloc(PATH_MAX, GFP_KERNEL);
>> +		if (!buf)
>> +			return -ENOMEM;
>> +		p = file_path(param->file, buf, PATH_MAX);
>> +		fc->source = kstrdup(IS_ERR(p) ? "(fd)" : p, GFP_KERNEL);
> 
> I think that /proc/self/fd/%d would be a more useful name for debugging
> if file_path() fails (not that it is really possible here AFAICS). But
> I'm not really too fussed.

Not quite sure if we should get in agreement with the format of this
one in advance (IOWs, users use source_fd and how fc->source looks like;
since other fses may follow the same practice if source_fd becomes common
later) since it's a user-visible field and I believe we shouldn't treat
this one as a dontcare field as some pseudo fses (since those fses don't
rely on `fc->source` by design but typically EROFS can rely on.)

I hope Christian and others could share move thought on this part too
before I land this feature for the next cycle.

Thanks,
Gao Xiang


> 

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

[Giuseppe Scrivano]

Gao Xiang <hsiangkao@linux.alibaba.com> writes:

> On 2026/7/14 08:49, Aleksa Sarai wrote:
>> On 2026-07-13, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>>> thanks for the hints.
>>>
>>> I'll prepare a v3 if you are fine with the version below:
>> No worries, and this seems more reasonable at a first glance.
>> 
>>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>>> index 86fa5c6a0c70..72c85cc53085 100644
>>> --- a/fs/erofs/super.c
>>> +++ b/fs/erofs/super.c
>> ...
>>> @@ -437,6 +439,38 @@ 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: {
>>> +		char *buf, *p;
>>> +
>>> +		sbi->dif0.file = get_file(param->file);
>> A very minor nit, but you can actually steal the file reference here
>> with
>> 		sbi->dif0.file = no_free_ptr(param->file);
>> A few other places do this. (You'll also need to change the
>> param->file
>> reference below.)
>> 
>>> +		buf = kmalloc(PATH_MAX, GFP_KERNEL);
>>> +		if (!buf)
>>> +			return -ENOMEM;
>>> +		p = file_path(param->file, buf, PATH_MAX);
>>> +		fc->source = kstrdup(IS_ERR(p) ? "(fd)" : p, GFP_KERNEL);
>> I think that /proc/self/fd/%d would be a more useful name for
>> debugging
>> if file_path() fails (not that it is really possible here AFAICS). But
>> I'm not really too fussed.
>
> Not quite sure if we should get in agreement with the format of this
> one in advance (IOWs, users use source_fd and how fc->source looks like;
> since other fses may follow the same practice if source_fd becomes common
> later) since it's a user-visible field and I believe we shouldn't treat
> this one as a dontcare field as some pseudo fses (since those fses don't
> rely on `fc->source` by design but typically EROFS can rely on.)
>
> I hope Christian and others could share move thought on this part too
> before I land this feature for the next cycle.

would it be better to just return the error from file_path without any
fallback?

Thanks,
Giuseppe

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

[Gao Xiang]

On 2026/7/14 14:36, Giuseppe Scrivano wrote:
> Gao Xiang <hsiangkao@linux.alibaba.com> writes:
> 
>> On 2026/7/14 08:49, Aleksa Sarai wrote:
>>> On 2026-07-13, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>>>> thanks for the hints.
>>>>
>>>> I'll prepare a v3 if you are fine with the version below:
>>> No worries, and this seems more reasonable at a first glance.
>>>
>>>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>>>> index 86fa5c6a0c70..72c85cc53085 100644
>>>> --- a/fs/erofs/super.c
>>>> +++ b/fs/erofs/super.c
>>> ...
>>>> @@ -437,6 +439,38 @@ 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: {
>>>> +		char *buf, *p;
>>>> +
>>>> +		sbi->dif0.file = get_file(param->file);
>>> A very minor nit, but you can actually steal the file reference here
>>> with
>>> 		sbi->dif0.file = no_free_ptr(param->file);
>>> A few other places do this. (You'll also need to change the
>>> param->file
>>> reference below.)
>>>
>>>> +		buf = kmalloc(PATH_MAX, GFP_KERNEL);
>>>> +		if (!buf)
>>>> +			return -ENOMEM;
>>>> +		p = file_path(param->file, buf, PATH_MAX);
>>>> +		fc->source = kstrdup(IS_ERR(p) ? "(fd)" : p, GFP_KERNEL);
>>> I think that /proc/self/fd/%d would be a more useful name for
>>> debugging
>>> if file_path() fails (not that it is really possible here AFAICS). But
>>> I'm not really too fussed.
>>
>> Not quite sure if we should get in agreement with the format of this
>> one in advance (IOWs, users use source_fd and how fc->source looks like;
>> since other fses may follow the same practice if source_fd becomes common
>> later) since it's a user-visible field and I believe we shouldn't treat
>> this one as a dontcare field as some pseudo fses (since those fses don't
>> rely on `fc->source` by design but typically EROFS can rely on.)
>>
>> I hope Christian and others could share move thought on this part too
>> before I land this feature for the next cycle.
> 
> would it be better to just return the error from file_path without any
> fallback?

I hope Christian or other vfs folks can decide how to handle fc->source
string here, since in the long term, how to deal with source_fd should
be unique among different fses: just our current short-term
implementation lands into erofs directly for file-backed mounts to
fulfill composefs needs.

Thanks,
Gao Xiang

> 
> Thanks,
> Giuseppe

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

[Giuseppe Scrivano]

Gao Xiang <hsiangkao@linux.alibaba.com> writes:

> On 2026/7/14 14:36, Giuseppe Scrivano wrote:
>> Gao Xiang <hsiangkao@linux.alibaba.com> writes:
>> 
>>> On 2026/7/14 08:49, Aleksa Sarai wrote:
>>>> On 2026-07-13, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>>>>> thanks for the hints.
>>>>>
>>>>> I'll prepare a v3 if you are fine with the version below:
>>>> No worries, and this seems more reasonable at a first glance.
>>>>
>>>>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>>>>> index 86fa5c6a0c70..72c85cc53085 100644
>>>>> --- a/fs/erofs/super.c
>>>>> +++ b/fs/erofs/super.c
>>>> ...
>>>>> @@ -437,6 +439,38 @@ 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: {
>>>>> +		char *buf, *p;
>>>>> +
>>>>> +		sbi->dif0.file = get_file(param->file);
>>>> A very minor nit, but you can actually steal the file reference here
>>>> with
>>>> 		sbi->dif0.file = no_free_ptr(param->file);
>>>> A few other places do this. (You'll also need to change the
>>>> param->file
>>>> reference below.)
>>>>
>>>>> +		buf = kmalloc(PATH_MAX, GFP_KERNEL);
>>>>> +		if (!buf)
>>>>> +			return -ENOMEM;
>>>>> +		p = file_path(param->file, buf, PATH_MAX);
>>>>> +		fc->source = kstrdup(IS_ERR(p) ? "(fd)" : p, GFP_KERNEL);
>>>> I think that /proc/self/fd/%d would be a more useful name for
>>>> debugging
>>>> if file_path() fails (not that it is really possible here AFAICS). But
>>>> I'm not really too fussed.
>>>
>>> Not quite sure if we should get in agreement with the format of this
>>> one in advance (IOWs, users use source_fd and how fc->source looks like;
>>> since other fses may follow the same practice if source_fd becomes common
>>> later) since it's a user-visible field and I believe we shouldn't treat
>>> this one as a dontcare field as some pseudo fses (since those fses don't
>>> rely on `fc->source` by design but typically EROFS can rely on.)
>>>
>>> I hope Christian and others could share move thought on this part too
>>> before I land this feature for the next cycle.
>> would it be better to just return the error from file_path without
>> any
>> fallback?
>
> I hope Christian or other vfs folks can decide how to handle fc->source
> string here, since in the long term, how to deal with source_fd should
> be unique among different fses: just our current short-term
> implementation lands into erofs directly for file-backed mounts to
> fulfill composefs needs.
>

overlayfs is already handling such a case, and it resolves the path using d_path:

static int ovl_parse_layer(struct fs_context *fc, struct fs_parameter *param,
			   enum ovl_opt layer)
{
	struct path layer_path __free(path_put) = {};
	int err = 0;

	switch (param->type) {
	case fs_value_is_string:
		err = ovl_kern_path(param->string, &layer_path, layer);
		if (err)
			return err;
		err = ovl_do_parse_layer(fc, param->string, &layer_path, layer);
		break;
	case fs_value_is_file: {
		char *buf __free(kfree);
		char *layer_name;

		buf = kmalloc(PATH_MAX, GFP_KERNEL_ACCOUNT);
		if (!buf)
			return -ENOMEM;

		layer_path = param->file->f_path;
		path_get(&layer_path);

		layer_name = d_path(&layer_path, buf, PATH_MAX);
		if (IS_ERR(layer_name))
			return PTR_ERR(layer_name);

		err = ovl_do_parse_layer(fc, layer_name, &layer_path, layer);
		break;
	}
	default:
		WARN_ON_ONCE(true);
		err = -EINVAL;
	}

	return err;
}

If there are no objections, I'll change the code to match what overlay
does and return the error instead of using a fallback.

Regards,
Giuseppe