[regression?] escaping commas in overlayfs mount options

[regression?] escaping commas in overlayfs mount options

[Ryan Hendrickson]

Up to and including kernel 6.4.15, it was possible to have commas in 
the lowerdir/upperdir/workdir paths used by overlayfs, provided they were 
escaped with backslashes:

     mkdir /tmp/test-lower, /tmp/test-upper /tmp/test-work /tmp/test
     mount -t overlay overlay -o 'lowerdir=/tmp/test-lower\,,upperdir=/tmp/test-upper,workdir=/tmp/test-work' /tmp/test

In 6.5.2 and 6.5.5, this no longer works; dmesg reports that overlayfs 
can't resolve '/tmp/test-lower' (without the comma).

I see that there is a commit between the 6.4 and 6.5 lines titled [ovl: 
port to new mount api][1]. I haven't compiled a kernel before and after 
this commit to verify, but based on the code it deletes I strongly suspect 
that it, or if not then one of the ovl commits committed on the same day, 
is responsible for this change.

[1]: https://github.com/torvalds/linux/commit/1784fbc2ed9c888ea4e895f30a53207ed7ee8208

Does this count as a regression? I can't find documentation for this 
escaping feature anywhere, even as it pertains to the non-comma characters 
'\\' and ':' (which, I've tested, can still be escaped as expected), so 
perhaps it was never properly supported? But a search for escaping commas 
in overlayfs turns up resources like [this post][2], suggesting that there 
are others who figured this out and expect it to work.

[2]: https://unix.stackexchange.com/a/552640

Is there a new way to escape commas for overlayfs options?

Thanks,
Ryan

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

[Amir Goldstein]

On Fri, Sep 29, 2023 at 4:08 AM Ryan Hendrickson
<ryan.hendrickson@alum.mit.edu> wrote:
>
> Up to and including kernel 6.4.15, it was possible to have commas in
> the lowerdir/upperdir/workdir paths used by overlayfs, provided they were
> escaped with backslashes:
>
>      mkdir /tmp/test-lower, /tmp/test-upper /tmp/test-work /tmp/test
>      mount -t overlay overlay -o 'lowerdir=/tmp/test-lower\,,upperdir=/tmp/test-upper,workdir=/tmp/test-work' /tmp/test
>
> In 6.5.2 and 6.5.5, this no longer works; dmesg reports that overlayfs
> can't resolve '/tmp/test-lower' (without the comma).
>
> I see that there is a commit between the 6.4 and 6.5 lines titled [ovl:
> port to new mount api][1]. I haven't compiled a kernel before and after
> this commit to verify, but based on the code it deletes I strongly suspect
> that it, or if not then one of the ovl commits committed on the same day,
> is responsible for this change.
>
> [1]: https://github.com/torvalds/linux/commit/1784fbc2ed9c888ea4e895f30a53207ed7ee8208
>

That's a good guess.
It helps to CC the author of the patch in this case ;-)

> Does this count as a regression?

"used to work, does not work now" is pretty close to a dictionary
definition of a regression :)

The question is whether we should fix it.
The rule of thumb is that if users complain than we need to fix it,
but it's a corner case and if the only users that complained are willing
to work around the problem (hint hint) then we may not need to fix it.

> I can't find documentation for this
> escaping feature anywhere, even as it pertains to the non-comma characters
> '\\' and ':' (which, I've tested, can still be escaped as expected), so
> perhaps it was never properly supported? But a search for escaping commas
> in overlayfs turns up resources like [this post][2], suggesting that there
> are others who figured this out and expect it to work.
>
> [2]: https://unix.stackexchange.com/a/552640
>
> Is there a new way to escape commas for overlayfs options?
>

Deferring the question to Christian.

Thanks,
Amir.

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

[Ryan Hendrickson]

At 2023-09-29 07:44+0300, Amir Goldstein <amir73il@gmail.com> sent:

> That's a good guess.
> It helps to CC the author of the patch in this case ;-)

Ha, oops! Sorry, I have been spoiled by web-based bug reporting interfaces 
and actually e-mailing maintainers is weirdly a novel experience.

> The question is whether we should fix it.
> The rule of thumb is that if users complain than we need to fix it,
> but it's a corner case and if the only users that complained are willing
> to work around the problem (hint hint) then we may not need to fix it.

Gotcha. In my specific situation, unless there is an alternate escaping 
mechanism or mount API that works identically in both <6.5 and >=6.5, I'm 
going to have to do some version detection to work around the regression 
in released 6.5 kernels regardless of whether the old functionality is 
restored later. As long as there is *some* way of using overlayfs with 
arbitrary path nemes (including commas), I'm fine with whatever you 
decide. (If there isn't currently a way to mount a path with commas in it, 
you may consider me complaining.)

But if the status quo remains, I would like an answer to:

>> Is there a new way to escape commas for overlayfs options?
>>
>
> Deferring the question to Christian.

BTW, in case there's a difference between them, the actual code I'm 
working with uses the libc mount() function, not the mount CLI tool. I've 
also experimented with the functions in libmount but they seems to have 
the same problem with commas. Either is fine as long as I can support 
mounting arbitrary path names.

Thanks,
Ryan

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

[Amir Goldstein]

On Tue, Oct 3, 2023 at 1:23 AM Ryan Hendrickson
<ryan.hendrickson@alum.mit.edu> wrote:
>
> At 2023-09-29 07:44+0300, Amir Goldstein <amir73il@gmail.com> sent:
>
> > That's a good guess.
> > It helps to CC the author of the patch in this case ;-)
>
> Ha, oops! Sorry, I have been spoiled by web-based bug reporting interfaces
> and actually e-mailing maintainers is weirdly a novel experience.
>
> > The question is whether we should fix it.
> > The rule of thumb is that if users complain than we need to fix it,
> > but it's a corner case and if the only users that complained are willing
> > to work around the problem (hint hint) then we may not need to fix it.
>
> Gotcha. In my specific situation, unless there is an alternate escaping
> mechanism or mount API that works identically in both <6.5 and >=6.5, I'm
> going to have to do some version detection to work around the regression
> in released 6.5 kernels regardless of whether the old functionality is
> restored later. As long as there is *some* way of using overlayfs with
> arbitrary path nemes (including commas), I'm fine with whatever you
> decide. (If there isn't currently a way to mount a path with commas in it,
> you may consider me complaining.)
>

What you can do is use the new mount API for kernel >=6.5
and provide the parameters one by one, e.g.:

fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/tmp/test-lower,", 0);

See example in commit message of
b36a5780cb44 ("ovl: modify layer parameter parsing")

