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@xxxxxxxxxx>
> > > > > > > ---
> > > > > > > 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
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
