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
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.
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.