> But if the status quo remains, I would like an answer to:
>
> >> Is there a new way to escape commas for overlayfs options?
> >>
> >
> > Deferring the question to Christian.
>
> BTW, in case there's a difference between them, the actual code I'm
> working with uses the libc mount() function, not the mount CLI tool. I've
> also experimented with the functions in libmount but they seems to have
> the same problem with commas. Either is fine as long as I can support
> mounting arbitrary path names.
>

The mount tool and libmount in util-linux 2.39 support the new mount API [1].
They already auto detect and fallback to the old mount API, so you wouldn't
need to implement per kernel version logic.

I do not know if the new mount tool would split this string on the escaped \,

-o 'lowerdir=/tmp/test-lower\,,upperdir=/tmp/test-upper,workdir=/tmp/test-work'

But the kernel old mount(2) syscall does NOT escape the , when splitting
the options string into parameters.

ovl used to do it with the old mount API:
91c77947133f ("ovl: allow filenames with comma")

But it does not do that after the conversion to the new mount API,
where generic_parse_monolithic() is used for splitting the parameters.
I prefer not to have to override ->parse_monolithic() for this behavior
if it works as desired for you when using new libmount.

Thanks,
Amir.

[1] http://karelzak.blogspot.com/2023/06/util-linux-v239-improved-mount.html

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

[Ryan Hendrickson]

At 2023-10-03 12:50+0300, Amir Goldstein <amir73il@gmail.com> sent:

> What you can do is use the new mount API for kernel >=6.5
> and provide the parameters one by one, e.g.:
>
> fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/tmp/test-lower,", 0);
>
> See example in commit message of
> b36a5780cb44 ("ovl: modify layer parameter parsing")

That would be great, but fsconfig doesn't appear to be documented. It's a 
new syscall? What kernel version was it introduced in, and how am I 
supposed to support older kernels?

The second part of your message suggests that the answer to older kernel 
support is libmount:

> The mount tool and libmount in util-linux 2.39 support the new mount API 
> [1]. They already auto detect and fallback to the old mount API, so you 
> wouldn't need to implement per kernel version logic.

Again sounds great, but in the libmount documentation I've been looking at 
[1], there doesn't seem to be a function for setting options individually. 
mnt_context_set_options accepts a comma-delimited string, and 
mnt_optstr_set_option modifies a comma-delimited string. What am I 
missing?

Thanks,
Ryan


[1] https://cdn.kernel.org/pub/linux/utils/util-linux/v2.39/libmount-docs/index.html

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

[Amir Goldstein]

On Tue, Oct 3, 2023 at 10:07 PM Ryan Hendrickson
<ryan.hendrickson@alum.mit.edu> wrote:
>
> At 2023-10-03 12:50+0300, Amir Goldstein <amir73il@gmail.com> sent:
>
> > What you can do is use the new mount API for kernel >=6.5
> > and provide the parameters one by one, e.g.:
> >
> > fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/tmp/test-lower,", 0);
> >
> > See example in commit message of
> > b36a5780cb44 ("ovl: modify layer parameter parsing")
>
> That would be great, but fsconfig doesn't appear to be documented. It's a
> new syscall? What kernel version was it introduced in, and how am I
> supposed to support older kernels?
>

For old kernel would need to use the old mount API.

I don't know about the status of man pages for new mount API,
but you should be able to figure it out from the examples and from
libmount code if you want to try.

> The second part of your message suggests that the answer to older kernel
> support is libmount:
>
> > The mount tool and libmount in util-linux 2.39 support the new mount API
> > [1]. They already auto detect and fallback to the old mount API, so you
> > wouldn't need to implement per kernel version logic.
>
> Again sounds great, but in the libmount documentation I've been looking at
> [1], there doesn't seem to be a function for setting options individually.
> mnt_context_set_options accepts a comma-delimited string, and
> mnt_optstr_set_option modifies a comma-delimited string. What am I
> missing?

The parsing of the comma-delimited string, if kernel and fs support new
mount API is supposed to be implemented by userspace library.

So the question of whether \, should be escaped in moved from the
kernel to libmount.

I don't know what libmount does - I did not check.
Deferring the question to Karl.

Thanks,
Amir.

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

[Sebastian Wick]

On Fri, Sep 29, 2023 at 07:44:09AM +0300, Amir Goldstein wrote:
> On Fri, Sep 29, 2023 at 4:08 AM Ryan Hendrickson
> <ryan.hendrickson@alum.mit.edu> wrote:
> >
> > Up to and including kernel 6.4.15, it was possible to have commas in
> > the lowerdir/upperdir/workdir paths used by overlayfs, provided they were
> > escaped with backslashes:
> >
> >      mkdir /tmp/test-lower, /tmp/test-upper /tmp/test-work /tmp/test
> >      mount -t overlay overlay -o 'lowerdir=/tmp/test-lower\,,upperdir=/tmp/test-upper,workdir=/tmp/test-work' /tmp/test
> >
> > In 6.5.2 and 6.5.5, this no longer works; dmesg reports that overlayfs
> > can't resolve '/tmp/test-lower' (without the comma).
> >
> > I see that there is a commit between the 6.4 and 6.5 lines titled [ovl:
> > port to new mount api][1]. I haven't compiled a kernel before and after
> > this commit to verify, but based on the code it deletes I strongly suspect
> > that it, or if not then one of the ovl commits committed on the same day,
> > is responsible for this change.
> >
> > [1]: https://github.com/torvalds/linux/commit/1784fbc2ed9c888ea4e895f30a53207ed7ee8208
> >
> 
> That's a good guess.
> It helps to CC the author of the patch in this case ;-)
> 
> > Does this count as a regression?
> 
> "used to work, does not work now" is pretty close to a dictionary
> definition of a regression :)
> 
> The question is whether we should fix it.
> The rule of thumb is that if users complain than we need to fix it,
> but it's a corner case and if the only users that complained are willing
> to work around the problem (hint hint) then we may not need to fix it.

It would be nice to have this fixed. A more general question: will you
commit on keeping the escaping stable from now on or do we have to
expect changes at any point in the future?

In that case we would just reject any string contianing characters that
need escaping.

