Re: [regression?] escaping commas in overlayfs mount options

[Christian Brauner <brauner@kernel.org>]

On Thu, Oct 12, 2023 at 12:27:26PM +0300, Amir Goldstein wrote:
> On Thu, Oct 12, 2023 at 11:26 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > > > Christian,
> > > >
> > > > Do you know any userspace that already uses your new append prefixes?
> > > > Do we have any good reason to support "lowerdir_first"
> > > > so a lower dir stack could be reset before creating the sb?
> > >
> > > If that is a requirement, I suggest extending fsconfig(2) to allow
> > > resetting an option.
> >
> > Overlayfs does already support this. If you pass:
> > fsconfig(FSCONFIG_SET_STRING, "lowerdir", "", ...)
> > then the lower layer stack is reset. I've implemented it that way in
> > ovl_parse_param_lowerdir().
> >
> 
> Yes, I noticed that. Cool.
> 
> > >
> > > > > > > >
> > > > > > > > Anyway, let's focus on what you would like best.
> > > > > > > > If you prefer to just fix the regression, it is doable.
> > > > > > > > If you prefer the upperdirfd, workdirfd, lowerdirfd API, I think we can
> > > > > > > > find a volunteer to write it up.
> > >
> > > Can't the existing option names be overloaded if a separate cmd
> > > (FSCONFIG_SET_PATH or FSCONFIG_SET_PATH_EMPTY) is used in fsconfig()?
> >
> > Yes, they can and filesystems do do that today depending on whether they
> > want to e.g., take an fd or a path or something.
> 
> Nice. It seems like Miklos has volunteered to implement the
> dirfd and/or unescaped API variants for the new mount API :)
> 
> What is your opinion about the original regression report
> regarding escaping of commas in ->parse_monolithic()?
> 
> It's easy to implement ovl_parse_monolithic() that will
> conform to the old ovl_next_opt() behavior, but it does not
> solve the problem long term.
> 
> If there are currently setups in the wild that pass arguments
> like [lowerdir=/tmp/a\,b/], even if I do fix up ovl_parse_monolithic()
> those setups will regress when they upgrade to libmount v2.39,
> because AFAICT, libmount does not respect "\," to escape option split,
> it respects [lowerdir="/tmp/a,b/"] to escape option split.

For full backward compatibility we would probably need to fix both the
kernel and libmount. Because libmount/mount(8) is encouraged to split a
lowerdir=/a:/b:/c:/d option into separate fsconfig calls, especially
when dealing with really long paths. So libmount would need to be aware
of overlayfs parsing behavior that includes escaping \, even if we fix
the kernel itself.

I don't think that would be a big deal because libmount already has to
deal with all kinds of filesystems specific quirks.

However, libmount also added LIBMOUNT_FORCE_MOUNT2={always,never,auto}
which can be used to disable using the new mount api and makes it use
the old mount api which is available in libmount 2.39.

So I think complementing overlayfs with a ->parse_monolithic() option
might be something that we could consider doing but this is a judgement
call there's not clear right and wrong with so many moving parts...

> 
> If we do decide that we need to or want to fix ->parse_monolithic()
> then do you think it would make sense to respect "\," escaping in
> generic_parse_monolithic()?
> I cannot imagine any workload that would get regressed by this
> (famous last words).

I'm pretty sure that this would break something so I would be hesitant
doing this.