Re: [PATCH RFC v5 05/16] fuse: make args->in_args[0] to be always the header

[Joanne Koong <joannelkoong@gmail.com>]

On Thu, Nov 7, 2024 at 9:04 AM Bernd Schubert <bschubert@ddn.com> wrote:
>
> This change sets up FUSE operations to have headers in args.in_args[0],
> even for opcodes without an actual header. We do this to prepare for
> cleanly separating payload from headers in the future.
>
> For opcodes without a header, we use a zero-sized struct as a
> placeholder. This approach:
> - Keeps things consistent across all FUSE operations
> - Will help with payload alignment later
> - Avoids future issues when header sizes change
>
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> ---
>  fs/fuse/dax.c    | 13 ++++++++-----
>  fs/fuse/dev.c    | 24 ++++++++++++++++++++----
>  fs/fuse/dir.c    | 41 +++++++++++++++++++++++++++--------------
>  fs/fuse/fuse_i.h |  7 +++++++
>  fs/fuse/xattr.c  |  9 ++++++---
>  5 files changed, 68 insertions(+), 26 deletions(-)
>
> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> index 12ef91d170bb3091ac35a33d2b9dc38330b00948..e459b8134ccb089f971bebf8da1f7fc5199c1271 100644
> --- a/fs/fuse/dax.c
> +++ b/fs/fuse/dax.c
> @@ -237,14 +237,17 @@ static int fuse_send_removemapping(struct inode *inode,
>         struct fuse_inode *fi = get_fuse_inode(inode);
>         struct fuse_mount *fm = get_fuse_mount(inode);
>         FUSE_ARGS(args);
> +       struct fuse_zero_in zero_arg;
>
>         args.opcode = FUSE_REMOVEMAPPING;
>         args.nodeid = fi->nodeid;
> -       args.in_numargs = 2;
> -       args.in_args[0].size = sizeof(*inargp);
> -       args.in_args[0].value = inargp;
> -       args.in_args[1].size = inargp->count * sizeof(*remove_one);
> -       args.in_args[1].value = remove_one;
> +       args.in_numargs = 3;
> +       args.in_args[0].size = sizeof(zero_arg);
> +       args.in_args[0].value = &zero_arg;
> +       args.in_args[1].size = sizeof(*inargp);
> +       args.in_args[1].value = inargp;
> +       args.in_args[2].size = inargp->count * sizeof(*remove_one);
> +       args.in_args[2].value = remove_one;
>         return fuse_simple_request(fm, &args);
>  }
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index dbc222f9b0f0e590ce3ef83077e6b4cff03cff65..6effef4073da3dad2f6140761eca98147a41d88d 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -1007,6 +1007,19 @@ static int fuse_copy_args(struct fuse_copy_state *cs, unsigned numargs,
>
>         for (i = 0; !err && i < numargs; i++)  {
>                 struct fuse_arg *arg = &args[i];
> +
> +               /* zero headers */
> +               if (arg->size == 0) {
> +                       if (WARN_ON_ONCE(i != 0)) {
> +                               if (cs->req)
> +                                       pr_err_once(
> +                                               "fuse: zero size header in opcode %d\n",
> +                                               cs->req->in.h.opcode);
> +                               return -EINVAL;
> +                       }
> +                       continue;
> +               }
> +
>                 if (i == numargs - 1 && argpages)
>                         err = fuse_copy_pages(cs, arg->size, zeroing);
>                 else
> @@ -1662,6 +1675,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode,
>         size_t args_size = sizeof(*ra);
>         struct fuse_args_pages *ap;
>         struct fuse_args *args;
> +       struct fuse_zero_in zero_arg;
>
>         offset = outarg->offset & ~PAGE_MASK;
>         file_size = i_size_read(inode);
> @@ -1688,7 +1702,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode,
>         args = &ap->args;
>         args->nodeid = outarg->nodeid;
>         args->opcode = FUSE_NOTIFY_REPLY;
> -       args->in_numargs = 2;
> +       args->in_numargs = 3;
>         args->in_pages = true;
>         args->end = fuse_retrieve_end;
>
> @@ -1715,9 +1729,11 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode,
>         }
>         ra->inarg.offset = outarg->offset;
>         ra->inarg.size = total_len;
> -       args->in_args[0].size = sizeof(ra->inarg);
> -       args->in_args[0].value = &ra->inarg;
> -       args->in_args[1].size = total_len;
> +       args->in_args[0].size = sizeof(zero_arg);
> +       args->in_args[0].value = &zero_arg;
> +       args->in_args[1].size = sizeof(ra->inarg);
> +       args->in_args[1].value = &ra->inarg;
> +       args->in_args[2].size = total_len;
>
>         err = fuse_simple_notify_reply(fm, args, outarg->notify_unique);
>         if (err)

Do we also need to add a zero arg header for FUSE_READLINK,
FUSE_DESTROY, and FUSE_BATCH_FORGET requests as well?


Thanks,
Joanne

> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 2b0d4781f39484d50d1fd7f4f673d8b08c5fd7cf..6d67d7f8e6b4460c759df3fb293e169bcc78a897 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -172,12 +172,16 @@ 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_zero_in zero_arg;
> +
>         memset(outarg, 0, sizeof(struct fuse_entry_out));
>         args->opcode = FUSE_LOOKUP;
>         args->nodeid = nodeid;
> -       args->in_numargs = 1;
> -       args->in_args[0].size = name->len + 1;
> -       args->in_args[0].value = name->name;
> +       args->in_numargs = 2;
> +       args->in_args[0].size = sizeof(zero_arg);
> +       args->in_args[0].value = &zero_arg;
> +       args->in_args[1].size = name->len + 1;
> +       args->in_args[1].value = name->name;
>         args->out_numargs = 1;
>         args->out_args[0].size = sizeof(struct fuse_entry_out);
>         args->out_args[0].value = outarg;
> @@ -915,16 +919,19 @@ static int fuse_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  static int fuse_symlink(struct mnt_idmap *idmap, struct inode *dir,
>                         struct dentry *entry, const char *link)
>  {
> +       struct fuse_zero_in zero_arg;
>         struct fuse_mount *fm = get_fuse_mount(dir);
>         unsigned len = strlen(link) + 1;
>         FUSE_ARGS(args);
>
>         args.opcode = FUSE_SYMLINK;
> -       args.in_numargs = 2;
> -       args.in_args[0].size = entry->d_name.len + 1;
> -       args.in_args[0].value = entry->d_name.name;
> -       args.in_args[1].size = len;
> -       args.in_args[1].value = link;
> +       args.in_numargs = 3;
> +       args.in_args[0].size = sizeof(zero_arg);
> +       args.in_args[0].value = &zero_arg;
> +       args.in_args[1].size = entry->d_name.len + 1;
> +       args.in_args[1].value = entry->d_name.name;
> +       args.in_args[2].size = len;
> +       args.in_args[2].value = link;
>         return create_new_entry(fm, &args, dir, entry, S_IFLNK);
>  }
>
> @@ -975,6 +982,7 @@ static void fuse_entry_unlinked(struct dentry *entry)
>
>  static int fuse_unlink(struct inode *dir, struct dentry *entry)
>  {
> +       struct fuse_zero_in inarg;
>         int err;
>         struct fuse_mount *fm = get_fuse_mount(dir);
>         FUSE_ARGS(args);
> @@ -984,9 +992,11 @@ static int fuse_unlink(struct inode *dir, struct dentry *entry)
>
>         args.opcode = FUSE_UNLINK;
>         args.nodeid = get_node_id(dir);
> -       args.in_numargs = 1;
> -       args.in_args[0].size = entry->d_name.len + 1;
> -       args.in_args[0].value = entry->d_name.name;
> +       args.in_numargs = 2;
> +       args.in_args[0].size = sizeof(inarg);
> +       args.in_args[0].value = &inarg;
> +       args.in_args[1].size = entry->d_name.len + 1;
> +       args.in_args[1].value = entry->d_name.name;
>         err = fuse_simple_request(fm, &args);
>         if (!err) {
>                 fuse_dir_changed(dir);
> @@ -998,6 +1008,7 @@ static int fuse_unlink(struct inode *dir, struct dentry *entry)
>
>  static int fuse_rmdir(struct inode *dir, struct dentry *entry)
>  {
> +       struct fuse_zero_in zero_arg;
>         int err;
>         struct fuse_mount *fm = get_fuse_mount(dir);
>         FUSE_ARGS(args);
> @@ -1007,9 +1018,11 @@ static int fuse_rmdir(struct inode *dir, struct dentry *entry)
>
>         args.opcode = FUSE_RMDIR;
>         args.nodeid = get_node_id(dir);
> -       args.in_numargs = 1;
> -       args.in_args[0].size = entry->d_name.len + 1;
> -       args.in_args[0].value = entry->d_name.name;
> +       args.in_numargs = 2;
> +       args.in_args[0].size = sizeof(zero_arg);
> +       args.in_args[0].value = &zero_arg;
> +       args.in_args[1].size = entry->d_name.len + 1;
> +       args.in_args[1].value = entry->d_name.name;
>         err = fuse_simple_request(fm, &args);
>         if (!err) {
>                 fuse_dir_changed(dir);
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index f2391961031374d8d55916c326c6472f0c03aae6..e2d1d90dfdb13b2c3e7de4789501ee45d3bf7794 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -941,6 +941,13 @@ struct fuse_mount {
>         struct rcu_head rcu;
>  };
>
> +/*
> + * Empty header for FUSE opcodes without specific header needs.
> + * Used as a placeholder in args->in_args[0] for consistency
> + * across all FUSE operations, simplifying request handling.
> + */
> +struct fuse_zero_in {};
> +
>  static inline struct fuse_mount *get_fuse_mount_super(struct super_block *sb)
>  {
>         return sb->s_fs_info;
> diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c
> index 5b423fdbb13f8f17c3982e96dd0de836662092b0..2df1efd2e9bdb46571148f484d7927044f31c184 100644
> --- a/fs/fuse/xattr.c
> +++ b/fs/fuse/xattr.c
> @@ -158,15 +158,18 @@ int fuse_removexattr(struct inode *inode, const char *name)
>         struct fuse_mount *fm = get_fuse_mount(inode);
>         FUSE_ARGS(args);
>         int err;
> +       struct fuse_zero_in zero_arg;
>
>         if (fm->fc->no_removexattr)
>                 return -EOPNOTSUPP;
>
>         args.opcode = FUSE_REMOVEXATTR;
>         args.nodeid = get_node_id(inode);
> -       args.in_numargs = 1;
> -       args.in_args[0].size = strlen(name) + 1;
> -       args.in_args[0].value = name;
> +       args.in_numargs = 2;
> +       args.in_args[0].size = sizeof(zero_arg);
> +       args.in_args[0].value = &zero_arg;
> +       args.in_args[1].size = strlen(name) + 1;
> +       args.in_args[1].value = name;
>         err = fuse_simple_request(fm, &args);
>         if (err == -ENOSYS) {
>                 fm->fc->no_removexattr = 1;
>
> --
> 2.43.0
>