Linux filesystem development
 help / color / mirror / Atom feed
* [PATCH v4] erofs: accept source file descriptor via fsconfig
@ 2026-07-17 13:41 Giuseppe Scrivano
  2026-07-20  9:45 ` Gao Xiang
  2026-07-22 16:11 ` Christian Brauner
  0 siblings, 2 replies; 7+ messages in thread
From: Giuseppe Scrivano @ 2026-07-17 13:41 UTC (permalink / raw)
  To: linux-erofs; +Cc: cyphar, hsiangkao, linux-fsdevel, gscrivan

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 <gscrivan@redhat.com>
---
v3: https://lore.kernel.org/linux-fsdevel/20260714154917.489993-1-gscrivan@redhat.com/
v2: https://lore.kernel.org/linux-fsdevel/20260711071137.4130824-1-gscrivan@redhat.com/
v1: https://lore.kernel.org/linux-fsdevel/ak5GfvVfWLJU1EwK@debian/

 fs/erofs/super.c | 73 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 62 insertions(+), 11 deletions(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 86fa5c6a0c70..558041011398 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: {
+		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;
+		sbi->dif0.file = no_free_ptr(param->file);
+		return 0;
+	}
+	default:
+		WARN_ON_ONCE(true);
+		return -EINVAL;
+	}
+}
+
 static int erofs_fc_parse_param(struct fs_context *fc,
 				struct fs_parameter *param)
 {
@@ -524,6 +560,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,13 +790,26 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
 
 static int erofs_fc_get_tree(struct fs_context *fc)
 {
+	struct erofs_sb_info *sbi = fc->s_fs_info;
 	int ret;
 
+	if (sbi->dif0.file) {
+		if (!IS_ENABLED(CONFIG_EROFS_FS_BACKED_BY_FILE)) {
+			errorfc(fc, "source fd option not supported");
+			return -EINVAL;
+		}
+		if (!S_ISREG(file_inode(sbi->dif0.file)->i_mode) ||
+		    !sbi->dif0.file->f_mapping->a_ops->read_folio) {
+			errorfc(fc, "source is unsupported");
+			return -EINVAL;
+		}
+		return get_tree_nodev(fc, erofs_fc_fill_super);
+	}
+
 	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)
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v4] erofs: accept source file descriptor via fsconfig
  2026-07-17 13:41 [PATCH v4] erofs: accept source file descriptor via fsconfig Giuseppe Scrivano
@ 2026-07-20  9:45 ` Gao Xiang
  2026-07-22 16:11 ` Christian Brauner
  1 sibling, 0 replies; 7+ messages in thread
From: Gao Xiang @ 2026-07-20  9:45 UTC (permalink / raw)
  To: Giuseppe Scrivano, linux-erofs, Christian Brauner, Aleksa Sarai
  Cc: linux-fsdevel



On 2026/7/17 21:41, Giuseppe Scrivano 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 <gscrivan@redhat.com>
> ---

Looks good and tested:

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

I wonder if Aleksa and Christian have more comments since
anyway it touches fc->source, but I will queue for the next
cycle if there is no other opinion.

Thanks,
Gao Xiang

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4] erofs: accept source file descriptor via fsconfig
  2026-07-17 13:41 [PATCH v4] erofs: accept source file descriptor via fsconfig Giuseppe Scrivano
  2026-07-20  9:45 ` Gao Xiang
@ 2026-07-22 16:11 ` Christian Brauner
  2026-07-22 16:25   ` Gao Xiang
  1 sibling, 1 reply; 7+ messages in thread
From: Christian Brauner @ 2026-07-22 16:11 UTC (permalink / raw)
  To: Giuseppe Scrivano; +Cc: linux-erofs, cyphar, hsiangkao, linux-fsdevel

> 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 <gscrivan@redhat.com>
>
> 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()

-- 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4] erofs: accept source file descriptor via fsconfig
  2026-07-22 16:11 ` Christian Brauner
@ 2026-07-22 16:25   ` Gao Xiang
  2026-07-23 14:46     ` Christian Brauner
  0 siblings, 1 reply; 7+ messages in thread
