Message-ID: <CAOQ4uxgaxkJexvUFOFDEAbm+vW4A1qkmvqZJEYkZGR5Mp=gtrg@mail.gmail.com>
Date: Wed, 18 Jun 2025 13:01:44 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Paul Lawrence <paullawrence@...gle.com>
Cc: Miklos Szeredi <miklos@...redi.hu>, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, Bernd Schubert <bernd.schubert@...tmail.fm>
Subject: Re: [PATCH v1 0/2] RFC: Extend fuse-passthrough to directories

Hi Paul,

I am very happy that you are getting back to this task
as I find myself never getting to it.

I would like to ask you to try to build on the code that was already
merged to the upstream kernel rather than trying to go back to
the out of tree version concepts.

I've added libfuse maintainer in CC, please remember to CC him
in future emails about proposed FUSE UAPI changes.

On Wed, Jun 18, 2025 at 12:15 AM Paul Lawrence <paullawrence@...gle.com> wrote:
>
> This is the first part of a much larger patch set that would allow a directory
> to be marked ‘passthrough’. At a high level, the fuse daemon can return an
> optional extra argument to FUSE_LOOKUP that contains an fd.

What's wrong with returning a backing_id?
Why "reinvent" the pre-release wheel?

> Extra fields are
> added to the fuse_dentry, fuse_inode and fuse_file structs to have a backing
> path, inode and file respectively. When fuse is performing an operation, it will
> check for the existence of a backing object and if it exists forward the
> operation to the backing object.

I read your patches and I have no idea why you needed to add fields to
fuse_dentry and why you needed to duplicate the backing file fields in
fuse_inode and fuse_file. Please explain yourself.

>
> These two patches add the core infrastructure, handling of the extra argument
> response to lookup, and forwarding open, flush and close to the backing file.
> This is sufficient to validate the concept.

What I am reading between the lines is that open/close are examples
of passthrough ops that are expected to be extended to more ops later.
Please see my WIP branch [1] for a suggestion to use an ops_mask API
for declaring which inode operations are passthrough.

[1] https://github.com/amir73il/linux/commits/fuse-backing-inode-wip/

>
> The questions I have:
>
> * Is this something that interests the fuse maintainers and community?

Definitely interested in inode ops passthrough.
I am willing to help with review and implementation if needed.

> * Is this approach the correct one?

Which approach?

Miklos and I have been discussing setting up a backing file on
FUSE_LOOKUP for a while.
The same concept was discussed also for returning an iomap
description on FUSE_LOOKUP request [2]

[2] https://lore.kernel.org/linux-fsdevel/20250529164503.GB8282@frogsfrogsfrogs/

So yes, the concept of setting up passthough on lookup is the correct one.

One thing that you need to be aware of is that FUSE_LOOKUP is only one
of several ways to instantiate a fuse dentry/inode.

The commands FUSE_CREATE, FUSE_TMPFILE and FUSE_READDIRPLUS
also instantiate dentries and need to be dealt with as well.

Therefore, I was looking at the direction of extending struct fuse_entry_out
rather than adding a new argument to FUSE_LOOKUP.

> * (if we agree yes to 1 & 2) Detailed analysis of the patches for errors.
>
> A few observations:
>
> I have matched backing objects to their fuse objects. Currently fuse passthrough
> puts a backing file into the fuse inode. I’m not quite sure why this was done -
> it seems to have been a very late change in the passthrough patch sets which
> happened without comment.

It was done to be able to have sane support for mmap passthrough among
other things and be able to have a sane story about inode attributes.

> It does not really make sense for full directory
> passthrough since unopened inodes still need to have backing inodes.

I do not understand this statement.
Why doesn't it make sense?
Why does it make sense to have a backing path per dentry?
Are you expecting to map different hardlink aliases to different backing inodes?
It might have helped if you had a design document explaining the reasoning
behind the implementation choices.

> The one
> advantage I can see is that it reduces the number of opens/closes of the backing
> file. However, this may also be a disadvantage - it moves closes, in particular,
> to an arbitrary point when the inode is flushed from cache.
>

Rather than when? when dentry is flushed from cache?
I do not follow.

> Backing operations need to happen in the context of the daemon, not the caller.

Correct.

> (I am a firm believer of this principle.) This is not yet implemented, and is
> not (currently, and unfortunately) the way Android uses passthough. It is not
> hard to do, and if these patches are otherwise acceptable, will be added.
>

Note that many of the passthrough method implementations were moved
to common "library" code for handling backing_file, which may or may not
be sharable with overlayfs.

I don't think that we need to have all passthough methods implemented in the
generic backing_file "library" and share code with overlayfs, but we
should consider
it for new methods to see if it makes sense.

> There was a long discussion about the security issues of using an fd returned
> from the fuse daemon in the context of fuse passthrough, and the end solution
> was to use an ioctl to set the backing file. I have used the previously-rejected
> approach of passing the fd in a struct in the fuse_daemon response. My defense
> of this approach is
>
> * The fd is simply used to pull out the path and inode
> * All operations are revalidated
> * Thus there is no risk even if a privileged process with a protected fd is
> tricked into passing that fd back in this structure.

Not convinced.
Are you saying that passing an O_PATH fd of /etc/passwd is ok?
when that O_PATH is used to do passthrough open?
I do not follow and I also do not see the problem with sticking with
the already established backing_id and ioctl solution.

I would add that if you make your solution dependent on io_uring
then the security concern goes away, but you did not give a strong
enough reason to have this limitation IMO.

>
> I’m sure we will discuss this at length if this patch set is otherwise deemed
> valuable, and I am certainly not wedded to this approach.
>
> I have written tests to validate this approach using tools/testing/selftests. I
> don’t want this patch set to get derailed by a discussion of the way I wrote the
> tests, so I have not included them. I am very open to any and every suggestion
> as to how (and where) tests should be written for these patches.
>

Apart from specific function tests for passthrough, FUSE_PASSTHROUGH
was tested with fstests, using the scripts included in libfuse to run the
passthrough_hp examples in fstests.

So a basic sanity test for your code is that it does not regress fstests that
are currently passing with passthrough_hp.

Please let me know if anything I wrote is not clear and if there is anything
else that I can help with.

Thanks,
Amir.

Powered by blists - more mailing lists