On Mon, Oct 16, 2023 at 03:10:33PM +0200, Miklos Szeredi wrote:
> On Mon, 16 Oct 2023 at 13:56, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > On Mon, Oct 16, 2023 at 12:27 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> > >
> > > On Sun, 15 Oct 2023 at 08:58, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > >
> > > > + for (nr = 0; nr < nr_added_lower; nr++, lowerdirs++) {
> > > > + if (nr < nr_merged_lower)
> > > > + seq_show_option(m, "lowerdir+", *lowerdirs);
> > > > + else
> > > > + seq_show_option(m, "datadir+", *lowerdirs);
> > >
> > > Good.
> > >
> > > I did some testing and it turns out libmount still regresses on
> > > 6.6-rc6 for the escaped comma case. The reason is that libmount
> > > doesn't understand escaping of commas, hence the '-oupper=upper\,1'
> > > will result in two fsconfig() calls: 'upper=upper\' and '1'. Prior
> > > to 6.5 these were nicely reconstructed into the original
> > > 'upper=upper\,1' by legacy_parse_param().
Yes, libmount does not interpret '\,' in any way, it's just comma
after a char :-)
> >
> > Technically, I think this is a libmount regression, not a kernel regression.
> > Since libmount 2.39, libmount will split the commas differently than
> > overlayfs always did.
The difference is that old libmount versions do not split the string;
it only removes well-known non-kernel stuff and flags from the string.
However, the rest of the string remains unmodified. This means that
"upper=aaa,bbb" comprises two options for the old libmount, but
because it neither reorders nor splits it, it is sent unmodified to
the mount(2) syscall.
The new libmount works with mount options differently. It keeps them
parsed in memory (in a struct libmnt_optlist), and this list is used
for fsconfig().
> Ah, but it's not a regression after all, since the kernel un-split the
> same commas until 6.5, so there was no way the libmount devs would
> have observed any regression in overlayfs mount. But arguing about
> which component is the cause of the regression is not very productive.
> Indeed libmount can be fixed parse overlayfs options the same way as
> the kernel parsed them before 6.5, which is probably a much better
> fix, than a kernel one.
>
> Karel, is doing such filesystem specific option handling feasible?
For decade we have support for commas in mount option due to
creativity of SELinux developers:
foo,context="aaa,bbb,ccc",bar
is valid mount options string and libmount will ignore commas within
" " and split it to foo, bar, and context=.
The current util-linux git (and old 6.2 kernel):
# strace -e fsopen,fsconfig ./mount -t overlay overlay -o 'lowerdir="/tmp/test-lower,",upperdir=/tmp/test-upper,workdir=/tmp/test-work' /tmp/test
fsopen("overlay", FSOPEN_CLOEXEC) = 3
fsconfig(3, FSCONFIG_SET_STRING, "source", "overlay", 0) = 0
fsconfig(3, FSCONFIG_SET_STRING, "lowerdir", "/tmp/test-lower,", 0) = -1 EINVAL (Invalid argument)
You can see "/tmp/test-lower,".
Maybe all we need is to improve mount(8) docs to force people use ""
for paths when used in mount options.
Anyway, I think we can improve libmount to ignore \, as non-separator.
The question is what the rest of the userspace universe, because fstab
is interpreted on many places ...
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
