| Message ID | 165637590710.37553.7481596265813355098.stgit@donald.themaw.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [REPOST] nfs: fix port value parsing | expand |
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()?
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. I think that's a bit to general for this case. This seemed like the most sensible way to fix it. Ian
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. > > 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.
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
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;
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 ... 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; >
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; >>
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
Hi Ian, On Fri, 2022-07-01 at 14:10 +0800, 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; > > > > 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? > "Suggested-by:" would be fine. Cheers Trond
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),
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(-)