* [regression?] escaping commas in overlayfs mount options @ 2023-09-29 1:07 Ryan Hendrickson 2023-09-29 4:44 ` Amir Goldstein 2023-09-29 5:07 ` Linux regression tracking (Thorsten Leemhuis) 0 siblings, 2 replies; 31+ messages in thread From: Ryan Hendrickson @ 2023-09-29 1:07 UTC (permalink / raw) To: Miklos Szeredi, Amir Goldstein, linux-unionfs 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-09-29 1:07 [regression?] escaping commas in overlayfs mount options Ryan Hendrickson @ 2023-09-29 4:44 ` Amir Goldstein 2023-10-02 22:22 ` Ryan Hendrickson 2023-10-06 13:02 ` Sebastian Wick 2023-09-29 5:07 ` Linux regression tracking (Thorsten Leemhuis) 1 sibling, 2 replies; 31+ messages in thread From: Amir Goldstein @ 2023-09-29 4:44 UTC (permalink / raw) To: Ryan Hendrickson, Christian Brauner; +Cc: Miklos Szeredi, linux-unionfs 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-09-29 4:44 ` Amir Goldstein @ 2023-10-02 22:22 ` Ryan Hendrickson 2023-10-03 9:50 ` Amir Goldstein 2023-10-06 13:02 ` Sebastian Wick 1 sibling, 1 reply; 31+ messages in thread From: Ryan Hendrickson @ 2023-10-02 22:22 UTC (permalink / raw) To: Amir Goldstein; +Cc: Christian Brauner, Miklos Szeredi, linux-unionfs 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-02 22:22 ` Ryan Hendrickson @ 2023-10-03 9:50 ` Amir Goldstein 2023-10-03 19:07 ` Ryan Hendrickson 0 siblings, 1 reply; 31+ messages in thread From: Amir Goldstein @ 2023-10-03 9:50 UTC (permalink / raw) To: Ryan Hendrickson Cc: Christian Brauner, Miklos Szeredi, linux-unionfs, Karel Zak 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-03 9:50 ` Amir Goldstein @ 2023-10-03 19:07 ` Ryan Hendrickson 2023-10-04 9:03 ` Amir Goldstein 0 siblings, 1 reply; 31+ messages in thread From: Ryan Hendrickson @ 2023-10-03 19:07 UTC (permalink / raw) To: Amir Goldstein Cc: Christian Brauner, Miklos Szeredi, linux-unionfs, Karel Zak 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-03 19:07 ` Ryan Hendrickson @ 2023-10-04 9:03 ` Amir Goldstein 0 siblings, 0 replies; 31+ messages in thread From: Amir Goldstein @ 2023-10-04 9:03 UTC (permalink / raw) To: Ryan Hendrickson Cc: Christian Brauner, Miklos Szeredi, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-09-29 4:44 ` Amir Goldstein 2023-10-02 22:22 ` Ryan Hendrickson @ 2023-10-06 13:02 ` Sebastian Wick 2023-10-06 16:17 ` Amir Goldstein 1 sibling, 1 reply; 31+ messages in thread From: Sebastian Wick @ 2023-10-06 13:02 UTC (permalink / raw) To: Amir Goldstein Cc: Ryan Hendrickson, Christian Brauner, Miklos Szeredi, linux-unionfs 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-06 13:02 ` Sebastian Wick @ 2023-10-06 16:17 ` Amir Goldstein 2023-10-06 16:42 ` Ryan Hendrickson 0 siblings, 1 reply; 31+ messages in thread From: Amir Goldstein @ 2023-10-06 16:17 UTC (permalink / raw) To: Sebastian Wick Cc: Ryan Hendrickson, Christian Brauner, Miklos Szeredi, linux-unionfs, Karel Zak 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. > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-06 16:17 ` Amir Goldstein @ 2023-10-06 16:42 ` Ryan Hendrickson 2023-10-06 17:21 ` Amir Goldstein 0 siblings, 1 reply; 31+ messages in thread From: Ryan Hendrickson @ 2023-10-06 16:42 UTC (permalink / raw) To: Amir Goldstein Cc: Sebastian Wick, Christian Brauner, Miklos Szeredi, linux-unionfs, Karel Zak [-- 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-06 16:42 ` Ryan Hendrickson @ 2023-10-06 17:21 ` Amir Goldstein 2023-10-10 9:06 ` Miklos Szeredi 0 siblings, 1 reply; 31+ messages in thread From: Amir Goldstein @ 2023-10-06 17:21 UTC (permalink / raw) To: Ryan Hendrickson Cc: Sebastian Wick, Christian Brauner, Miklos Szeredi, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-06 17:21 ` Amir Goldstein @ 2023-10-10 9:06 ` Miklos Szeredi 2023-10-10 10:00 ` Amir Goldstein 0 siblings, 1 reply; 31+ messages in thread From: Miklos Szeredi @ 2023-10-10 9:06 UTC (permalink / raw) To: Amir Goldstein Cc: Ryan Hendrickson, Sebastian Wick, Christian Brauner, linux-unionfs, Karel Zak 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-10 9:06 ` Miklos Szeredi @ 2023-10-10 10:00 ` Amir Goldstein 2023-10-10 16:13 ` Sebastian Wick 0 siblings, 1 reply; 31+ messages in thread From: Amir Goldstein @ 2023-10-10 10:00 UTC (permalink / raw) To: Miklos Szeredi Cc: Ryan Hendrickson, Sebastian Wick, Christian Brauner, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-10 10:00 ` Amir Goldstein @ 2023-10-10 16:13 ` Sebastian Wick 2023-10-10 16:54 ` Amir Goldstein 0 siblings, 1 reply; 31+ messages in thread From: Sebastian Wick @ 2023-10-10 16:13 UTC (permalink / raw) To: Amir Goldstein Cc: Miklos Szeredi, Ryan Hendrickson, Christian Brauner, linux-unionfs, Karel Zak 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. > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-10 16:13 ` Sebastian Wick @ 2023-10-10 16:54 ` Amir Goldstein 2023-10-10 17:33 ` Sebastian Wick 0 siblings, 1 reply; 31+ messages in thread From: Amir Goldstein @ 2023-10-10 16:54 UTC (permalink / raw) To: Sebastian Wick Cc: Miklos Szeredi, Ryan Hendrickson, Christian Brauner, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-10 16:54 ` Amir Goldstein @ 2023-10-10 17:33 ` Sebastian Wick 2023-10-10 18:15 ` Amir Goldstein 0 siblings, 1 reply; 31+ messages in thread From: Sebastian Wick @ 2023-10-10 17:33 UTC (permalink / raw) To: Amir Goldstein Cc: Miklos Szeredi, Ryan Hendrickson, Christian Brauner, linux-unionfs, Karel Zak 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. > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-10 17:33 ` Sebastian Wick @ 2023-10-10 18:15 ` Amir Goldstein 2023-10-10 18:33 ` Miklos Szeredi 0 siblings, 1 reply; 31+ messages in thread From: Amir Goldstein @ 2023-10-10 18:15 UTC (permalink / raw) To: Sebastian Wick Cc: Miklos Szeredi, Ryan Hendrickson, Christian Brauner, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-10 18:15 ` Amir Goldstein @ 2023-10-10 18:33 ` Miklos Szeredi 2023-10-11 8:44 ` Amir Goldstein 0 siblings, 1 reply; 31+ messages in thread From: Miklos Szeredi @ 2023-10-10 18:33 UTC (permalink / raw) To: Amir Goldstein Cc: Sebastian Wick, Ryan Hendrickson, Christian Brauner, linux-unionfs, Karel Zak 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-10 18:33 ` Miklos Szeredi @ 2023-10-11 8:44 ` Amir Goldstein 2023-10-11 10:18 ` Miklos Szeredi 0 siblings, 1 reply; 31+ messages in thread From: Amir Goldstein @ 2023-10-11 8:44 UTC (permalink / raw) To: Miklos Szeredi Cc: Sebastian Wick, Ryan Hendrickson, Christian Brauner, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-11 8:44 ` Amir Goldstein @ 2023-10-11 10:18 ` Miklos Szeredi 2023-10-11 12:06 ` Amir Goldstein 0 siblings, 1 reply; 31+ messages in thread From: Miklos Szeredi @ 2023-10-11 10:18 UTC (permalink / raw) To: Amir Goldstein Cc: Sebastian Wick, Ryan Hendrickson, Christian Brauner, linux-unionfs, Karel Zak 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-11 10:18 ` Miklos Szeredi @ 2023-10-11 12:06 ` Amir Goldstein 2023-10-11 13:07 ` Miklos Szeredi 2023-10-12 8:21 ` Christian Brauner 0 siblings, 2 replies; 31+ messages in thread From: Amir Goldstein @ 2023-10-11 12:06 UTC (permalink / raw) To: Miklos Szeredi Cc: Sebastian Wick, Ryan Hendrickson, Christian Brauner, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-11 12:06 ` Amir Goldstein @ 2023-10-11 13:07 ` Miklos Szeredi 2023-10-11 14:33 ` Miklos Szeredi ` (2 more replies) 2023-10-12 8:21 ` Christian Brauner 1 sibling, 3 replies; 31+ messages in thread From: Miklos Szeredi @ 2023-10-11 13:07 UTC (permalink / raw) To: Amir Goldstein Cc: Sebastian Wick, Ryan Hendrickson, Christian Brauner, linux-unionfs, Karel Zak 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-11 13:07 ` Miklos Szeredi @ 2023-10-11 14:33 ` Miklos Szeredi 2023-10-11 16:37 ` Amir Goldstein 2023-10-11 16:43 ` Amir Goldstein 2023-10-12 8:26 ` Christian Brauner 2 siblings, 1 reply; 31+ messages in thread From: Miklos Szeredi @ 2023-10-11 14:33 UTC (permalink / raw) To: Amir Goldstein Cc: Sebastian Wick, Ryan Hendrickson, Christian Brauner, linux-unionfs, Karel Zak 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-11 14:33 ` Miklos Szeredi @ 2023-10-11 16:37 ` Amir Goldstein 0 siblings, 0 replies; 31+ messages in thread From: Amir Goldstein @ 2023-10-11 16:37 UTC (permalink / raw) To: Miklos Szeredi Cc: Sebastian Wick, Ryan Hendrickson, Christian Brauner, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-11 13:07 ` Miklos Szeredi 2023-10-11 14:33 ` Miklos Szeredi @ 2023-10-11 16:43 ` Amir Goldstein 2023-10-12 8:26 ` Christian Brauner 2 siblings, 0 replies; 31+ messages in thread From: Amir Goldstein @ 2023-10-11 16:43 UTC (permalink / raw) To: Miklos Szeredi Cc: Sebastian Wick, Ryan Hendrickson, Christian Brauner, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-11 13:07 ` Miklos Szeredi 2023-10-11 14:33 ` Miklos Szeredi 2023-10-11 16:43 ` Amir Goldstein @ 2023-10-12 8:26 ` Christian Brauner 2023-10-12 9:27 ` Amir Goldstein 2 siblings, 1 reply; 31+ messages in thread From: Christian Brauner @ 2023-10-12 8:26 UTC (permalink / raw) To: Miklos Szeredi Cc: Amir Goldstein, Sebastian Wick, Ryan Hendrickson, linux-unionfs, Karel Zak > > 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-12 8:26 ` Christian Brauner @ 2023-10-12 9:27 ` Amir Goldstein 2023-10-12 9:49 ` Christian Brauner 0 siblings, 1 reply; 31+ messages in thread From: Amir Goldstein @ 2023-10-12 9:27 UTC (permalink / raw) To: Christian Brauner Cc: Miklos Szeredi, Sebastian Wick, Ryan Hendrickson, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-12 9:27 ` Amir Goldstein @ 2023-10-12 9:49 ` Christian Brauner 2023-10-12 13:54 ` Amir Goldstein 0 siblings, 1 reply; 31+ messages in thread From: Christian Brauner @ 2023-10-12 9:49 UTC (permalink / raw) To: Amir Goldstein Cc: Miklos Szeredi, Sebastian Wick, Ryan Hendrickson, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-12 9:49 ` Christian Brauner @ 2023-10-12 13:54 ` Amir Goldstein 2023-10-12 18:07 ` Sebastian Wick 0 siblings, 1 reply; 31+ messages in thread From: Amir Goldstein @ 2023-10-12 13:54 UTC (permalink / raw) To: Christian Brauner Cc: Miklos Szeredi, Sebastian Wick, Ryan Hendrickson, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-12 13:54 ` Amir Goldstein @ 2023-10-12 18:07 ` Sebastian Wick 0 siblings, 0 replies; 31+ messages in thread From: Sebastian Wick @ 2023-10-12 18:07 UTC (permalink / raw) To: Amir Goldstein Cc: Christian Brauner, Miklos Szeredi, Ryan Hendrickson, linux-unionfs, Karel Zak 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. > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-10-11 12:06 ` Amir Goldstein 2023-10-11 13:07 ` Miklos Szeredi @ 2023-10-12 8:21 ` Christian Brauner 1 sibling, 0 replies; 31+ messages in thread From: Christian Brauner @ 2023-10-12 8:21 UTC (permalink / raw) To: Amir Goldstein Cc: Miklos Szeredi, Sebastian Wick, Ryan Hendrickson, linux-unionfs, Karel Zak 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [regression?] escaping commas in overlayfs mount options 2023-09-29 1:07 [regression?] escaping commas in overlayfs mount options Ryan Hendrickson 2023-09-29 4:44 ` Amir Goldstein @ 2023-09-29 5:07 ` Linux regression tracking (Thorsten Leemhuis) 1 sibling, 0 replies; 31+ messages in thread From: Linux regression tracking (Thorsten Leemhuis) @ 2023-09-29 5:07 UTC (permalink / raw) To: Ryan Hendrickson, Miklos Szeredi, Amir Goldstein, linux-unionfs, Linux kernel regressions list [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. ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2023-10-12 18:08 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-09-29 1:07 [regression?] escaping commas in overlayfs mount options Ryan Hendrickson 2023-09-29 4:44 ` Amir Goldstein 2023-10-02 22:22 ` Ryan Hendrickson 2023-10-03 9:50 ` Amir Goldstein 2023-10-03 19:07 ` Ryan Hendrickson 2023-10-04 9:03 ` Amir Goldstein 2023-10-06 13:02 ` Sebastian Wick 2023-10-06 16:17 ` Amir Goldstein 2023-10-06 16:42 ` Ryan Hendrickson 2023-10-06 17:21 ` Amir Goldstein 2023-10-10 9:06 ` Miklos Szeredi 2023-10-10 10:00 ` Amir Goldstein 2023-10-10 16:13 ` Sebastian Wick 2023-10-10 16:54 ` Amir Goldstein 2023-10-10 17:33 ` Sebastian Wick 2023-10-10 18:15 ` Amir Goldstein 2023-10-10 18:33 ` Miklos Szeredi 2023-10-11 8:44 ` Amir Goldstein 2023-10-11 10:18 ` Miklos Szeredi 2023-10-11 12:06 ` Amir Goldstein 2023-10-11 13:07 ` Miklos Szeredi 2023-10-11 14:33 ` Miklos Szeredi 2023-10-11 16:37 ` Amir Goldstein 2023-10-11 16:43 ` Amir Goldstein 2023-10-12 8:26 ` Christian Brauner 2023-10-12 9:27 ` Amir Goldstein 2023-10-12 9:49 ` Christian Brauner 2023-10-12 13:54 ` Amir Goldstein 2023-10-12 18:07 ` Sebastian Wick 2023-10-12 8:21 ` Christian Brauner 2023-09-29 5:07 ` Linux regression tracking (Thorsten Leemhuis)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).