> > I can't find documentation for this
> > escaping feature anywhere, even as it pertains to the non-comma characters
> > '\\' and ':' (which, I've tested, can still be escaped as expected), so
> > perhaps it was never properly supported? But a search for escaping commas
> > in overlayfs turns up resources like [this post][2], suggesting that there
> > are others who figured this out and expect it to work.
> >
> > [2]: https://unix.stackexchange.com/a/552640
> >
> > Is there a new way to escape commas for overlayfs options?
> >
> 
> Deferring the question to Christian.
> 
> Thanks,
> Amir.

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

[Amir Goldstein]

On Fri, Oct 6, 2023 at 4:03 PM Sebastian Wick <sebastian.wick@redhat.com> wrote:
>
> On Fri, Sep 29, 2023 at 07:44:09AM +0300, Amir Goldstein wrote:
> > On Fri, Sep 29, 2023 at 4:08 AM Ryan Hendrickson
> > <ryan.hendrickson@alum.mit.edu> wrote:
> > >
> > > Up to and including kernel 6.4.15, it was possible to have commas in
> > > the lowerdir/upperdir/workdir paths used by overlayfs, provided they were
> > > escaped with backslashes:
> > >
> > >      mkdir /tmp/test-lower, /tmp/test-upper /tmp/test-work /tmp/test
> > >      mount -t overlay overlay -o 'lowerdir=/tmp/test-lower\,,upperdir=/tmp/test-upper,workdir=/tmp/test-work' /tmp/test
> > >
> > > In 6.5.2 and 6.5.5, this no longer works; dmesg reports that overlayfs
> > > can't resolve '/tmp/test-lower' (without the comma).
> > >
> > > I see that there is a commit between the 6.4 and 6.5 lines titled [ovl:
> > > port to new mount api][1]. I haven't compiled a kernel before and after
> > > this commit to verify, but based on the code it deletes I strongly suspect
> > > that it, or if not then one of the ovl commits committed on the same day,
> > > is responsible for this change.
> > >
> > > [1]: https://github.com/torvalds/linux/commit/1784fbc2ed9c888ea4e895f30a53207ed7ee8208
> > >
> >
> > That's a good guess.
> > It helps to CC the author of the patch in this case ;-)
> >
> > > Does this count as a regression?
> >
> > "used to work, does not work now" is pretty close to a dictionary
> > definition of a regression :)
> >
> > The question is whether we should fix it.
> > The rule of thumb is that if users complain than we need to fix it,
> > but it's a corner case and if the only users that complained are willing
> > to work around the problem (hint hint) then we may not need to fix it.
>
> It would be nice to have this fixed. A more general question: will you
> commit on keeping the escaping stable from now on or do we have to
> expect changes at any point in the future?
>

I prefer that escaping would be handled in userspace, now that the new
mount API allows that, so deferring the question to libmount maintainer.

Thanks,
Amir.