From: Gao Xiang @ 2026-07-22 16:25 UTC (permalink / raw)
  To: Christian Brauner, Giuseppe Scrivano; +Cc: linux-erofs, cyphar, linux-fsdevel

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 <gscrivan@redhat.com>
>>
>> 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

> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4] erofs: accept source file descriptor via fsconfig
  2026-07-22 16:25   ` Gao Xiang
@ 2026-07-23 14:46     ` Christian Brauner
  2026-07-23 22:38       ` Gao Xiang
  0 siblings, 1 reply; 7+ messages in thread
From: Christian Brauner @ 2026-07-23 14:46 UTC (permalink / raw)
  To: Gao Xiang
  Cc: Christian Brauner, Giuseppe Scrivano, linux-erofs, cyphar,
	linux-fsdevel

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

Oh, so you only do it for file-backed mounts. Do you only allow regular
files or do you also support block devices with
CONFIG_EROFS_FS_BACKED_BY_FILE?

Do you document the expected behavior for the file you're consuming?
Meaning, are concurrent modifications supported and what type of
behavior does this exhibit?

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

No, I misunderstood this.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4] erofs: accept source file descriptor via fsconfig
  2026-07-23 14:46     ` Christian Brauner
@ 2026-07-23 22:38       ` Gao Xiang
  2026-07-27  8:01         ` Giuseppe Scrivano
  0 siblings, 1 reply; 7+ messages in thread
From: Gao Xiang @ 2026-07-23 22:38 UTC (permalink / raw)
  To: Christian Brauner; +Cc: Giuseppe Scrivano, linux-erofs, cyphar, linux-fsdevel

Hi Christian,

On 2026/7/23 22:46, Christian Brauner wrote:
>> 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.
> 
> Oh, so you only do it for file-backed mounts. Do you only allow regular
> files or do you also support block devices with
> CONFIG_EROFS_FS_BACKED_BY_FILE?

Block devices with CONFIG_EROFS_FS_BACKED_BY_FILE are supported,
but with only `fc->source` (not this way.)

That is the limitation I see in Giuseppe's patch. I'd hoped
bdev-backed mounts could work the same way, but that would require
changes to the VFS flow.

Since this is a side improvement, I think it's fine as long as
it's documented somewhere, and I do hope Giuseppe can at least
address the documentation.

I'd also like to make sure the way fc->source is filled
out of fd passing follows common practice, so that if fd-based
bdev-backed mounts land in the VFS later, they can keep
the same fc->source convention, otherwise it will cause
a userspace behavior change.

> 
> Do you document the expected behavior for the file you're consuming?
> Meaning, are concurrent modifications supported and what type of
> behavior does this exhibit?


As I perhaps mentioned, EROFS itself (or many EROFS) won't do any
modification to the underlayfs bdev or files by design so the
standard behavior is the blob devices / files won't get any change.
Beyond that, both the on-disk format and the implementation are
designed to tolerate unexpected external modifications (or storage
media damage). Even in the worst case, where the underlying storage
(block device or backing filesystem) is malicious, corrupted on-disk
(meta)data will not lead to the kind of complex, hard-to-resolve
inconsistencies you see in general-purpose writable filesystems,
whose ondisk/in-memory cached metadata is much harder to reconcile.

I'm not sure whether you'll agree, but I want to emphasize that
again this is one of EROFS core design goals: the on-disk and
implementation design ensure that. If there is any human bug, it
will be addressed and fixed as long as it discloses: it won't be
hard to fixed.

But if you really want to avoid concurrent modifications or keep
the image golden, I think dmverity or fsverify should be enforced
to ensure the filesystem won't be modified unexpectedly or expectedly.

> 
>> 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.
> 
> No, I misunderstood this.

Thanks,
Gao Xiang


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4] erofs: accept source file descriptor via fsconfig
  2026-07-23 22:38       ` Gao Xiang
@ 2026-07-27  8:01         ` Giuseppe Scrivano
  0 siblings, 0 replies; 7+ messages in thread
From: Giuseppe Scrivano @ 2026-07-27  8:01 UTC (permalink / raw)
  To: Gao Xiang; +Cc: Christian Brauner, linux-erofs, cyphar, linux-fsdevel

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

