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

[Giuseppe Scrivano <gscrivan@redhat.com>]

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