> In that case we would just reject any string contianing characters that
> need escaping.
>
> > > I can't find documentation for this
> > > escaping feature anywhere, even as it pertains to the non-comma characters
> > > '\\' and ':' (which, I've tested, can still be escaped as expected), so
> > > perhaps it was never properly supported? But a search for escaping commas
> > > in overlayfs turns up resources like [this post][2], suggesting that there
> > > are others who figured this out and expect it to work.
> > >
> > > [2]: https://unix.stackexchange.com/a/552640
> > >
> > > Is there a new way to escape commas for overlayfs options?
> > >
> >
> > Deferring the question to Christian.
> >
> > Thanks,
> > Amir.
>

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

[Ryan Hendrickson]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

At 2023-10-06 19:17+0300, Amir Goldstein <amir73il@gmail.com> sent:

> On Fri, Oct 6, 2023 at 4:03 PM Sebastian Wick 
> <sebastian.wick@redhat.com> wrote:
>>
>> It would be nice to have this fixed. A more general question: will you
>> commit on keeping the escaping stable from now on or do we have to
>> expect changes at any point in the future?
>>
>
> I prefer that escaping would be handled in userspace, now that the new
> mount API allows that, so deferring the question to libmount maintainer.

Note that with overlayfs on the new mount API, there are now two levels of 
escaping to consider:

There is the escaping that needs to happen for commas when splitting mount 
parameters; this could be handled in libmount when using the new API.

And there is the escaping that needs to happen for ':' and '\' when 
parsing the path parameters (':' is only special syntax in lowerdir, but 
the escaping logic seems to apply to upperdir and workdir as well, based 
on my testing). Even using the new API, this is handled in the kernel. 
We'd like to know if this escaping can be considered stable as well, and I 
don't think that's a question for the libmount maintainer.

Thanks,
Ryan

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

[Amir Goldstein]

On Fri, Oct 6, 2023 at 7:42 PM Ryan Hendrickson
<ryan.hendrickson@alum.mit.edu> wrote:
>
> At 2023-10-06 19:17+0300, Amir Goldstein <amir73il@gmail.com> sent:
>
> > On Fri, Oct 6, 2023 at 4:03 PM Sebastian Wick
> > <sebastian.wick@redhat.com> wrote:
> >>
> >> It would be nice to have this fixed. A more general question: will you
> >> commit on keeping the escaping stable from now on or do we have to
> >> expect changes at any point in the future?
> >>
> >
> > I prefer that escaping would be handled in userspace, now that the new
> > mount API allows that, so deferring the question to libmount maintainer.
>
> Note that with overlayfs on the new mount API, there are now two levels of
> escaping to consider:
>
> There is the escaping that needs to happen for commas when splitting mount
> parameters; this could be handled in libmount when using the new API.
>

Right.

> And there is the escaping that needs to happen for ':' and '\' when
> parsing the path parameters (':' is only special syntax in lowerdir, but
> the escaping logic seems to apply to upperdir and workdir as well, based
> on my testing). Even using the new API, this is handled in the kernel.
> We'd like to know if this escaping can be considered stable as well, and I
> don't think that's a question for the libmount maintainer.

Agree.
Unlike the comma separated parameters list,
upperdir,workdir,lowerdir are overlayfs specific format.

ovl_unescape() (for upperdir/workdir) unescapes '\' characters.
as does ovl_parse_param_split_lowerdirs().
Not sure why this was needed for upperdir/workdir, but it It has
been this way for a long time.
I see no reason for it to change in the future.

Thanks,
Amir.

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

[Miklos Szeredi]

On Fri, 6 Oct 2023 at 19:21, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Oct 6, 2023 at 7:42 PM Ryan Hendrickson

> > And there is the escaping that needs to happen for ':' and '\' when
> > parsing the path parameters (':' is only special syntax in lowerdir, but
> > the escaping logic seems to apply to upperdir and workdir as well, based
> > on my testing). Even using the new API, this is handled in the kernel.
> > We'd like to know if this escaping can be considered stable as well, and I
> > don't think that's a question for the libmount maintainer.
>
> Agree.
> Unlike the comma separated parameters list,
> upperdir,workdir,lowerdir are overlayfs specific format.
>
> ovl_unescape() (for upperdir/workdir) unescapes '\' characters.
> as does ovl_parse_param_split_lowerdirs().
> Not sure why this was needed for upperdir/workdir, but it It has
> been this way for a long time.
> I see no reason for it to change in the future.

Unescaping  upperdir/workdir was the side effect of using a common
helper; it wasn't intentional, I think.  The problem is that
unescaping breaks code that doesn't expect it, and filenames with
backslashes (and especially \\ or \: sequences) are very rare, so this
won't show up in testing.

At this point I'm not sure which is more likely to cause bugs: getting
rid of unescaping or leaving it alone.

One way out of this mess is to create explicit _unesc versions of these options.

Thanks,
Miklos

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

[Amir Goldstein]

On Tue, Oct 10, 2023 at 12:06 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, 6 Oct 2023 at 19:21, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, Oct 6, 2023 at 7:42 PM Ryan Hendrickson
>
> > > And there is the escaping that needs to happen for ':' and '\' when
> > > parsing the path parameters (':' is only special syntax in lowerdir, but
> > > the escaping logic seems to apply to upperdir and workdir as well, based
> > > on my testing). Even using the new API, this is handled in the kernel.
> > > We'd like to know if this escaping can be considered stable as well, and I
> > > don't think that's a question for the libmount maintainer.
> >
> > Agree.
> > Unlike the comma separated parameters list,
> > upperdir,workdir,lowerdir are overlayfs specific format.
> >
> > ovl_unescape() (for upperdir/workdir) unescapes '\' characters.
> > as does ovl_parse_param_split_lowerdirs().
> > Not sure why this was needed for upperdir/workdir, but it It has
> > been this way for a long time.
> > I see no reason for it to change in the future.
>
> Unescaping  upperdir/workdir was the side effect of using a common
> helper; it wasn't intentional, I think.  The problem is that
> unescaping breaks code that doesn't expect it, and filenames with
> backslashes (and especially \\ or \: sequences) are very rare, so this
> won't show up in testing.
>
> At this point I'm not sure which is more likely to cause bugs: getting
> rid of unescaping or leaving it alone.

Considering the fact that the applications that mount overlayfs has
always had to do the correct escaping, getting rid of escaping can
only solve issues in new deployments, so I think we should greatly
favor leaving it alone.

>
> One way out of this mess is to create explicit _unesc versions of these options.
>

I like that solution, with two reservations:
1. IMO, new _unesc versions should only be supported from new mount API
2. I only want to do that if real users exists - said users are expected
    to send the patch and explain their use case

Thanks,
Amir.

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

[Sebastian Wick]

On Tue, Oct 10, 2023 at 12:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Oct 10, 2023 at 12:06 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Fri, 6 Oct 2023 at 19:21, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Fri, Oct 6, 2023 at 7:42 PM Ryan Hendrickson
> >
> > > > And there is the escaping that needs to happen for ':' and '\' when
> > > > parsing the path parameters (':' is only special syntax in lowerdir, but
> > > > the escaping logic seems to apply to upperdir and workdir as well, based
> > > > on my testing). Even using the new API, this is handled in the kernel.
> > > > We'd like to know if this escaping can be considered stable as well, and I
> > > > don't think that's a question for the libmount maintainer.
> > >
> > > Agree.
> > > Unlike the comma separated parameters list,
> > > upperdir,workdir,lowerdir are overlayfs specific format.
> > >
> > > ovl_unescape() (for upperdir/workdir) unescapes '\' characters.
> > > as does ovl_parse_param_split_lowerdirs().
> > > Not sure why this was needed for upperdir/workdir, but it It has
> > > been this way for a long time.
> > > I see no reason for it to change in the future.
> >
> > Unescaping  upperdir/workdir was the side effect of using a common
> > helper; it wasn't intentional, I think.  The problem is that
> > unescaping breaks code that doesn't expect it, and filenames with
> > backslashes (and especially \\ or \: sequences) are very rare, so this
> > won't show up in testing.
> >
> > At this point I'm not sure which is more likely to cause bugs: getting
> > rid of unescaping or leaving it alone.
>
> Considering the fact that the applications that mount overlayfs has
> always had to do the correct escaping, getting rid of escaping can
> only solve issues in new deployments, so I think we should greatly
> favor leaving it alone.

Any change here is a regression. I'm seriously confused why this is
even debated. You already managed to have a regression and I'm still
of the opinion that this should be fixed because it literally breaks
user space.

> >
> > One way out of this mess is to create explicit _unesc versions of these options.
> >
>
> I like that solution, with two reservations:
> 1. IMO, new _unesc versions should only be supported from new mount API
> 2. I only want to do that if real users exists - said users are expected
>     to send the patch and explain their use case

This is confusing me a lot. Why would you not want to provide an API
which is clearly, objectively the better API? As user space, when we
can use the new mount API and we could use this, we absolutely would
use this.

> Thanks,
> Amir.
>

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

[Amir Goldstein]

On Tue, Oct 10, 2023 at 7:13 PM Sebastian Wick
<sebastian.wick@redhat.com> wrote:
>
> On Tue, Oct 10, 2023 at 12:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Oct 10, 2023 at 12:06 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Fri, 6 Oct 2023 at 19:21, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Fri, Oct 6, 2023 at 7:42 PM Ryan Hendrickson
> > >
> > > > > And there is the escaping that needs to happen for ':' and '\' when
> > > > > parsing the path parameters (':' is only special syntax in lowerdir, but
> > > > > the escaping logic seems to apply to upperdir and workdir as well, based
> > > > > on my testing). Even using the new API, this is handled in the kernel.
> > > > > We'd like to know if this escaping can be considered stable as well, and I
> > > > > don't think that's a question for the libmount maintainer.
> > > >
> > > > Agree.
> > > > Unlike the comma separated parameters list,
> > > > upperdir,workdir,lowerdir are overlayfs specific format.
> > > >
> > > > ovl_unescape() (for upperdir/workdir) unescapes '\' characters.
> > > > as does ovl_parse_param_split_lowerdirs().
> > > > Not sure why this was needed for upperdir/workdir, but it It has
> > > > been this way for a long time.
> > > > I see no reason for it to change in the future.
> > >
> > > Unescaping  upperdir/workdir was the side effect of using a common
> > > helper; it wasn't intentional, I think.  The problem is that
> > > unescaping breaks code that doesn't expect it, and filenames with
> > > backslashes (and especially \\ or \: sequences) are very rare, so this
> > > won't show up in testing.
> > >
> > > At this point I'm not sure which is more likely to cause bugs: getting
> > > rid of unescaping or leaving it alone.
> >
> > Considering the fact that the applications that mount overlayfs has
> > always had to do the correct escaping, getting rid of escaping can
> > only solve issues in new deployments, so I think we should greatly
> > favor leaving it alone.
>
> Any change here is a regression. I'm seriously confused why this is
> even debated. You already managed to have a regression and I'm still
> of the opinion that this should be fixed because it literally breaks
> user space.
>

You are right. Literally it does.
But if prospect users are ok with upgrading libmount and if that
solves the problem, I'd rather not have to carry in the kernel
baggage code to support old mount API for a very niche use case.

> > >
> > > One way out of this mess is to create explicit _unesc versions of these options.
> > >
> >
> > I like that solution, with two reservations:
> > 1. IMO, new _unesc versions should only be supported from new mount API
> > 2. I only want to do that if real users exists - said users are expected
> >     to send the patch and explain their use case
>
> This is confusing me a lot. Why would you not want to provide an API
> which is clearly, objectively the better API? As user space, when we
> can use the new mount API and we could use this, we absolutely would
> use this.

I am also confused by this reaction.
Who said that I do not want to provide the _unenc API?

IIUC, you are requesting a new feature that did not exist before,
namely, upperdir_unenc, workdir_unenc, lowerdir_unenc options.
Did I understand correctly?
If that is the case then please send a patch to support
those new options in the new mount API only
including documentation and tests.

Thanks,
Amir.

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

[Sebastian Wick]

On Tue, Oct 10, 2023 at 6:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Oct 10, 2023 at 7:13 PM Sebastian Wick
> <sebastian.wick@redhat.com> wrote:
> >
> > On Tue, Oct 10, 2023 at 12:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Oct 10, 2023 at 12:06 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Fri, 6 Oct 2023 at 19:21, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > On Fri, Oct 6, 2023 at 7:42 PM Ryan Hendrickson
> > > >
> > > > > > And there is the escaping that needs to happen for ':' and '\' when
> > > > > > parsing the path parameters (':' is only special syntax in lowerdir, but
> > > > > > the escaping logic seems to apply to upperdir and workdir as well, based
> > > > > > on my testing). Even using the new API, this is handled in the kernel.
> > > > > > We'd like to know if this escaping can be considered stable as well, and I
> > > > > > don't think that's a question for the libmount maintainer.
> > > > >
> > > > > Agree.
> > > > > Unlike the comma separated parameters list,
> > > > > upperdir,workdir,lowerdir are overlayfs specific format.
> > > > >
> > > > > ovl_unescape() (for upperdir/workdir) unescapes '\' characters.
> > > > > as does ovl_parse_param_split_lowerdirs().
> > > > > Not sure why this was needed for upperdir/workdir, but it It has
> > > > > been this way for a long time.
> > > > > I see no reason for it to change in the future.
> > > >
> > > > Unescaping  upperdir/workdir was the side effect of using a common
> > > > helper; it wasn't intentional, I think.  The problem is that
> > > > unescaping breaks code that doesn't expect it, and filenames with
> > > > backslashes (and especially \\ or \: sequences) are very rare, so this
> > > > won't show up in testing.
> > > >
> > > > At this point I'm not sure which is more likely to cause bugs: getting
> > > > rid of unescaping or leaving it alone.
> > >
> > > Considering the fact that the applications that mount overlayfs has
> > > always had to do the correct escaping, getting rid of escaping can
> > > only solve issues in new deployments, so I think we should greatly
> > > favor leaving it alone.
> >
> > Any change here is a regression. I'm seriously confused why this is
> > even debated. You already managed to have a regression and I'm still
> > of the opinion that this should be fixed because it literally breaks
> > user space.
> >
>
> You are right. Literally it does.
> But if prospect users are ok with upgrading libmount and if that
> solves the problem, I'd rather not have to carry in the kernel
> baggage code to support old mount API for a very niche use case.
>
> > > >
> > > > One way out of this mess is to create explicit _unesc versions of these options.
> > > >
> > >
> > > I like that solution, with two reservations:
> > > 1. IMO, new _unesc versions should only be supported from new mount API
> > > 2. I only want to do that if real users exists - said users are expected
> > >     to send the patch and explain their use case
> >
> > This is confusing me a lot. Why would you not want to provide an API
> > which is clearly, objectively the better API? As user space, when we
> > can use the new mount API and we could use this, we absolutely would
> > use this.
>
> I am also confused by this reaction.
> Who said that I do not want to provide the _unenc API?
>
> IIUC, you are requesting a new feature that did not exist before,
> namely, upperdir_unenc, workdir_unenc, lowerdir_unenc options.
> Did I understand correctly?
> If that is the case then please send a patch to support
> those new options in the new mount API only
> including documentation and tests.

My entire problem is that you break user space. Either fix the
regression and *continue* fixing regressions instead of hoping that no
one complains enough and escalates things, or give us another API
where you can actually make that guarantee. The current way is simply
not workable.

Even if we'd accept this regression (and thus regress our user space
to not handle any paths any more), the commitment to keeping the API
stable in this thread has been "we'll try" instead of a "yes,
absolutely" and that makes me worry as well.

