On Tue, Oct 17, 2023 at 1:11 PM Karel Zak <kzak@xxxxxxxxxx> wrote:
>
> 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.
>
Even if people would read documentation ;-)
I don't think that would have helped in this case,
because with old libmount and old kernel, overlayfs does not parse
lowerdir="/tmp/test-lower," correctly - it requires lowerdir="/tmp/test-lower\,"
> 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 ...
The other option is to require overlayfs users to opt-in to new mount api
(LIBMOUNT_FORCE_MOUNT2=always) and otherwise default to
the old mount api or
instead of improving libmount parsing of \,
maybe just detect it and default to the old mount api
just for overlayfs or generally?
All-in-all, those cases are probably rare, so I think the minimal
solution with as little room for complication should be applied.
Detecting \, and defaulting to old mount api seems to meet this criteria?
BTW, smb3_fs_context_parse_monolithic() also has special
handling of comma parsing in kernel (,, is a non-separator comma),
so smb3 (cifs) may also require special treatment when choosing
old/new mount api.
Thanks,
Amir.
