| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOQ4uxgNjDuitnAn1np29nB7WfDbEkN3K8oOPUc7wH7A+UfuuA@mail.gmail.com>
Date: Sun, 10 Aug 2025 16:21:47 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Paul Lawrence <paullawrence@...gle.com>
Cc: bernd.schubert@...tmail.fm, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, miklos@...redi.hu
Subject: Re: [PATCH 1/2] fuse: Allow backing file to be set at lookup (WIP)
On Mon, Aug 4, 2025 at 7:32 PM Paul Lawrence <paullawrence@...gle.com> wrote:
>
> Add optional extra outarg to FUSE_LOOKUP which holds a backing id to set
> a backing file at lookup.
>
> Signed-off-by: Paul Lawrence <paullawrence@...gle.com>
> ---
> fs/fuse/dir.c | 23 ++++++++++++++++++----
> fs/fuse/fuse_i.h | 3 +++
> fs/fuse/iomode.c | 41 +++++++++++++++++++++++++++++++++++----
> fs/fuse/passthrough.c | 40 +++++++++++++++++++++++++++++---------
> include/uapi/linux/fuse.h | 4 ++++
> 5 files changed, 94 insertions(+), 17 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 62508d212826..c0bef93dd078 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -170,7 +170,8 @@ static void fuse_invalidate_entry(struct dentry *entry)
>
> static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args,
> u64 nodeid, const struct qstr *name,
> - struct fuse_entry_out *outarg)
> + struct fuse_entry_out *outarg,
> + struct fuse_entry_passthrough_out *backing)
> {
> memset(outarg, 0, sizeof(struct fuse_entry_out));
> args->opcode = FUSE_LOOKUP;
> @@ -184,6 +185,12 @@ static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args,
> args->out_numargs = 1;
> args->out_args[0].size = sizeof(struct fuse_entry_out);
> args->out_args[0].value = outarg;
> + if (backing) {
> + args->out_numargs = 2;
> + args->out_args[1].size = sizeof(struct fuse_entry_passthrough_out);
> + args->out_args[1].value = backing;
> + args->out_argvar = true;
> + }
> }
>
> /*
> @@ -236,7 +243,7 @@ static int fuse_dentry_revalidate(struct inode *dir, const struct qstr *name,
> attr_version = fuse_get_attr_version(fm->fc);
>
> fuse_lookup_init(fm->fc, &args, get_node_id(dir),
> - name, &outarg);
> + name, &outarg, NULL);
> ret = fuse_simple_request(fm, &args);
> /* Zero nodeid is same as -ENOENT */
> if (!ret && !outarg.nodeid)
> @@ -369,6 +376,7 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name
> struct fuse_forget_link *forget;
> u64 attr_version, evict_ctr;
> int err;
> + struct fuse_entry_passthrough_out passthrough;
>
> *inode = NULL;
> err = -ENAMETOOLONG;
> @@ -384,10 +392,10 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name
> attr_version = fuse_get_attr_version(fm->fc);
> evict_ctr = fuse_get_evict_ctr(fm->fc);
>
> - fuse_lookup_init(fm->fc, &args, nodeid, name, outarg);
> + fuse_lookup_init(fm->fc, &args, nodeid, name, outarg, &passthrough);
> err = fuse_simple_request(fm, &args);
> /* Zero nodeid is same as -ENOENT, but with valid timeout */
> - if (err || !outarg->nodeid)
> + if (err < 0 || !outarg->nodeid)
Why this change?
> goto out_put_forget;
>
> err = -EIO;
> @@ -406,6 +414,13 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name
> fuse_queue_forget(fm->fc, forget, outarg->nodeid, 1);
> goto out;
> }
> +
> + // TODO check that if fuse_backing is already set they are consistent
> + if (args.out_args[1].size && !get_fuse_inode(*inode)->fb) {
This check is not atomic.
fuse_inode_set_passthrough() already does nothing if
fuse_inode_passthrough() is already set.
> + err = fuse_inode_set_passthrough(*inode, passthrough.backing_id);
> + if (err)
> + goto out;
This needs to queue forget + iput.
> + }
> err = 0;
>
> out_put_forget:
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 1e8e732a2f09..aebd338751f1 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -1595,9 +1595,12 @@ ssize_t fuse_passthrough_splice_read(struct file *in, loff_t *ppos,
> ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
> struct file *out, loff_t *ppos,
> size_t len, unsigned int flags);
> +struct fuse_backing *fuse_backing_id_get(struct fuse_conn *fc, int backing_id);
> ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma);
> int fuse_passthrough_readdir(struct file *file, struct dir_context *ctx);
>
> +int fuse_inode_set_passthrough(struct inode *inode, int backing_id);
> +
> static inline struct fuse_backing *fuse_inode_passthrough(struct fuse_inode *fi)
> {
> #ifdef CONFIG_FUSE_PASSTHROUGH
> diff --git a/fs/fuse/iomode.c b/fs/fuse/iomode.c
> index f46dfa040e53..4c23ae640624 100644
> --- a/fs/fuse/iomode.c
> +++ b/fs/fuse/iomode.c
> @@ -166,6 +166,37 @@ static void fuse_file_uncached_io_release(struct fuse_file *ff,
> fuse_inode_uncached_io_end(fi);
> }
>
> +/* Setup passthrough for inode operations without an open file */
> +int fuse_inode_set_passthrough(struct inode *inode, int backing_id)
> +{
> + struct fuse_conn *fc = get_fuse_conn(inode);
> + struct fuse_inode *fi = get_fuse_inode(inode);
> + struct fuse_backing *fb;
> + int err;
> +
> + if (!IS_ENABLED(CONFIG_FUSE_PASSTHROUGH) || !fc->passthrough_ino)
> + return 0;
> +
> + /* backing inode is set once for the lifetime of the inode */
> + if (fuse_inode_passthrough(fi))
> + return 0;
> +
> + fb = fuse_backing_id_get(fc, backing_id);
> + err = PTR_ERR(fb);
> + if (IS_ERR(fb))
> + goto fail;
> +
> + fi->fb = fb;
> + set_bit(FUSE_I_PASSTHROUGH, &fi->state);
> + fi->iocachectr--;
These need a lock. My patch calls fuse_inode_uncached_io_start()
Why did you change that?
My patch also requires a minimal passthrough op:
/* Backing inode requires at least GETATTR op passthrough */
err = -EOPNOTSUPP;
if (!(fb->ops_mask & FUSE_PASSTHROUGH_OP(FUSE_GETATTR)))
goto fail;
Why did you remove this? it seems sane to me not to ever have to deal
with inode attr cache when setting up passthrough to inode operations.
I've split my patch to prep patch that adds those helpers and the demo API.
push to branch fuse_passthrough_iops on my github.
Please use the prep patch as is without changing the helper functions
unless you have a good reason to change them and document this reason.
> + return 0;
> +
> +fail:
> + pr_debug("failed to setup backing inode (ino=%lu, backing_id=%d, err=%i).\n",
> + inode->i_ino, backing_id, err);
> + return err;
> +}
> +
> /*
> * Open flags that are allowed in combination with FOPEN_PASSTHROUGH.
> * A combination of FOPEN_PASSTHROUGH and FOPEN_DIRECT_IO means that read/write
> @@ -185,8 +216,10 @@ static int fuse_file_passthrough_open(struct inode *inode, struct file *file)
> int err;
>
> /* Check allowed conditions for file open in passthrough mode */
> - if (!IS_ENABLED(CONFIG_FUSE_PASSTHROUGH) || !fc->passthrough ||
> - (ff->open_flags & ~FOPEN_PASSTHROUGH_MASK))
> + if (!IS_ENABLED(CONFIG_FUSE_PASSTHROUGH) || !fc->passthrough)
> + return -EINVAL;
> +
> + if (ff->open_flags & ~FOPEN_PASSTHROUGH_MASK && !fuse_inode_backing(get_fuse_inode(inode)))
I don't understand this condition.
If there is already a backing inode, then open_flags are ignored?
This does not make sense to me.
What is the reason to make this change?
> return -EINVAL;
>
> fb = fuse_passthrough_open(file, inode,
> @@ -224,8 +257,8 @@ int fuse_file_io_open(struct file *file, struct inode *inode)
> * which is already open for passthrough.
> */
> err = -EINVAL;
> - if (fuse_inode_backing(fi) && !(ff->open_flags & FOPEN_PASSTHROUGH))
> - goto fail;
> + if (fuse_inode_backing(fi))
> + ff->open_flags |= FOPEN_PASSTHROUGH;
I agree that it makes some sense for an inode in passthrough mode
to imply FOPEN_PASSTHROUGH, as long as FOPEN_DIRECT_IO
is not cleared, but what do we really gain from making this flag implicit?
If a server is setting up backing inodes, what's the problem with keeping
existing API and requiring the server to use explicit FOPEN_PASSTHROUGH
flag for the inodes that it had mapped?
I understand that you want the server to be able to use backing_id 0
for open for simplicity in this case?
Nevertheless, I see no reason to drop the flag.
>
> /*
> * FOPEN_PARALLEL_DIRECT_WRITES requires FOPEN_DIRECT_IO.
> diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c
> index de6ece996ff8..cee40e1c6e4a 100644
> --- a/fs/fuse/passthrough.c
> +++ b/fs/fuse/passthrough.c
> @@ -229,7 +229,6 @@ static int fuse_backing_id_free(int id, void *p, void *data)
> {
> struct fuse_backing *fb = p;
>
> - WARN_ON_ONCE(refcount_read(&fb->count) != 1);
Why remove this assertion?
Did you change the refcounting rules?
Why? and if so, please document the change.
> fuse_backing_free(fb);
> return 0;
> }
> @@ -348,6 +347,29 @@ int fuse_backing_close(struct fuse_conn *fc, int backing_id)
> return err;
> }
>
> +/*
> + * Get fuse backing object by backing id.
> + *
> + * Returns an fb object with elevated refcount to be stored in fuse inode.
> + */
> +struct fuse_backing *fuse_backing_id_get(struct fuse_conn *fc, int backing_id)
> +{
> + struct fuse_backing *fb;
> +
> + if (backing_id <= 0)
> + return ERR_PTR(-EINVAL);
> +
> + rcu_read_lock();
> + fb = idr_find(&fc->backing_files_map, backing_id);
> + fb = fuse_backing_get(fb);
> + rcu_read_unlock();
> +
> + if (!fb)
> + return ERR_PTR(-ENOENT);
> +
> + return fb;
> +}
> +
> /*
> * Setup passthrough to a backing file.
> *
> @@ -363,18 +385,18 @@ struct fuse_backing *fuse_passthrough_open(struct file *file,
> struct file *backing_file;
> int err;
>
> - err = -EINVAL;
> - if (backing_id <= 0)
> - goto out;
> -
> rcu_read_lock();
> - fb = idr_find(&fc->backing_files_map, backing_id);
> + if (backing_id <= 0) {
> + err = -EINVAL;
> + fb = fuse_inode_backing(get_fuse_inode(inode));
> + } else {
> + err = -ENOENT;
> + fb = idr_find(&fc->backing_files_map, backing_id);
> + }
> fb = fuse_backing_get(fb);
> - rcu_read_unlock();
> -
> - err = -ENOENT;
Maybe you are over complicating this?
My patch replaced all of the above with:
fb = fuse_backing_id_get(fc, backing_id);
if (IS_ERR(fb)) {
err = PTR_ERR(fb);
fb = NULL;
}
I think you want something like:
if (!backing_id) {
fb = fuse_backing_get(fuse_inode_passthrough(fi));
} else {
fb = fuse_backing_id_get(fc, backing_id);
}
if (IS_ERR_OR_NULL(fb)) {
err = fb ? PTR_ERR(fb) : -ENOENT;
fb = NULL;
}
But also, that is a separate API change.
You should do it in its own patch that documents the change
and not in the same patch that implements the backing inode
setup on lookup response.
My patches added fuse_inode_passthrough() exactly so you can
write code that checks if inode has permanent backing inode
*before* you add the API to make this association.
Thanks,
Amir.
Powered by blists - more mailing lists