> Thanks,
> Amir.
>

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

[Amir Goldstein]

On Tue, Oct 10, 2023 at 8:33 PM Sebastian Wick
<sebastian.wick@redhat.com> wrote:
>
> On Tue, Oct 10, 2023 at 6:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Oct 10, 2023 at 7:13 PM Sebastian Wick
> > <sebastian.wick@redhat.com> wrote:
> > >
> > > On Tue, Oct 10, 2023 at 12:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Tue, Oct 10, 2023 at 12:06 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > >
> > > > > On Fri, 6 Oct 2023 at 19:21, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Oct 6, 2023 at 7:42 PM Ryan Hendrickson
> > > > >
> > > > > > > And there is the escaping that needs to happen for ':' and '\' when
> > > > > > > parsing the path parameters (':' is only special syntax in lowerdir, but
> > > > > > > the escaping logic seems to apply to upperdir and workdir as well, based
> > > > > > > on my testing). Even using the new API, this is handled in the kernel.
> > > > > > > We'd like to know if this escaping can be considered stable as well, and I
> > > > > > > don't think that's a question for the libmount maintainer.
> > > > > >
> > > > > > Agree.
> > > > > > Unlike the comma separated parameters list,
> > > > > > upperdir,workdir,lowerdir are overlayfs specific format.
> > > > > >
> > > > > > ovl_unescape() (for upperdir/workdir) unescapes '\' characters.
> > > > > > as does ovl_parse_param_split_lowerdirs().
> > > > > > Not sure why this was needed for upperdir/workdir, but it It has
> > > > > > been this way for a long time.
> > > > > > I see no reason for it to change in the future.
> > > > >
> > > > > Unescaping  upperdir/workdir was the side effect of using a common
> > > > > helper; it wasn't intentional, I think.  The problem is that
> > > > > unescaping breaks code that doesn't expect it, and filenames with
> > > > > backslashes (and especially \\ or \: sequences) are very rare, so this
> > > > > won't show up in testing.
> > > > >
> > > > > At this point I'm not sure which is more likely to cause bugs: getting
> > > > > rid of unescaping or leaving it alone.
> > > >
> > > > Considering the fact that the applications that mount overlayfs has
> > > > always had to do the correct escaping, getting rid of escaping can
> > > > only solve issues in new deployments, so I think we should greatly
> > > > favor leaving it alone.
> > >
> > > Any change here is a regression. I'm seriously confused why this is
> > > even debated. You already managed to have a regression and I'm still
> > > of the opinion that this should be fixed because it literally breaks
> > > user space.
> > >
> >
> > You are right. Literally it does.
> > But if prospect users are ok with upgrading libmount and if that
> > solves the problem, I'd rather not have to carry in the kernel
> > baggage code to support old mount API for a very niche use case.
> >
> > > > >
> > > > > One way out of this mess is to create explicit _unesc versions of these options.
> > > > >
> > > >
> > > > I like that solution, with two reservations:
> > > > 1. IMO, new _unesc versions should only be supported from new mount API
> > > > 2. I only want to do that if real users exists - said users are expected
> > > >     to send the patch and explain their use case
> > >
> > > This is confusing me a lot. Why would you not want to provide an API
> > > which is clearly, objectively the better API? As user space, when we
> > > can use the new mount API and we could use this, we absolutely would
> > > use this.
> >
> > I am also confused by this reaction.
> > Who said that I do not want to provide the _unenc API?
> >
> > IIUC, you are requesting a new feature that did not exist before,
> > namely, upperdir_unenc, workdir_unenc, lowerdir_unenc options.
> > Did I understand correctly?
> > If that is the case then please send a patch to support
> > those new options in the new mount API only
> > including documentation and tests.
>
> My entire problem is that you break user space. Either fix the
> regression and *continue* fixing regressions instead of hoping that no
> one complains enough and escalates things, or give us another API
> where you can actually make that guarantee. The current way is simply
> not workable.

