From: Sebastian Wick <sebastian.wick@redhat.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Christian Brauner <brauner@kernel.org>,
Miklos Szeredi <miklos@szeredi.hu>,
Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>,
linux-unionfs@vger.kernel.org, Karel Zak <kzak@redhat.com>
Subject: Re: [regression?] escaping commas in overlayfs mount options
Date: Thu, 12 Oct 2023 20:07:13 +0200 [thread overview]
Message-ID: <CA+hFU4xwgqeVwyQPnKSfX76xvNtgwzHHeDjfc1+4=5B8qskiWg@mail.gmail.com> (raw)
In-Reply-To: <CAOQ4uxi4dLuJeYZ7p-AGRq543ijXXL0p=V-MdKshW5gQHaAWBw@mail.gmail.com>
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.
>
next prev parent reply other threads:[~2023-10-12 18:08 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2023-10-12 8:21 ` Christian Brauner
2023-09-29 5:07 ` Linux regression tracking (Thorsten Leemhuis)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+hFU4xwgqeVwyQPnKSfX76xvNtgwzHHeDjfc1+4=5B8qskiWg@mail.gmail.com' \
--to=sebastian.wick@redhat.com \
--cc=amir73il@gmail.com \
--cc=brauner@kernel.org \
--cc=kzak@redhat.com \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=ryan.hendrickson@alum.mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).