linux-kernel - Re: [PATCH xfstests] overlay: Enable character device to be the base fs partition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 25 Sep 2019 11:15:54 +0800
From:   Eryu Guan <eguan@...ux.alibaba.com>
To:     Zhihao Cheng <chengzhihao1@...wei.com>
Cc:     Amir Goldstein <amir73il@...il.com>,
        Eryu Guan <guaneryu@...il.com>,
        David Oberhollenzer <david.oberhollenzer@...ma-star.at>,
        Eric Biggers <ebiggers@...gle.com>,
        "zhangyi (F)" <yi.zhang@...wei.com>,
        fstests <fstests@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH xfstests] overlay: Enable character device to be the base
 fs partition

On Tue, Sep 24, 2019 at 10:19:38PM +0800, Zhihao Cheng wrote:
> As far as I know, _require_scratch_shutdown() is called after _overay_config_override(), at this moment, FSTYP equals to base fs. According the implementation of _require_scratch_shutdown:
> 3090 _require_scratch_shutdown()
> 3091 {
> 3092     [ -x src/godown ] || _notrun "src/godown executable not found"
> 3093
> 3094     _scratch_mkfs > /dev/null 2>&1 || _notrun "_scratch_mkfs failed on $SCRATCH_DEV"
> 3095     _scratch_mount
> 3096
> 3097     if [ $FSTYP = "overlay" ]; then                                            # FSTYP = base fs
> 3098         if [ -z $OVL_BASE_SCRATCH_DEV ]; then
> 3099             # In lagacy overlay usage, it may specify directory as
> 3100             # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV
> 3101             # will be null, so check OVL_BASE_SCRATCH_DEV before
> 3102             # running shutdown to avoid shutting down base fs accidently.
> 3103             _notrun "$SCRATCH_DEV is not a block device"
> 3104         else
> 3105             src/godown -f $OVL_BASE_SCRATCH_MNT 2>&1 \
> 3106             || _notrun "Underlying filesystem does not support shutdown"
> 3107         fi
> 3108     else
> 3109         src/godown -f $SCRATCH_MNT 2>&1 \
> 3110             || _notrun "$FSTYP does not support shutdown"                      # Executes this path
> 3111     fi
> 3112
> 3113     _scratch_unmount
> 3114 }
> So, we can't get output: _notrun "$SCRATCH_DEV is not a block device". Instead, the verbose should like:
>   after _overlay_config_override FSTYP=ubifs    # Additional print message
>   FSTYP         -- ubifs
>   PLATFORM      -- Linux/x86_64
>   MKFS_OPTIONS  -- /dev/ubi0_1
>   MOUNT_OPTIONS -- -t ubifs /dev/ubi0_1 /tmp/scratch
> 
>   generic/042	[not run] ubifs does not support shutdown
> 
> But I'll consider describing error more concisely in v2.
> 
> 在 2019/9/24 20:33, Amir Goldstein 写道:
> > On Tue, Sep 24, 2019 at 12:34 PM Zhihao Cheng <chengzhihao1@...wei.com> wrote:
> >>
> >> When running overlay tests using character devices as base fs partitions,
> >> all overlay usecase results become 'notrun'. Function
> >> '_overay_config_override' (common/config) detects that the current base
> >> fs partition is not a block device and will set FSTYP to base fs. The
> >> overlay usecase will check the current FSTYP, and if it is not 'overlay'
> >> or 'generic', it will skip the execution.
> >>
> >> For example, using UBIFS as base fs skips all overlay usecases:
> >>
> >>   FSTYP         -- ubifs       # FSTYP should be overridden as 'overlay'
> >>   MKFS_OPTIONS  -- /dev/ubi0_1 # Character device
> >>   MOUNT_OPTIONS -- -t ubifs /dev/ubi0_1 /tmp/scratch
> >>
> >>   overlay/001   [not run] not suitable for this filesystem type: ubifs
> >>   overlay/002   [not run] not suitable for this filesystem type: ubifs
> >>   overlay/003   [not run] not suitable for this filesystem type: ubifs
> >>   ...
> >>
> >> When checking that the base fs partition is a block/character device,
> >> FSTYP is overwritten as 'overlay'. This patch allows the base fs
> >> partition to be a character device that can also execute overlay
> >> usecases (such as ubifs).
> >>
> >> Signed-off-by: Zhihao Cheng <chengzhihao1@...wei.com>
> >> ---
> >>  common/config | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/common/config b/common/config
> >> index 4c86a49..a22acdb 100644
> >> --- a/common/config
> >> +++ b/common/config
> >> @@ -550,7 +550,7 @@ _overlay_config_override()
> >>         #    the new OVL_BASE_SCRATCH/TEST_DEV/MNT vars are set to the values
> >>         #    of the configured base fs and SCRATCH/TEST_DEV vars are set to the
> >>         #    overlayfs base and mount dirs inside base fs mount.
> >> -       [ -b "$TEST_DEV" ] || return 0
> >> +       [ -b "$TEST_DEV" ] || [ -c "$TEST_DEV" ] || return 0
> >>
> >>         # Config file may specify base fs type, but we obay -overlay flag
> >>         [ "$FSTYP" == overlay ] || export OVL_BASE_FSTYP="$FSTYP"
> >> @@ -570,7 +570,7 @@ _overlay_config_override()
> >>         export TEST_DIR="$OVL_BASE_TEST_DIR/$OVL_MNT"
> >>         export MOUNT_OPTIONS="$OVERLAY_MOUNT_OPTIONS"
> >>
> >> -       [ -b "$SCRATCH_DEV" ] || return 0
> >> +       [ -b "$SCRATCH_DEV" ] || [ -c "$SCRATCH_DEV" ] || return 0
> >>
> >>         # Store original base fs vars
> >>         export OVL_BASE_SCRATCH_DEV="$SCRATCH_DEV"
> >> --
> >> 2.7.4
> >>
> > 
> > Looks fine.
> > 
> > One nit: there is a message in _require_scratch_shutdown():
> > _notrun "$SCRATCH_DEV is not a block device"
> > for when $OVL_BASE_SCRATCH_DEV is not defined.
> > 
> > Could probably use a better describing error anyway.

I think what Amir suggested is that, as you add char device support to
overlay base device, the message in _require_scratch_shutdown() should
be updated accordingly, not the commit log.

Thanks,
Eryu