Ok. No need for hostility, I am just trying to figure out what would
be the best solution for everyone going forward.

How about an API that takes upperdirfd, workdirfd, lowerdirfd,
leaving string parsing and path resolution completely out of the equation?

I think this is something that Christian had once suggested.
I think that will be a good improvement for many other use cases as well.

Will that work for you?

>
> Even if we'd accept this regression (and thus regress our user space
> to not handle any paths any more), the commitment to keeping the API
> stable in this thread has been "we'll try" instead of a "yes,
> absolutely" and that makes me worry as well.

It may not be me, it may be someone else, so there is a limit to my
commitment, but kernel developers usually abide by Linus' no regressions
rules (which do allow some slack).

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.

Thanks,
Amir.

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

[Miklos Szeredi]

On Tue, 10 Oct 2023 at 20:15, Amir Goldstein <amir73il@gmail.com> wrote:

> It may not be me, it may be someone else, so there is a limit to my
> commitment, but kernel developers usually abide by Linus' no regressions
> rules (which do allow some slack).

Note: the no regressions rule is about actual "out in the field"
regressions, not about potential or theoretical regressions.

My guess is that changing the escaping rules for workdir and upperdir
would not make any difference.  Look: on my laptop 0.0032% of
filenames contain a backslash.  How likely is such a filename to be
used as workdir or upperdir?  So yes, I think getting rid of
unescaping for these parameters on the new API is safe and will not
invoke the no regressions rule.

The same cannot be said of lowerdir, because the incidence of colons
in filenames is much higher.  But the new API also introduced an
"append mode" for lowerdirs, where the colon doesn't have the same
separator role as with the "bulk mode".   Unfortunately it's not
possible to clearly differentiate the two modes, which I think is a
downside of the current design, and it's why I suggested the _noesc
variants.

>
> 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.

It's not all good: when showing these options, the result is
completely meaningless.   Or is there a plan to make that work nicely?

THanks,
Miklos

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

[Amir Goldstein]

On Tue, Oct 10, 2023 at 9:33 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 10 Oct 2023 at 20:15, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > It may not be me, it may be someone else, so there is a limit to my
> > commitment, but kernel developers usually abide by Linus' no regressions
> > rules (which do allow some slack).
>
> Note: the no regressions rule is about actual "out in the field"
> regressions, not about potential or theoretical regressions.
>
> My guess is that changing the escaping rules for workdir and upperdir
> would not make any difference.  Look: on my laptop 0.0032% of
> filenames contain a backslash.  How likely is such a filename to be
> used as workdir or upperdir?  So yes, I think getting rid of
> unescaping for these parameters on the new API is safe and will not
> invoke the no regressions rule.
>
> The same cannot be said of lowerdir, because the incidence of colons
> in filenames is much higher.  But the new API also introduced an
> "append mode" for lowerdirs, where the colon doesn't have the same
> separator role as with the "bulk mode".   Unfortunately it's not
> possible to clearly differentiate the two modes, which I think is a
> downside of the current design, and it's why I suggested the _noesc
> variants.
>

We could add new keys:
lowerdir_first=,lowerdir_next=,lowerdatadir_next=
as explicit variants for the "[^:]",":","::" prefix detection
and those don't need to be unescaped.

> >
> > 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.
>
> It's not all good: when showing these options, the result is
> completely meaningless.   Or is there a plan to make that work nicely?
>

Currently, the paths to display in mount options are stored
in ofs->config.lowerdirs array.

Those paths could be invalid or point to different objects or not
accessible from the mount namepsace by the time someone
observes them in mount options, so there are not many guarantees
about those displayed paths.

We could use file_path() to resolve path strings of the moment in the
mount namepsace of the mounter from the lowerdirfd files and store
them in ofs->config.lowerdirs array.

I don't think it matters that the displayed mount options are not the
actual user passed mount options as long as the paths are fairly
descriptive and as long as they can be use to mount the same overlay
with the same mount options in the common case (no escaped chars).

BTW, it looks like we also don't display the user passed lowerdir
parameter as is in the case of escaped characters in lowerdirs.
Admittedly, that is another change of behavior from the new mount
api param parsers.

Thanks,
Amir.

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

[Miklos Szeredi]

On Wed, 11 Oct 2023 at 10:45, Amir Goldstein <amir73il@gmail.com> wrote:

>
> We could add new keys:
> lowerdir_first=,lowerdir_next=,lowerdatadir_next=
> as explicit variants for the "[^:]",":","::" prefix detection
> and those don't need to be unescaped.

