Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i31e841b0:Fastmail
Message-ID: <a9cc2c3d-cc38-4337-a3a6-ceb4c4b1602e@themaw.net>
Date:   Thu, 30 Jun 2022 08:46:14 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [REPOST PATCH] nfs: fix port value parsing
Content-Language: en-US
From:   Ian Kent <raven@themaw.net>
To:     Trond Myklebust <trondmy@hammerspace.com>,
        "Anna.Schumaker@Netapp.com" <Anna.Schumaker@Netapp.com>
Cc:     "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "SteveD@redhat.com" <SteveD@redhat.com>,
        "bcodding@redhat.com" <bcodding@redhat.com>,
        "dhowells@redhat.com" <dhowells@redhat.com>
References: <165637590710.37553.7481596265813355098.stgit@donald.themaw.net>
 <cadcb382d47ef037c5b713b099ae46640dfea37d.camel@hammerspace.com>
 <ccd23a54-27b5-e65c-4a97-b169676c23bc@themaw.net>
 <891563475afc32c49fab757b8b56ecdc45b30641.camel@hammerspace.com>
 <fd23da3f-e242-da15-ab1c-3e53490a8577@themaw.net>
 <c81b95d2b68480ead9f3bb88d6cf5a82a43c73b8.camel@hammerspace.com>
 <2dbeb7b8-8994-d610-f536-58d41767a1ec@themaw.net>
In-Reply-To: <2dbeb7b8-8994-d610-f536-58d41767a1ec@themaw.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 30/6/22 08:41, Ian Kent wrote:
>
> On 30/6/22 07:57, Trond Myklebust wrote:
>> On Thu, 2022-06-30 at 07:33 +0800, Ian Kent wrote:
>>> On 29/6/22 23:33, Trond Myklebust wrote:
>>>> On Wed, 2022-06-29 at 09:02 +0800, Ian Kent wrote:
>>>>> On 28/6/22 22:34, Trond Myklebust wrote:
>>>>>> On Tue, 2022-06-28 at 08:25 +0800, Ian Kent wrote:
>>>>>>> The valid values of nfs options port and mountport are 0 to
>>>>>>> USHRT_MAX.
>>>>>>>
>>>>>>> The fs parser will return a fail for port values that are
>>>>>>> negative
>>>>>>> and the sloppy option handling then returns success.
>>>>>>>
>>>>>>> But the sloppy option handling is meant to return success for
>>>>>>> invalid
>>>>>>> options not valid options with invalid values.
>>>>>>>
>>>>>>> Parsing these values as s32 rather than u32 prevents the
>>>>>>> parser
>>>>>>> from
>>>>>>> returning a parse fail allowing the later USHRT_MAX option
>>>>>>> check
>>>>>>> to
>>>>>>> correctly return a fail in this case. The result check could
>>>>>>> be
>>>>>>> changed
>>>>>>> to use the int_32 union variant as well but leaving it as a
>>>>>>> uint_32
>>>>>>> check avoids using two logical compares instead of one.
>>>>>>>
>>>>>>> Signed-off-by: Ian Kent <raven@themaw.net>
>>>>>>> ---
>>>>>>>     fs/nfs/fs_context.c |    4 ++--
>>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
>>>>>>> index 9a16897e8dc6..f4da1d2be616 100644
>>>>>>> --- a/fs/nfs/fs_context.c
>>>>>>> +++ b/fs/nfs/fs_context.c
>>>>>>> @@ -156,14 +156,14 @@ static const struct fs_parameter_spec
>>>>>>> nfs_fs_parameters[] = {
>>>>>>>            fsparam_u32 ("minorversion",  Opt_minorversion),
>>>>>>>            fsparam_string("mountaddr",     Opt_mountaddr),
>>>>>>>            fsparam_string("mounthost",     Opt_mounthost),
>>>>>>> -       fsparam_u32 ("mountport",     Opt_mountport),
>>>>>>> +       fsparam_s32 ("mountport",     Opt_mountport),
>>>>>>>            fsparam_string("mountproto",    Opt_mountproto),
>>>>>>>            fsparam_u32 ("mountvers",     Opt_mountvers),
>>>>>>>            fsparam_u32 ("namlen",        Opt_namelen),
>>>>>>>            fsparam_u32 ("nconnect",      Opt_nconnect),
>>>>>>>            fsparam_u32 ("max_connect",   Opt_max_connect),
>>>>>>>            fsparam_string("nfsvers",       Opt_vers),
>>>>>>> -       fsparam_u32   ("port",          Opt_port),
>>>>>>> +       fsparam_s32   ("port",          Opt_port),
>>>>>>>            fsparam_flag_no("posix",        Opt_posix),
>>>>>>>            fsparam_string("proto",         Opt_proto),
>>>>>>>            fsparam_flag_no("rdirplus",     Opt_rdirplus),
>>>>>>>
>>>>>>>
>>>>>> Why don't we just check for the ENOPARAM return value from
>>>>>> fs_parse()?
>>>>> In this case I think the return will be EINVAL.
>>>> My point is that 'sloppy' is only supposed to work to suppress the
>>>> error in the case where an option is not found by the parser. That
>>>> corresponds to the error ENOPARAM.
>>> Well, yes, and that's why ENOPARAM isn't returned and shouldn't be.
>>>
>>> And if the sloppy option is given it doesn't get to check the value
>>>
>>> of the option, it just returns success which isn't right.
>>>
>>>
>>>>> I think that's a bit to general for this case.
>>>>>
>>>>> This seemed like the most sensible way to fix it.
>>>>>
>>>> Your patch works around just one symptom of the problem instead of
>>>> addressing the root cause.
>>>>
>>> Ok, how do you recommend I fix this?
>>>
>> Maybe I'm missing something, but why not this?
>>
>> 8<--------------------------------
>> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
>> index 9a16897e8dc6..8f1f9b4af89d 100644
>> --- a/fs/nfs/fs_context.c
>> +++ b/fs/nfs/fs_context.c
>> @@ -484,7 +484,7 @@ static int nfs_fs_context_parse_param(struct
>> fs_context *fc,
>>         opt = fs_parse(fc, nfs_fs_parameters, param, &result);
>>       if (opt < 0)
>> -        return ctx->sloppy ? 1 : opt;
>> +        return (opt == -ENOPARAM && ctx->sloppy) ? 1 : opt;
>
>
> Right but fs_parse() will return EINVAL in the case where a valid option
>
> is used but its value is wrong such as where the value given is negative
>
> but the param definition is unsigned (causing the EINVAL).
>
> Of course this case is checked for and handled later in the NFS option
>
> handling ...


Oh wait ... I think I've been too hasty and not understood what you

suggested ... let me ponder that a little ... and thanks for the suggestion.


Ian

>
>
> There's also the question of option ordering which I haven't looked at
>
> closely yet but might not be working properly.
>
>
> Ian
>
>>         if (fc->security)
>>           ctx->has_sec_mnt_opts = 1;
>>