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: <22f9fbb7-a557-f372-7ede-92f0af338bd1@themaw.net>
Date:   Fri, 1 Jul 2022 14:10:41 +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
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>
From:   Ian Kent <raven@themaw.net>
In-Reply-To: <c81b95d2b68480ead9f3bb88d6cf5a82a43c73b8.camel@hammerspace.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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;
>   
>   	if (fc->security)
>   		ctx->has_sec_mnt_opts = 1;
>
I tested this with the autofs connectathon tests I use which has lots of

success and fail cases. As expected there were no surprises, the tests

worked fine and gave the expected results.


I'll send an updated patch, is a "Suggested-by" attribution sufficient

or would you like something different?


Ian