Good idea.  I'd merge "lowerdir_first" and "lowerdir_next" into
"lowerdir_one" or whatever for simplicity.  I'd also consider dropping
the prefix detection, since it has only been in mainline for one
cycle.

> > >
> > > 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.
> >
> > It's not all good: when showing these options, the result is
> > completely meaningless.   Or is there a plan to make that work nicely?
> >
>
> Currently, the paths to display in mount options are stored
> in ofs->config.lowerdirs array.
>
> Those paths could be invalid or point to different objects or not
> accessible from the mount namepsace by the time someone
> observes them in mount options, so there are not many guarantees
> about those displayed paths.
>
> We could use file_path() to resolve path strings of the moment in the
> mount namepsace of the mounter from the lowerdirfd files and store
> them in ofs->config.lowerdirs array.

Right, so the configuration would use fd's while the display would
fall back to string paths.  That makes sense.

> BTW, it looks like we also don't display the user passed lowerdir
> parameter as is in the case of escaped characters in lowerdirs.
> Admittedly, that is another change of behavior from the new mount
> api param parsers.

And it's a bug (regardless of being a regression or not) since commas
and whitespace  must be escaped on this interface, and colon too for
being a separator of lower layers.

More fun: upperdir and workdir use seq_show_option() which escapes
commas and whitespace, so any escaped characters during mount will end
up being double escaped.

Obviously this domain is severely undertested.

Thanks,
Miklos

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

[Amir Goldstein]

On Wed, Oct 11, 2023 at 1:18 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 11 Oct 2023 at 10:45, Amir Goldstein <amir73il@gmail.com> wrote:
>
> >
> > We could add new keys:
> > lowerdir_first=,lowerdir_next=,lowerdatadir_next=
> > as explicit variants for the "[^:]",":","::" prefix detection
> > and those don't need to be unescaped.
>
> Good idea.  I'd merge "lowerdir_first" and "lowerdir_next" into
> "lowerdir_one" or whatever for simplicity.  I'd also consider dropping
> the prefix detection, since it has only been in mainline for one
> cycle.
>

OK.

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?

> > > >
> > > > 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.
> > >
> > > It's not all good: when showing these options, the result is
> > > completely meaningless.   Or is there a plan to make that work nicely?
> > >
> >
> > Currently, the paths to display in mount options are stored
> > in ofs->config.lowerdirs array.
> >
> > Those paths could be invalid or point to different objects or not
> > accessible from the mount namepsace by the time someone
> > observes them in mount options, so there are not many guarantees
> > about those displayed paths.
> >
> > We could use file_path() to resolve path strings of the moment in the
> > mount namepsace of the mounter from the lowerdirfd files and store
> > them in ofs->config.lowerdirs array.
>
> Right, so the configuration would use fd's while the display would
> fall back to string paths.  That makes sense.
>

Assuming that using fds is a desired feature regardless, then
possibly the _noesc options are unneeded. Not sure.

> > BTW, it looks like we also don't display the user passed lowerdir
> > parameter as is in the case of escaped characters in lowerdirs.
> > Admittedly, that is another change of behavior from the new mount
> > api param parsers.
>
> And it's a bug (regardless of being a regression or not) since commas
> and whitespace  must be escaped on this interface, and colon too for
> being a separator of lower layers.

OK. I think it should be easy to fix this bug.
I can look into it.

>
> More fun: upperdir and workdir use seq_show_option() which escapes
> commas and whitespace, so any escaped characters during mount will end
> up being double escaped.
>
> Obviously this domain is severely undertested.

This is all very complicated because actual users always
go through escaping rules of bash and libmount.

For example, the output of 'mount' command unescapes the
escaping done by seq_show_option() for /proc/mounts.

That's why it is scary to change the legacy behavior and better
to provide the new unescaped options as you suggested
and leave all the escaping in the future to userspace.

Thanks,
Amir.

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

[Miklos Szeredi]

On Wed, 11 Oct 2023 at 14:07, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Oct 11, 2023 at 1:18 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Wed, 11 Oct 2023 at 10:45, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > >
> > > We could add new keys:
> > > lowerdir_first=,lowerdir_next=,lowerdatadir_next=
> > > as explicit variants for the "[^:]",":","::" prefix detection
> > > and those don't need to be unescaped.
> >
> > Good idea.  I'd merge "lowerdir_first" and "lowerdir_next" into
> > "lowerdir_one" or whatever for simplicity.  I'd also consider dropping
> > the prefix detection, since it has only been in mainline for one
> > cycle.
> >
>
> OK.
>
> 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.

> > > > >
> > > > > 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()?

> > > BTW, it looks like we also don't display the user passed lowerdir
> > > parameter as is in the case of escaped characters in lowerdirs.
> > > Admittedly, that is another change of behavior from the new mount
> > > api param parsers.
> >
> > And it's a bug (regardless of being a regression or not) since commas
> > and whitespace  must be escaped on this interface, and colon too for
> > being a separator of lower layers.
>
> OK. I think it should be easy to fix this bug.
> I can look into it.

Thanks.

> > More fun: upperdir and workdir use seq_show_option() which escapes
> > commas and whitespace, so any escaped characters during mount will end
> > up being double escaped.
> >
> > Obviously this domain is severely undertested.
>
> This is all very complicated because actual users always
> go through escaping rules of bash and libmount.
>
> For example, the output of 'mount' command unescapes the
> escaping done by seq_show_option() for /proc/mounts.
>
> That's why it is scary to change the legacy behavior and better
> to provide the new unescaped options as you suggested
> and leave all the escaping in the future to userspace.

And add a new api for retrieving fs specific mount options.  That
should be the next part after statmount(2) is done.

Thanks,
Miklos

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

[Miklos Szeredi]

On Wed, 11 Oct 2023 at 15:07, Miklos Szeredi <miklos@szeredi.hu> wrote:

> > > > > > 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()?

Looked and there's nothing in the uAPI that would prevent overloading
the existing names.  However the internal fs_parse() API only allows a
single type associated with a name.   That could be changed if we
want.   I think it would make more sense than adding newer names that
are not in sync with the command line usage and existing
documentation.   I can look into that if there's no objection.

Thanks,
Miklos

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

[Amir Goldstein]

On Wed, Oct 11, 2023 at 5:33 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 11 Oct 2023 at 15:07, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> > > > > > > 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()?
>
> Looked and there's nothing in the uAPI that would prevent overloading
> the existing names.  However the internal fs_parse() API only allows a
> single type associated with a name.   That could be changed if we
> want.   I think it would make more sense than adding newer names that
> are not in sync with the command line usage and existing
> documentation.   I can look into that if there's no objection.
>

Works for me.

FYI, I pushed the lowerdir escaping regression fix to ovl-fixes.