> Hi Christian,
>
> On 2026/7/23 22:46, Christian Brauner wrote:
>>> 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.
>> Oh, so you only do it for file-backed mounts. Do you only allow
>> regular
>> files or do you also support block devices with
>> CONFIG_EROFS_FS_BACKED_BY_FILE?
>
> Block devices with CONFIG_EROFS_FS_BACKED_BY_FILE are supported,
> but with only `fc->source` (not this way.)
>
> That is the limitation I see in Giuseppe's patch. I'd hoped
> bdev-backed mounts could work the same way, but that would require
> changes to the VFS flow.
>
> Since this is a side improvement, I think it's fine as long as
> it's documented somewhere, and I do hope Giuseppe can at least
> address the documentation.

would something like the following be enough?

diff --git a/Documentation/filesystems/erofs.rst b/Documentation/filesystems/erofs.rst
index 4230884fb359..768e1d43dfcc 100644
--- a/Documentation/filesystems/erofs.rst
+++ b/Documentation/filesystems/erofs.rst
@@ -139,6 +139,29 @@ inode_share            Enable inode page sharing for this filesystem.  Inodes wi
                        page cache.
 ===================    =========================================================
 
+File-backed mounts
+==================
+
+When ``CONFIG_EROFS_FS_BACKED_BY_FILE`` is enabled, EROFS can mount filesystem
+images stored as regular files directly, without requiring a loopback block
+device.  The source can be specified either by path or by passing an
+already-opened file descriptor via ``fsconfig(fd, FSCONFIG_SET_FD, "source",
+NULL, source_fd)``.  Only regular files are accepted; block devices must use
+the standard block device mount path.
+
+The backing file content must remain stable for the lifetime of the mount.
+EROFS never writes to it, but concurrent modifications by other processes lead
+to undefined behavior.
+
+Ioctls
+======
+
+``EROFS_IOC_GET_SOURCE_FD``
+  Return a read-only file descriptor (``O_CLOEXEC``) for the backing file of a
+  file-backed mount.  Returns ``-ENOENT`` on block-device-backed mounts.
+  Requires ``CAP_SYS_ADMIN`` in the initial user namespace (returns ``-EPERM``
+  otherwise).
+
 Sysfs Entries
 =============
 
Regards,
Giuseppe


> I'd also like to make sure the way fc->source is filled
> out of fd passing follows common practice, so that if fd-based
> bdev-backed mounts land in the VFS later, they can keep
> the same fc->source convention, otherwise it will cause
> a userspace behavior change.
>
>> Do you document the expected behavior for the file you're consuming?
>> Meaning, are concurrent modifications supported and what type of
>> behavior does this exhibit?
>
>
> As I perhaps mentioned, EROFS itself (or many EROFS) won't do any
> modification to the underlayfs bdev or files by design so the
> standard behavior is the blob devices / files won't get any change.
> Beyond that, both the on-disk format and the implementation are
> designed to tolerate unexpected external modifications (or storage
> media damage). Even in the worst case, where the underlying storage
> (block device or backing filesystem) is malicious, corrupted on-disk
> (meta)data will not lead to the kind of complex, hard-to-resolve
> inconsistencies you see in general-purpose writable filesystems,
> whose ondisk/in-memory cached metadata is much harder to reconcile.
>
> I'm not sure whether you'll agree, but I want to emphasize that
> again this is one of EROFS core design goals: the on-disk and
> implementation design ensure that. If there is any human bug, it
> will be addressed and fixed as long as it discloses: it won't be
> hard to fixed.
>
> But if you really want to avoid concurrent modifications or keep
> the image golden, I think dmverity or fsverify should be enforced
> to ensure the filesystem won't be modified unexpectedly or expectedly.
>
>> 
>>> 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.
>> No, I misunderstood this.
>
> Thanks,
> Gao Xiang


^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-07-27  8:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-17 13:41 [PATCH v4] erofs: accept source file descriptor via fsconfig Giuseppe Scrivano
2026-07-20  9:45 ` Gao Xiang
2026-07-22 16:11 ` Christian Brauner
2026-07-22 16:25   ` Gao Xiang
2026-07-23 14:46     ` Christian Brauner
2026-07-23 22:38       ` Gao Xiang
2026-07-27  8:01         ` Giuseppe Scrivano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox