Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Feedback-ID: i31e841b0:Fastmail
Message-ID: <ce5106f6-08ee-43e7-6239-6a46992649d4@themaw.net>
Date:   Tue, 26 Sep 2023 14:53:17 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 7/8] autofs: convert autofs to use the new mount api
Content-Language: en-US
From:   Ian Kent <raven@themaw.net>
To:     Christian Brauner <brauner@kernel.org>
Cc:     Al Viro <viro@ZenIV.linux.org.uk>,
        autofs mailing list <autofs@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Bill O'Donnell <billodo@redhat.com>,
        Miklos Szeredi <miklos@szeredi.hu>,
        David Howells <dhowells@redhat.com>
References: <20230922041215.13675-1-raven@themaw.net>
 <20230922041215.13675-8-raven@themaw.net>
 <20230922-vorbringen-spaghetti-946729122076@brauner>
 <7121dcf7-23fe-07a5-9df4-9ea7af6f5964@themaw.net>
In-Reply-To: <7121dcf7-23fe-07a5-9df4-9ea7af6f5964@themaw.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 22/9/23 21:27, Ian Kent wrote:
> On 22/9/23 19:59, Christian Brauner wrote:
>>> +    fsparam_fd    ("fd", Opt_fd),
>>> +/*
>>> + * Open the fd.  We do it here rather than in get_tree so that it's 
>>> done in the
>>> + * context of the system call that passed the data and not the one 
>>> that
>>> + * triggered the superblock creation, lest the fd gets reassigned.
>>> + */
>>> +static int autofs_parse_fd(struct fs_context *fc, int fd)
>>>   {
>>> +    struct autofs_sb_info *sbi = fc->s_fs_info;
>>>       struct file *pipe;
>>>       int ret;
>>>         pipe = fget(fd);
>>>       if (!pipe) {
>>> -        pr_err("could not open pipe file descriptor\n");
>>> +        errorf(fc, "could not open pipe file descriptor");
>>>           return -EBADF;
>>>       }
>>>         ret = autofs_check_pipe(pipe);
>>>       if (ret < 0) {
>>> -        pr_err("Invalid/unusable pipe\n");
>>> +        errorf(fc, "Invalid/unusable pipe");
>>>           fput(pipe);
>>>           return -EBADF;
>>>       }
>>> +static int autofs_parse_param(struct fs_context *fc, struct 
>>> fs_parameter *param)
>>>   {
>>> +        return autofs_parse_fd(fc, result.int_32);
>> Mah, so there's a difference between the new and the old mount api 
>> that we
>> should probably hide on the VFS level for fsparam_fd. Basically, if 
>> you're
>> coming through the new mount api via fsconfig(FSCONFIG_SET_FD, fd) 
>> then the vfs
>> will have done param->file = fget(fd) for you already so there's no 
>> need to
>> call fget() again. We can just take ownership of the reference that 
>> the vfs
>> took for us.
>>
>> But if we're coming in through the old mount api then we need to call 
>> fget.
>> There's nothing wrong with your code but it doesn't take advantage of 
>> the new
>> mount api which would be unfortunate. So I folded a small extension 
>> into this
>> see [1].
>>
>> There's an unrelated bug in fs_param_is_fd() that I'm also fixing see 
>> [2].
>
> Ok, that's interesting, I'll have a look at these developments tomorrow,
>
> admittedly (obviously) I hadn't looked at the code for some time ...

This code pattern is a bit unusual, it looks a bit untidy to have different

behavior between the two but I expect there was a reason to include the fd

handling and have this small difference anyway ...


In [2] that's a good catch, I certainly was caught by it, ;(


>
>
> Ian
>
>>
>> I've tested both changes with the old and new mount api.
>>
>> [1]:
>> diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
>> index 3f2dfed428f9..0477bce7d277 100644
>> --- a/fs/autofs/inode.c
>> +++ b/fs/autofs/inode.c
>> @@ -150,13 +150,20 @@ struct autofs_fs_context {
>>    * context of the system call that passed the data and not the one 
>> that
>>    * triggered the superblock creation, lest the fd gets reassigned.
>>    */
>> -static int autofs_parse_fd(struct fs_context *fc, int fd)
>> +static int autofs_parse_fd(struct fs_context *fc, struct 
>> autofs_sb_info *sbi,
>> +                          struct fs_parameter *param,
>> +                          struct fs_parse_result *result)
>>   {
>> -       struct autofs_sb_info *sbi = fc->s_fs_info;
>>          struct file *pipe;
>>          int ret;
>>
>> -       pipe = fget(fd);
>> +       if (param->type == fs_value_is_file) {
>> +               /* came through the new api */
>> +               pipe = param->file;
>> +               param->file = NULL;
>> +       } else {
>> +               pipe = fget(result->uint_32);
>> +       }
>>          if (!pipe) {
>>                  errorf(fc, "could not open pipe file descriptor");
>>                  return -EBADF;
>> @@ -165,14 +172,15 @@ static int autofs_parse_fd(struct fs_context 
>> *fc, int fd)
>>          ret = autofs_check_pipe(pipe);
>>          if (ret < 0) {
>>                  errorf(fc, "Invalid/unusable pipe");
>> -               fput(pipe);
>> +               if (param->type != fs_value_is_file)
>> +                       fput(pipe);
>>                  return -EBADF;
>>          }
>>
>>          if (sbi->pipe)
>>                  fput(sbi->pipe);
>>
>> -       sbi->pipefd = fd;
>> +       sbi->pipefd = result->uint_32;
>>          sbi->pipe = pipe;
>>
>>          return 0;
>>
>> [2]:
>>  From 2f9171200505c82e744a235c85377e36ed190109 Mon Sep 17 00:00:00 2001
>> From: Christian Brauner <brauner@kernel.org>
>> Date: Fri, 22 Sep 2023 13:49:05 +0200
>> Subject: [PATCH] fsconfig: ensure that dirfd is set to aux
>>
>> The code in fs_param_is_fd() expects param->dirfd to be set to the fd
>> that was used to set param->file to initialize result->uint_32. So make
>> sure it's set so users like autofs using FSCONFIG_SET_FD with the new
>> mount api can rely on this to be set to the correct value.
>>
>> Signed-off-by: Christian Brauner <brauner@kernel.org>
>> ---
>>   fs/fsopen.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/fsopen.c b/fs/fsopen.c
>> index ce03f6521c88..6593ae518115 100644
>> --- a/fs/fsopen.c
>> +++ b/fs/fsopen.c
>> @@ -465,6 +465,7 @@ SYSCALL_DEFINE5(fsconfig,
>>           param.file = fget(aux);
>>           if (!param.file)
>>               goto out_key;
>> +        param.dirfd = aux;
>>           break;
>>       default:
>>           break;