On Tue, Oct 24, 2023 at 09:48:42AM +0300, Amir Goldstein wrote:
> On Tue, Oct 24, 2023 at 8:24 AM Zorro Lang <zlang@xxxxxxxxxx> wrote:
> >
> > On Mon, Oct 23, 2023 at 07:32:59PM +0300, Amir Goldstein wrote:
> > > Check parsing and display of spaces and escaped colons and commans in
> > > lowerdir mount option.
> > >
> > > This is a regression test for two bugs introduced in v6.5 with the
> > > conversion to new mount api.
> > >
> > > There is another regression of new mount api related to libmount parsing
> > > of escaped commas, but this needs a fix in libmount - this test only
> > > verifies the fixes in the kernel, so it uses LIBMOUNT_FORCE_MOUNT2=always
> > > to force mount(2) and kernel pasring of the comma separated options list.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > ---
> > >
> > > Changes since v2:
> > > - Fix test for when index feature is enabled
> > >
> > > Changes since v1:
> > > - Fix test for libmount >= 2.39
> > >
> > > tests/overlay/083 | 76 +++++++++++++++++++++++++++++++++++++++++++
> > > tests/overlay/083.out | 2 ++
> > > 2 files changed, 78 insertions(+)
> > > create mode 100755 tests/overlay/083
> > > create mode 100644 tests/overlay/083.out
> > >
> > > diff --git a/tests/overlay/083 b/tests/overlay/083
> > > new file mode 100755
> > > index 00000000..df82d1fd
> > > --- /dev/null
> > > +++ b/tests/overlay/083
> > > @@ -0,0 +1,76 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (C) 2023 CTERA Networks. All Rights Reserved.
> > > +#
> > > +# FS QA Test 083
> > > +#
> > > +# Test regressions in parsing and display of special chars in mount options.
> > > +#
> > > +# The following kernel commits from v6.5 introduced regressions:
> > > +# b36a5780cb44 ("ovl: modify layer parameter parsing")
> > > +# 1784fbc2ed9c ("ovl: port to new mount api")
> > > +#
> > > +
> > > +. ./common/preamble
> > > +_begin_fstest auto quick mount
> > > +
> > > +# Import common functions.
> > > +. ./common/filter
> > > +
> > > +# real QA test starts here
> > > +_supported_fs overlay
> > > +_fixed_by_kernel_commit 32db51070850 \
> > > + "ovl: fix regression in showing lowerdir mount option"
> > > +_fixed_by_kernel_commit c34706acf40b \
> > > + "ovl: fix regression in parsing of mount options with escaped comma"
> > > +
> > > +# _overlay_check_* helpers do not handle special chars well
> > > +_require_scratch_nocheck
> > > +
> > > +# Remove all files from previous tests
> > > +_scratch_mkfs
> > > +
> > > +# Create lowerdirs with special characters
> > > +lowerdir_spaces="$OVL_BASE_SCRATCH_MNT/lower1 with spaces"
> > > +lowerdir_colons="$OVL_BASE_SCRATCH_MNT/lower2:with::colons"
> > > +lowerdir_commas="$OVL_BASE_SCRATCH_MNT/lower3,with,,commas"
> > > +lowerdir_colons_esc="$OVL_BASE_SCRATCH_MNT/lower2\:with\:\:colons"
> > > +lowerdir_commas_esc="$OVL_BASE_SCRATCH_MNT/lower3\,with\,\,commas"
> > > +upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
> > > +workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
> > > +mkdir -p "$lowerdir_spaces" "$lowerdir_colons" "$lowerdir_commas"
> > > +
> > > +# _overlay_mount_* helpers do not handle special chars well, so execute mount directly.
> > > +# if escaped colons are not parsed correctly, mount will fail.
> > > +$MOUNT_PROG -t overlay ovl_esc_test $SCRATCH_MNT \
> > > + -o"upperdir=$upperdir,workdir=$workdir" \
> > > + -o"lowerdir=$lowerdir_colons_esc:$lowerdir_spaces" \
> > > + 2>&1 | tee -a $seqres.full
> > > +
> > > +# if spaces are not escaped when showing mount options,
> > > +# mount command will not show the word 'spaces' after the spaces
> > > +$MOUNT_PROG -t overlay | grep ovl_esc_test | tee -a $seqres.full | grep -v spaces && \
> > > + echo "ERROR: escaped spaces truncated from lowerdir mount option"
> > > +
> > > +# Re-create the upper/work dirs to mount them with a different lower
> > > +# This is required in case index feature is enabled
> > > +$UMOUNT_PROG $SCRATCH_MNT
> > > +rm -rf "$upperdir" "$workdir"
> > > +mkdir -p "$upperdir" "$workdir"
> > > +
> > > +# Kernel commit c34706acf40b fixes parsing of mount options with escaped comma
> > > +# when the mount options string is provided via data argument to mount(2) syscall.
> > > +# With libmount >= 2.39, libmount itself will try to split the comma separated
> > > +# options list provided to mount(8) commnad line and call fsconfig(2) for each
> > > +# mount option seperately. Since libmount does not obay to overlayfs escaped
> > > +# commas format, it will call fsconfig(2) with the wrong path (i.e. ".../lower3")
> > > +# and this test will fail, but the failure would indicate a libmount issue, not
> > > +# a kernel issue. Therefore, force libmount to use mount(2) syscall, so we only
> > > +# test the kernel fix.
> > > +LIBMOUNT_FORCE_MOUNT2=always $MOUNT_PROG -t overlay $OVL_BASE_SCRATCH_DEV $SCRATCH_MNT \
> > > + -o"upperdir=$upperdir,workdir=$workdir,lowerdir=$lowerdir_commas_esc" 2>> $seqres.full || \
> > > + echo "ERROR: incorrect parsing of escaped comma in lowerdir mount option"
> >
> > This version looks good to me. I just hope we can remove the "LIBMOUNT_FORCE_MOUNT2=always"
> > after that issue get fixed, to let this case cover new mount API test too.
> >
>
> TBH, I am not really sure the best approach here would be.
> Let's say that the issue gets fixed in libmount 2.40.
> Would we then remove "LIBMOUNT_FORCE_MOUNT2=always"
> and add a hint:
>
> _fixed_in_version libmount 2.40
Sure, that's good to me. If there's a git commit ($id $subject), I think
that would be better than the version number, e.g.
# A known issue of util-linux/libmount ....
_fixed_by_git_commit util-linux xxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxx
Due to ...
>
> Do you think that would be the appropriate thing to do for fstests?
>
> I am asking because so far fstests was used to track regressions
> in kernel and in various fs progs, e.g.:
> _fixed_by_git_commit btrfs-progs ...
> _fixed_by_git_commit xfsprogs ...
> _fixed_by_git_commit xfsdump ...
>
> Where developers of said fs progs often build their own binaries.
... as you say, downstream progs might have their own backport and build.
Thanks,
Zorro
>
> An alternative would be to extend _require_command to take
> an optional min_ver argument and try to figure out the version
> from running $command -V (best effort).
> I am not going to cheer for this approach...
>
> Anyway, we plan to add new overlayfs mount options that
> are only supported with FSCONFIG_SET_PATH and libmount 2.39
> does not support this yet, so are probably going to need to use
> a helper program in src to test the new mount API on overlayfs anyway.
>
> Thanks,
> Amir.
>
