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