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: <fd23da3f-e242-da15-ab1c-3e53490a8577@themaw.net>
Date:   Thu, 30 Jun 2022 07:33:46 +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>
From:   Ian Kent <raven@themaw.net>
In-Reply-To: <891563475afc32c49fab757b8b56ecdc45b30641.camel@hammerspace.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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?


Ian