Thanks,
Amir.

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

[Amir Goldstein]

On Wed, Oct 11, 2023 at 4:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 11 Oct 2023 at 14:07, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, Oct 11, 2023 at 1:18 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Wed, 11 Oct 2023 at 10:45, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > >
> > > > We could add new keys:
> > > > lowerdir_first=,lowerdir_next=,lowerdatadir_next=
> > > > as explicit variants for the "[^:]",":","::" prefix detection
> > > > and those don't need to be unescaped.
> > >
> > > Good idea.  I'd merge "lowerdir_first" and "lowerdir_next" into
> > > "lowerdir_one" or whatever for simplicity.  I'd also consider dropping
> > > the prefix detection, since it has only been in mainline for one
> > > cycle.
> > >
> >
> > OK.
> >
> > 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.
>

FWIW, I see that Christian has also implemented reset of lowerdir
stack with an empty string.

> > > > > >
> > > > > > 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()?
>
> > > > BTW, it looks like we also don't display the user passed lowerdir
> > > > parameter as is in the case of escaped characters in lowerdirs.
> > > > Admittedly, that is another change of behavior from the new mount
> > > > api param parsers.
> > >
> > > And it's a bug (regardless of being a regression or not) since commas
> > > and whitespace  must be escaped on this interface, and colon too for
> > > being a separator of lower layers.
> >
> > OK. I think it should be easy to fix this bug.
> > I can look into it.
>
> Thanks.
>
> > > More fun: upperdir and workdir use seq_show_option() which escapes
> > > commas and whitespace, so any escaped characters during mount will end
> > > up being double escaped.
> > >

FYI, I tried to reproduce this bug, but could not.
The reason (I think) is that the escaping provided by seq_show_option()
is not the dumb "prepend \", but it's the "convert to \octal".

It turns out that I also needed to store the lowerdir before unescaping,
so that a path like 'lower\:2' will be presented correctly (with \) in mount
options.

Thanks,
Amir.

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

[Christian Brauner]

> > 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().

> 
> > > > > >
> > > > > > 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.

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

[Amir Goldstein]

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.

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).

Thoughts?

Thanks,
Amir.

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

[Christian Brauner]

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.

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

[Amir Goldstein]

On Thu, Oct 12, 2023 at 12:49 PM Christian Brauner <brauner@kernel.org> wrote:
>
> 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...
>

OK. it was quite simple so I posted the regression fix.

> >
> > 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.

OK. Instead of changing generic_parse_monolithic() I re-factored it:

https://lore.kernel.org/linux-unionfs/20231012134428.1874373-1-amir73il@gmail.com/

This may end up being useful for other fs.
If you agree with the new helper, please ACK and I will send it for 6.6-rc6
along with the two fixes I have on ovl-fixes.

Also updated the xfstest to test escaped commas:

https://github.com/amir73il/xfstests/commits/overlayfs-devel

Thanks,
Amir.

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

[Sebastian Wick]

Sorry if my previous mail sounded hostile. I was just really confused
why a regression wasn't taken seriously.

Thank you very much for fixing it!

On Thu, Oct 12, 2023 at 3:55 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Oct 12, 2023 at 12:49 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > 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...
> >
>
> OK. it was quite simple so I posted the regression fix.
>
> > >
> > > 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.
>
> OK. Instead of changing generic_parse_monolithic() I re-factored it:
>
> https://lore.kernel.org/linux-unionfs/20231012134428.1874373-1-amir73il@gmail.com/
>
> This may end up being useful for other fs.
> If you agree with the new helper, please ACK and I will send it for 6.6-rc6
> along with the two fixes I have on ovl-fixes.
>
> Also updated the xfstest to test escaped commas:
>
> https://github.com/amir73il/xfstests/commits/overlayfs-devel
>
> Thanks,
> Amir.
>

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

[Christian Brauner]

On Wed, Oct 11, 2023 at 03:06:49PM +0300, Amir Goldstein wrote:
> On Wed, Oct 11, 2023 at 1:18 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Wed, 11 Oct 2023 at 10:45, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > >
> > > We could add new keys:
> > > lowerdir_first=,lowerdir_next=,lowerdatadir_next=
> > > as explicit variants for the "[^:]",":","::" prefix detection
> > > and those don't need to be unescaped.
> >
> > Good idea.  I'd merge "lowerdir_first" and "lowerdir_next" into
> > "lowerdir_one" or whatever for simplicity.  I'd also consider dropping
> > the prefix detection, since it has only been in mainline for one
> > cycle.
> >
> 
> OK.
> 
> Christian,
> 
> Do you know any userspace that already uses your new append prefixes?

I'm not sure if @Karel already ported libmount to rely on this.
The new layer parsing where you can append and replace layers was done
so that we can have the maximum number of allowed lower layers.

So in the future I would expect libmount should parse the lowerdir,
upperdir, workdir strings and split them up in userspace before passing
them to the kernel.

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

[Linux regression tracking (Thorsten Leemhuis)]

[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

On 29.09.23 03:07, Ryan Hendrickson wrote:
> Up to and including kernel 6.4.15, it was possible to have commas in the
> lowerdir/upperdir/workdir paths used by overlayfs, provided they were
> escaped with backslashes:
> 
>     mkdir /tmp/test-lower, /tmp/test-upper /tmp/test-work /tmp/test
>     mount -t overlay overlay -o
> 'lowerdir=/tmp/test-lower\,,upperdir=/tmp/test-upper,workdir=/tmp/test-work' /tmp/test
> 
> In 6.5.2 and 6.5.5, this no longer works; dmesg reports that overlayfs
> can't resolve '/tmp/test-lower' (without the comma).
> 
> I see that there is a commit between the 6.4 and 6.5 lines titled [ovl:
> port to new mount api][1]. I haven't compiled a kernel before and after
> this commit to verify, but based on the code it deletes I strongly
> suspect that it, or if not then one of the ovl commits committed on the
> same day, is responsible for this change.
> 
> [1]:
> https://github.com/torvalds/linux/commit/1784fbc2ed9c888ea4e895f30a53207ed7ee8208
> 
> Does this count as a regression? I can't find documentation for this
> escaping feature anywhere, even as it pertains to the non-comma
> characters '\\' and ':' (which, I've tested, can still be escaped as
> expected), so perhaps it was never properly supported? But a search for
> escaping commas in overlayfs turns up resources like [this post][2],
> suggesting that there are others who figured this out and expect it to
> work.
> 
> [2]: https://unix.stackexchange.com/a/552640
> 
> Is there a new way to escape commas for overlayfs options?

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced v6.4..v6.5.2
#regzbot title ovl: commas in overlayfs mount options broke
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.