Re: [PATCH] vfs: parse sloppy mount option in correct order

[David Wysochanski <dwysocha@redhat.com>]

On Thu, Aug 26, 2021 at 9:32 AM Roberto Bergantinos Corpas
<rbergant@redhat.com> wrote:
>
> On Thu, Aug 26, 2021 at 3:13 PM Eric Sandeen <sandeen@sandeen.net> wrote:
> >
> > On 8/26/21 7:46 AM, Matthew Wilcox wrote:
> > > On Wed, Jul 21, 2021 at 01:30:57PM +0200, Roberto Bergantinos Corpas wrote:
> > >> With addition of fs_context support, options string is parsed
> > >> sequentially, if 'sloppy' option is not leftmost one, we may
> > >> return ENOPARAM to userland if a non-valid option preceeds sloopy
> > >> and mount will fail :
> > >>
> > >> host# mount -o quota,sloppy 172.23.1.225:/share /mnt
> > >> mount.nfs: an incorrect mount option was specified
> > >> host# mount -o sloppy,quota 172.23.1.225:/share /mnt
> > >
> > > It isn't clear to me that this is incorrect behaviour.  Perhaps the user
> > > actually wants the options to the left parsed strictly and the options
> > > to the right parsed sloppily?
> >
> > I don't think mount options have ever been order-dependent, at least not
> > intentionally so, have they?
> >
> > And what matters most here is surely "how did it work before the mount
> > API change?"
> >> And it seems to me that previously, invalid options were noted, and whether the
> > mount would fail was left to the end, depending on whether sloppy was seen
> > anywhere in the mount options string.  This is the old option parsing:
> >
> >          while ((p = strsep(&raw, ",")) != NULL) {
> > ...
> >                  switch (token) {
> > ...
> >                  case Opt_sloppy:
> >                          sloppy = 1;
> >                          dfprintk(MOUNT, "NFS:   relaxing parsing rules\n");
> >                          break;
> > ...
> >                  default:
> >                          invalid_option = 1;
> >                          dfprintk(MOUNT, "NFS:   unrecognized mount option "
> >                                          "'%s'\n", p);
> >                  }
> >          }
> >
> >          if (!sloppy && invalid_option)
> >                  return 0;
>
>  Agree, that's my main point. I think that breaks from previous
> behaviour and indeed causes issues on the field.
> My understanding too is that there's no order-dependency.
>
> roberto
>
Hi Roberto, Eric and David H, this seems to have stalled.  What are
the next steps here?

Roberto, it sounded like David H was suggesting maybe an alternative
approach.  Did you look into that?

Then there was a discussion about whether this change was intended
behavior or not, and the main point made was it was a change in
behavior, which I don't think was intended.  I guess one could make
the argument order was never guaranteed, but then why make behavior
order dependent now if it was not intentional kernel change?  I agree
if possible we should retain the order independent behavior in the
kernel.