Re: [REPOST PATCH] nfs: fix port value parsing

[Ian Kent <raven@themaw.net>]

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;
>>