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

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 11/14/24 21:57, Joanne Koong wrote:
> 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 for looking at the patch! I should have added to the commit message
that I didn't modify these, as they don't have an in argument at all.


Thanks,
Bernd