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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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






[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux