Add UBIFS support to xfstests

Add UBIFS support to xfstests

[David Oberhollenzer]

Hello,


This patch series attempts to add support for UBIFS to xfstests, based
on the previous patches for xfstests-dev and xfstests-bld.


The first patch adds UBIFS support to xfstests-dev. It mainly deals with
a few quirks of UBIFS, e.g. requiring a char device rather than a block
device.

The second patch modifies an encryption test to also accept the slightly
different behaviour of UBIFS.


The final patch updates xfstests-bld (a separate project maintained by
Theodore Ts'o) to support UBIFS with kvm-xfstests and gce-xfstests. It
uses block2mtd to emulate MTD devices using standard block devices, then
layers UBI volumes on top of these.

It is basically Eric's original xfstests-bld patch but adds one line to
xfstests-packages to require the mtd-utils package to be added to the
file system image.


Modifying the xfstests-bld patch to use nandsim (which simulates a NAND
flash MTD device) instead of block2mtd has been considered, however the
default size of 128MiB is slightly too small for generic/129.

Nandsim does not have an option to directly specify the size of the
backing buffer but instead requires a set of NAND ID bytes that imply
(among other things) the size of the chip (example, see 1). The ID bytes
would have to be added to the kernel arguments for the UBIFS case. It
might be desirable to document this somehow, for the next person who
has to adjust the size, once a new test exhausts it. Also, it would be
ultimately limted by available RAM and what can be encoded using the
NAND ID bytes.

So right now, it looks like block2mtd is the simplest option for getting
xfstests-bld to work with UBIFS.


The first two patches have been tested inside a Debian Jessie VM, using
the UBIFS kernel tree from git://git.infradead.org/linux-ubifs.git and
nandsim for emulating a nand flash device.

The patch for xfstests-bld was tested in combination with the xfstests
patches on a clean Ubuntu 16.04 installation.


David

[1] http://www.linux-mtd.infradead.org/nand-data/nanddata.html

[PATCH 1/2] Add support for UBIFS

[David Oberhollenzer]

This patch is mostly based on the previous attempts of Eric Biggers
and Dongsheng Yang at adding UBIFS support to xfstests.

In addition to rebasing the previous attempts to recent xfstests,
most of the encryption tests now also support UBIFS with this patch.

Since mkfs.ubifs doesn't support creating encryption-capable filesystems
yet, _scratch_mkfs_encrypted() is modified to wipe the underlying UBI
volume. The filesystem is then created when mounting the emtpy volume.

Some tests that require SCRATCH_DEV to be a block device, despite
not using device-mapper or otherwise doing something block device
specific have that requirement removed.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 check             |  2 ++
 common/config     |  7 +++++++
 common/encrypt    |  3 +++
 common/rc         | 24 ++++++++++++++++++++++++
 tests/generic/076 |  1 -
 tests/generic/409 |  1 -
 tests/generic/410 |  1 -
 tests/generic/411 |  1 -
 8 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/check b/check
index 9cef58b4..f8db3cd6 100755
--- a/check
+++ b/check
@@ -70,6 +70,7 @@ check options
     -overlay		test overlay
     -pvfs2          test PVFS2
     -tmpfs              test TMPFS
+    -ubifs              test ubifs
     -l			line mode diff
     -udiff		show unified diff (default)
     -n			show me, do not run tests
@@ -267,6 +268,7 @@ while [ $# -gt 0 ]; do
 	-overlay)	FSTYP=overlay; export OVERLAY=true ;;
 	-pvfs2)		FSTYP=pvfs2 ;;
 	-tmpfs)		FSTYP=tmpfs ;;
+	-ubifs)		FSTYP=ubifs ;;
 
 	-g)	group=$2 ; shift ;
 		GROUP_LIST="$GROUP_LIST ${group//,/ }"
diff --git a/common/config b/common/config
index 59041a39..6c58e888 100644
--- a/common/config
+++ b/common/config
@@ -336,6 +336,9 @@ _mount_opts()
 		# We need to specify the size at mount, use 1G by default
 		export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS"
 		;;
+	ubifs)
+		export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS
+		;;
 	*)
 		;;
 	esac
@@ -475,6 +478,10 @@ _check_device()
 		if [ ! -d "$dev" ]; then
 			_fatal "common/config: $name ($dev) is not a directory for overlay"
 		fi
+	elif [ "$FSTYP" == "ubifs" ]; then
+		if [ ! -c "$dev" ]; then
+			_fatal "common/config: $name ($dev) is not a character device"
+		fi
 	else
 		_fatal "common/config: $name ($dev) is not a block device or a network filesystem"
 	fi
diff --git a/common/encrypt b/common/encrypt
index 723f1b11..b444c82d 100644
--- a/common/encrypt
+++ b/common/encrypt
@@ -71,6 +71,9 @@ _scratch_mkfs_encrypted()
 	ext4|f2fs)
 		_scratch_mkfs -O encrypt
 		;;
+	ubifs)
+		ubiupdatevol ${SCRATCH_DEV} -t
+		;;
 	*)
 		_notrun "No encryption support for $FSTYP"
 		;;
diff --git a/common/rc b/common/rc
index 257b1903..6191a0c0 100644
--- a/common/rc
+++ b/common/rc
@@ -1556,6 +1556,15 @@ _require_scratch_nocheck()
 		    _notrun "this test requires a valid \$SCRATCH_MNT and unique $SCRATCH_DEV"
 		fi
 		;;
+	ubifs)
+		# ubifs needs an UBI volume. This will be a char device, not a block device.
+		if [ ! -c "$SCRATCH_DEV" ]; then
+			_notrun "this test requires a valid UBI volume for \$SCRATCH_DEV"
+		fi
+		if [ ! -d "$SCRATCH_MNT" ]; then
+			_notrun "this test requires a valid \$SCRATCH_MNT"
+		fi
+		;;
 	*)
 		 if [ -z "$SCRATCH_DEV" -o "`_is_block_dev "$SCRATCH_DEV"`" = "" ]
 		 then
@@ -1650,6 +1659,15 @@ _require_test()
 		    _notrun "this test requires a valid \$TEST_DIR and unique $TEST_DEV"
 		fi
 		;;
+	ubifs)
+		# ubifs needs an UBI volume. This will be a char device, not a block device.
+		if [ ! -c "$TEST_DEV" ]; then
+			_notrun "this test requires a valid UBI volume for \$TEST_DEV"
+		fi
+		if [ ! -d "$TEST_DIR" ]; then
+			_notrun "this test requires a valid \$TEST_DIR"
+		fi
+		;;
 	*)
 		 if [ -z "$TEST_DEV" ] || [ "`_is_block_dev "$TEST_DEV"`" = "" ]
 		 then
@@ -2490,6 +2508,9 @@ _check_test_fs()
     tmpfs)
 	# no way to check consistency for tmpfs
 	;;
+    ubifs)
+	# there is no fsck program for ubifs yet
+	;;
     *)
 	_check_generic_filesystem $TEST_DEV
 	;;
@@ -2539,6 +2560,9 @@ _check_scratch_fs()
     tmpfs)
 	# no way to check consistency for tmpfs
 	;;
+    ubifs)
+	# there is no fsck program for ubifs yet
+	;;
     *)
 	_check_generic_filesystem $device
 	;;
diff --git a/tests/generic/076 b/tests/generic/076
index 64e69583..53e56a69 100755
--- a/tests/generic/076
+++ b/tests/generic/076
@@ -56,7 +56,6 @@ _supported_fs generic
 _supported_os IRIX Linux
 
 _require_scratch
-_require_block_device $SCRATCH_DEV
 
 echo "*** init fs"
 
diff --git a/tests/generic/409 b/tests/generic/409
index 4bfedf73..3be58ba5 100755
--- a/tests/generic/409
+++ b/tests/generic/409
@@ -64,7 +64,6 @@ _supported_fs generic
 _supported_os Linux
 _require_test
 _require_scratch
-_require_block_device $SCRATCH_DEV
 
 fs_stress()
 {
diff --git a/tests/generic/410 b/tests/generic/410
index f2e0d1bb..e3a933ce 100755
--- a/tests/generic/410
+++ b/tests/generic/410
@@ -72,7 +72,6 @@ _supported_fs generic
 _supported_os Linux
 _require_test
 _require_scratch
-_require_block_device $SCRATCH_DEV
 
 fs_stress()
 {
diff --git a/tests/generic/411 b/tests/generic/411
index 7b2dd33b..08b861c5 100755
--- a/tests/generic/411
+++ b/tests/generic/411
@@ -53,7 +53,6 @@ _supported_fs generic
 _supported_os Linux
 _require_test
 _require_scratch
-_require_block_device $SCRATCH_DEV
 
 fs_stress()
 {
-- 
2.12.0

[PATCH 2/2] Accept failing with EPERM in addition to ENOKEY for rename without key

[David Oberhollenzer]

Some filesystems (e.g. UBIFS) fail with EPERM when trying to move a
file into an encrypted directory via cross rename, without having
access to the encryption key.

Since rename is perfectly allowed to return EPERM, which is also tested
for, and no precise specification seems to exist that clarifys when to
expect EPERM and when ENOKEY, this patch modifies the generic/398 test
to accept both, for the test case where the key is not available.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 tests/generic/398 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/generic/398 b/tests/generic/398
index b486b9c5..390c4380 100755
--- a/tests/generic/398
+++ b/tests/generic/398
@@ -43,6 +43,12 @@ _cleanup()
 	rm -f $tmp.*
 }
 
+_filter_rename()
+{
+	# rename without key can also fail with EPERM instead of ENOKEY
+	sed -e "s/Operation not permitted/Required key not available/g"
+}
+
 # get standard environment, filters and checks
 . ./common/rc
 . ./common/filter
@@ -149,9 +155,9 @@ efile1=$(find $edir1 -type f)
 efile2=$(find $edir2 -type f)
 
 echo -e "\n\n*** Exchange encrypted <=> encrypted without key ***"
-src/renameat2 -x $efile1 $efile2
+src/renameat2 -x $efile1 $efile2 |& _filter_rename
 echo -e "\n*** Exchange encrypted <=> unencrypted without key ***"
-src/renameat2 -x $efile1 $udir/ufile
+src/renameat2 -x $efile1 $udir/ufile |& _filter_rename
 
 # success, all done
 status=0
-- 
2.12.0

[PATCH] xfstests-bld: add experimental support for ubifs

[David Oberhollenzer]

From: Eric Biggers <ebiggers@google.com>

This experimental patch adds ubifs support to kvm-xfstests and
gce-xfstests.  Unlike most filesystems ubifs is not block-device based.
Instead it uses an UBI volume, which is layered above an MTD (raw flash)
device.  Fortunately it's possible to use the block2mtd kernel module to
emulate an MTD device given a block device.  This patch adds an ubifs
xfstests-bld config which uses block2mtd to create MTD devices, then UBI
devices and UBI volumes, that are backed by the kvm-xfstests or
gce-xfstests block devices.

Here are the steps to run kvm-xfstests on ubifs after this patch:

1. Apply ubifs support patch to xfstests
2. Build a kernel based on the existing xfstests-bld config but also with:
	CONFIG_MTD=y
	CONFIG_MTD_BLOCK2MTD=y
	CONFIG_MTD_UBI=y
	CONFIG_UBIFS_FS=y
	CONFIG_UBIFS_FS_ENCRYPTION=y
3. Run: kvm-xfstests -c ubifs -g <group> ...

I also tried using QEMU to emulate a MTD device directly, but I couldn't
get it to work; and that definitely won't work for gce-xfstests.

Known issues (other than real test failures):

- The first time ubifs tests are run, it takes a long time to format the
  emulated MTD devices.  I don't know of a way around this other than
  simply using smaller devices.

- Using the same block devices for other filesystems and for MTD
  emulation can cause problems, e.g. without rebooting the VM, it's not
  possible to run tests on another filesystem after ubifs because
  block2mtd does not support removing MTD devices.

Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 .../files/root/fs/ubifs/cfg/all.list               |   1 +
 .../test-appliance/files/root/fs/ubifs/cfg/default |  11 ++
 .../test-appliance/files/root/fs/ubifs/config      | 167 +++++++++++++++++++++
 kvm-xfstests/test-appliance/files/root/runtests.sh |   4 +-
 kvm-xfstests/test-appliance/xfstests-packages      |   1 +
 5 files changed, 182 insertions(+), 2 deletions(-)
 create mode 100644 kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/all.list
 create mode 100644 kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/default
 create mode 100644 kvm-xfstests/test-appliance/files/root/fs/ubifs/config

diff --git a/kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/all.list b/kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/all.list
new file mode 100644
index 0000000..4ad96d5
--- /dev/null
+++ b/kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/all.list
@@ -0,0 +1 @@
+default
diff --git a/kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/default b/kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/default
new file mode 100644
index 0000000..be3ba67
--- /dev/null
+++ b/kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/default
@@ -0,0 +1,11 @@
+SIZE=small
+
+# set up UBI volumes for our block devices
+export TEST_DEV=$(__blkdev_to_ubi_volume $SM_TST_DEV)
+export TEST_DIR=$SM_TST_MNT
+export SCRATCH_DEV=$(__blkdev_to_ubi_volume $SM_SCR_DEV)
+export SCRATCH_MNT=$SM_SCR_MNT
+
+export MKFS_OPTIONS=""
+export UBIFS_MOUNT_OPTIONS=""
+TESTNAME="ubifs"
diff --git a/kvm-xfstests/test-appliance/files/root/fs/ubifs/config b/kvm-xfstests/test-appliance/files/root/fs/ubifs/config
new file mode 100644
index 0000000..91d8de6
--- /dev/null
+++ b/kvm-xfstests/test-appliance/files/root/fs/ubifs/config
@@ -0,0 +1,167 @@
+#
+# Configuration file for ubifs
+#
+
+DEFAULT_MKFS_OPTIONS=""
+
+function check_filesystem()
+{
+    # there is no fsck.ubifs yet
+    :
+}
+
+# Find the MTD device which is backed by the specified block device
+function __mtd_find()
+{
+    local blkdev=$1 mtd_dir
+
+    for mtd_dir in /sys/class/mtd/*; do
+	if [[ $mtd_dir =~ ^.*/mtd[0-9]+$ ]] &&
+	   [[ $(awk '/^block2mtd:/{print $2}' $mtd_dir/name) == $blkdev ]]
+	then
+	    echo /dev/$(basename $mtd_dir)
+	    return
+	fi
+    done
+}
+
+# Find or create the MTD device which is backed by the specified block device
+function __mtd_find_or_create()
+{
+    local blkdev=$1 mtd
+
+    if [ ! -e /sys/module/block2mtd ]; then
+	echo 1>&2 "Error: CONFIG_MTD_BLOCK2MTD=y is required to emulate flash device for ubifs!"
+	return
+    fi
+
+    mtd=$(__mtd_find $blkdev)
+    if [ ! -c "$mtd" ]; then
+	# Create a new block2mtd device.  For now choose an eraseblock size of
+	# 128 KiB.  I'm not sure if that's the best value or not.
+	echo "$blkdev,131072" > /sys/module/block2mtd/parameters/block2mtd
+	mtd=$(__mtd_find $blkdev)
+    fi
+    echo $mtd
+}
+
+# Find the UBI device which has the specified mtd device attached
+function __ubi_find()
+{
+    local mtd_num="${1#/dev/mtd}" ubi_dir
+
+    for ubi_dir in /sys/class/ubi/*; do
+	if [[ $ubi_dir =~ ^.*/ubi[0-9]+$ ]] &&
+	   [[ $(<$ubi_dir/mtd_num) == $mtd_num ]]
+	then
+	    echo /dev/$(basename $ubi_dir)
+	    return
+	fi
+    done
+}
+
+# Find or create the UBI device which has the specified mtd device attached
+function __ubi_find_or_create()
+{
+    local mtd="$1" ubi
+
+    ubi=$(__ubi_find $mtd)
+    if [ ! -c "$ubi" ]; then
+	if ! ubiattach -p $mtd &> /dev/null; then
+	    # ubiattach didn't work; try formatting the MTD device first.
+	    # Note: since this requires writing to the entire device, it may be
+	    # *very* slow...
+	    echo 1>&2 "Formatting $mtd with ubiformat (this may take a while!)..."
+	    ubiformat -e 0 -y $mtd > /dev/null
+	    ubiattach -p $mtd > /dev/null
+	fi
+	ubi=$(__ubi_find $mtd)
+    fi
+    echo $ubi
+}
+
+
+#
+# Find or create the UBI volume backed by the specified block device.
+#
+# There are four types of devices in play here.  Here's an example:
+#
+#	/dev/vdb     --- Block device
+#	/dev/mtd0    --- MTD device backed by /dev/vdb using block2mtd
+#	/dev/ubi0    --- UBI device to which /dev/mtd0 is attached
+#	/dev/ubi0_0  --- UBI volume within /dev/ubi0
+#
+# In this example, this function would take in /dev/vdb as $1 and echo back
+# /dev/ubi0_0, creating it and the two intermediary devices if needed.
+#
+function __blkdev_to_ubi_volume()
+{
+    local blkdev=$1 mtd ubi ubivol
+
+    mtd=$(__mtd_find_or_create $blkdev)
+    if [ ! -c "$mtd" ]; then
+	echo 1>&2 "Error: Failed to create MTD device from $blkdev!"
+	return
+    fi
+
+    ubi=$(__ubi_find_or_create $mtd)
+    if [ ! -c "$ubi" ]; then
+	echo 1>&2 "Error: Failed create UBI device from $mtd!"
+	return
+    fi
+
+    ubivol=${ubi}_0
+    if [ ! -c "$ubivol" ]; then
+	ubimkvol $ubi -N vol -m > /dev/null
+	if [ ! -c "$ubivol" ]; then
+	    echo 1>&2 "Error: Failed to create UBI volume $ubivol from $ubi"
+	    return
+	fi
+    fi
+    echo $ubivol
+}
+
+function format_filesystem()
+{
+    local dev="$1"
+    local opts="$2"
+
+    mkfs.ubifs -y $opts "$dev"
+}
+
+function setup_mount_opts()
+{
+    if test -n "$MNTOPTS" ; then
+	if test -n "$UBIFS_MOUNT_OPTIONS" ; then
+	    UBIFS_MOUNT_OPTIONS="$UBIFS_MOUNT_OPTIONS,$MNTOPTS"
+	else
+	    UBIFS_MOUNT_OPTIONS="-o $MNTOPTS"
+	fi
+    fi
+}
+
+function get_mkfs_opts()
+{
+    echo "$MKFS_OPTIONS"
+}
+
+function show_mkfs_opts()
+{
+    echo MKFS_OPTIONS: "$MKFS_OPTIONS"
+}
+
+function show_mount_opts()
+{
+    echo UBIFS_MOUNT_OPTIONS: "$UBIFS_MOUNT_OPTIONS"
+}
+
+function test_name_alias()
+{
+    echo "$1"
+}
+
+function reset_vars()
+{
+    unset UBIFS_MOUNT_OPTIONS
+    unset MKFS_OPTIONS
+}
diff --git a/kvm-xfstests/test-appliance/files/root/runtests.sh b/kvm-xfstests/test-appliance/files/root/runtests.sh
index 8363427..9577f3e 100755
--- a/kvm-xfstests/test-appliance/files/root/runtests.sh
+++ b/kvm-xfstests/test-appliance/files/root/runtests.sh
@@ -272,11 +272,11 @@ do
 	    */ovl) ;;
 	    *:/*) ;;
 	    *)
-		if ! test -b $TEST_DEV ; then
+		if ! [ -b $TEST_DEV -o -c $TEST_DEV ]; then
 		    echo "Test device $TEST_DEV does not exist, skipping $i config"
 		    continue
 		fi
-		if ! test -b $SCRATCH_DEV ; then
+		if ! [ -b $SCRATCH_DEV -o -c $SCRATCH_DEV ]; then
 		    echo "Scratch device $SCRATCH_DEV does not exist, skipping $i config"
 		    continue
 		fi
diff --git a/kvm-xfstests/test-appliance/xfstests-packages b/kvm-xfstests/test-appliance/xfstests-packages
index afb6c5d..2f8e4b0 100644
--- a/kvm-xfstests/test-appliance/xfstests-packages
+++ b/kvm-xfstests/test-appliance/xfstests-packages
@@ -30,3 +30,4 @@ time
 uuid-runtime
 udev
 xz-utils
+mtd-utils
-- 
2.12.0

Re: [PATCH 1/2] Add support for UBIFS

[Eryu Guan]

On Wed, May 17, 2017 at 11:55:29AM +0200, David Oberhollenzer wrote:
> This patch is mostly based on the previous attempts of Eric Biggers
> and Dongsheng Yang at adding UBIFS support to xfstests.
> 
> In addition to rebasing the previous attempts to recent xfstests,
> most of the encryption tests now also support UBIFS with this patch.
> 
> Since mkfs.ubifs doesn't support creating encryption-capable filesystems
> yet, _scratch_mkfs_encrypted() is modified to wipe the underlying UBI
> volume. The filesystem is then created when mounting the emtpy volume.
> 
> Some tests that require SCRATCH_DEV to be a block device, despite
> not using device-mapper or otherwise doing something block device
> specific have that requirement removed.
> 
> Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>

Thanks for the patch! I know only very little about ubifs (from previous
reviews to attempts to add ubifs support to fstests), but it's still
good to have some words to introduce ubifs briefly in commit log,
especially the fact that it uses char device not block device.

> ---
>  check             |  2 ++
>  common/config     |  7 +++++++
>  common/encrypt    |  3 +++
>  common/rc         | 24 ++++++++++++++++++++++++
>  tests/generic/076 |  1 -
>  tests/generic/409 |  1 -
>  tests/generic/410 |  1 -
>  tests/generic/411 |  1 -
>  8 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/check b/check
> index 9cef58b4..f8db3cd6 100755
> --- a/check
> +++ b/check
> @@ -70,6 +70,7 @@ check options
>      -overlay		test overlay
>      -pvfs2          test PVFS2
>      -tmpfs              test TMPFS
> +    -ubifs              test ubifs
>      -l			line mode diff
>      -udiff		show unified diff (default)
>      -n			show me, do not run tests
> @@ -267,6 +268,7 @@ while [ $# -gt 0 ]; do
>  	-overlay)	FSTYP=overlay; export OVERLAY=true ;;
>  	-pvfs2)		FSTYP=pvfs2 ;;
>  	-tmpfs)		FSTYP=tmpfs ;;
> +	-ubifs)		FSTYP=ubifs ;;

As being pointed out in previous reviews, it'll be great if we can probe
ubifs from the char device if possible instead of adding new fs-specific
option, just as what we're doing at the end of common/config for other
local filesystems. But I'm not sure if blkid works for char device and
ubifs (probably not).

>  
>  	-g)	group=$2 ; shift ;
>  		GROUP_LIST="$GROUP_LIST ${group//,/ }"
> diff --git a/common/config b/common/config
> index 59041a39..6c58e888 100644
> --- a/common/config
> +++ b/common/config
> @@ -336,6 +336,9 @@ _mount_opts()
>  		# We need to specify the size at mount, use 1G by default
>  		export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS"
>  		;;
> +	ubifs)
> +		export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS
> +		;;
>  	*)
>  		;;
>  	esac
> @@ -475,6 +478,10 @@ _check_device()
>  		if [ ! -d "$dev" ]; then
>  			_fatal "common/config: $name ($dev) is not a directory for overlay"
>  		fi
> +	elif [ "$FSTYP" == "ubifs" ]; then
> +		if [ ! -c "$dev" ]; then
> +			_fatal "common/config: $name ($dev) is not a character device"
> +		fi
>  	else
>  		_fatal "common/config: $name ($dev) is not a block device or a network filesystem"

This error message should be updated too. And turning this if-elif-fi
block to a case switch on $FSTYP seems cleaner.

And you need to setup MKFS_UBIFS_PROG and all other needed tools in
common/config too, and check if the tools are available in common/rc and
abort if the required tools are not met. e.g.

[ "$MKFS_EXT4_PROG" = "" ] && _fatal "mkfs.ext4 not found"

>  	fi
> diff --git a/common/encrypt b/common/encrypt
> index 723f1b11..b444c82d 100644
> --- a/common/encrypt
> +++ b/common/encrypt
> @@ -71,6 +71,9 @@ _scratch_mkfs_encrypted()
>  	ext4|f2fs)
>  		_scratch_mkfs -O encrypt
>  		;;
> +	ubifs)
> +		ubiupdatevol ${SCRATCH_DEV} -t
> +		;;

Need a commont on this.

>  	*)
>  		_notrun "No encryption support for $FSTYP"
>  		;;
> diff --git a/common/rc b/common/rc
> index 257b1903..6191a0c0 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1556,6 +1556,15 @@ _require_scratch_nocheck()
>  		    _notrun "this test requires a valid \$SCRATCH_MNT and unique $SCRATCH_DEV"
>  		fi
>  		;;
> +	ubifs)
> +		# ubifs needs an UBI volume. This will be a char device, not a block device.
> +		if [ ! -c "$SCRATCH_DEV" ]; then
> +			_notrun "this test requires a valid UBI volume for \$SCRATCH_DEV"
> +		fi
> +		if [ ! -d "$SCRATCH_MNT" ]; then
> +			_notrun "this test requires a valid \$SCRATCH_MNT"
> +		fi
> +		;;
>  	*)
>  		 if [ -z "$SCRATCH_DEV" -o "`_is_block_dev "$SCRATCH_DEV"`" = "" ]
>  		 then
> @@ -1650,6 +1659,15 @@ _require_test()
>  		    _notrun "this test requires a valid \$TEST_DIR and unique $TEST_DEV"
>  		fi
>  		;;
> +	ubifs)
> +		# ubifs needs an UBI volume. This will be a char device, not a block device.
> +		if [ ! -c "$TEST_DEV" ]; then
> +			_notrun "this test requires a valid UBI volume for \$TEST_DEV"
> +		fi
> +		if [ ! -d "$TEST_DIR" ]; then
> +			_notrun "this test requires a valid \$TEST_DIR"
> +		fi
> +		;;
>  	*)
>  		 if [ -z "$TEST_DEV" ] || [ "`_is_block_dev "$TEST_DEV"`" = "" ]
>  		 then
> @@ -2490,6 +2508,9 @@ _check_test_fs()
>      tmpfs)
>  	# no way to check consistency for tmpfs
>  	;;
> +    ubifs)
> +	# there is no fsck program for ubifs yet
> +	;;
>      *)
>  	_check_generic_filesystem $TEST_DEV
>  	;;
> @@ -2539,6 +2560,9 @@ _check_scratch_fs()
>      tmpfs)
>  	# no way to check consistency for tmpfs
>  	;;
> +    ubifs)
> +	# there is no fsck program for ubifs yet
> +	;;
>      *)
>  	_check_generic_filesystem $device
>  	;;
> diff --git a/tests/generic/076 b/tests/generic/076
> index 64e69583..53e56a69 100755
> --- a/tests/generic/076
> +++ b/tests/generic/076
> @@ -56,7 +56,6 @@ _supported_fs generic
>  _supported_os IRIX Linux
>  
>  _require_scratch
> -_require_block_device $SCRATCH_DEV

This test does "cat $SCRATCH_DEV >/dev/null &" which assumes
$SCRATCH_DEV is a local device, and test fails if it's a network device,
or an overlayfs pseudo mount device. And similarly generic/409 to
generic/411 fail if SCRATCH_DEV is a overlay pseudo mount device.

So I think what's needed here is a new helper that requires a local
device to make sure the given device resides on local host and is either
block or char device, e.g. something like

_require_local_device $SCRATCH_DEV

Thanks,
Eryu

>  
>  echo "*** init fs"
>  
> diff --git a/tests/generic/409 b/tests/generic/409
> index 4bfedf73..3be58ba5 100755
> --- a/tests/generic/409
> +++ b/tests/generic/409
> @@ -64,7 +64,6 @@ _supported_fs generic
>  _supported_os Linux
>  _require_test
>  _require_scratch
> -_require_block_device $SCRATCH_DEV
>  
>  fs_stress()
>  {
> diff --git a/tests/generic/410 b/tests/generic/410
> index f2e0d1bb..e3a933ce 100755
> --- a/tests/generic/410
> +++ b/tests/generic/410
> @@ -72,7 +72,6 @@ _supported_fs generic
>  _supported_os Linux
>  _require_test
>  _require_scratch
> -_require_block_device $SCRATCH_DEV
>  
>  fs_stress()
>  {
> diff --git a/tests/generic/411 b/tests/generic/411
> index 7b2dd33b..08b861c5 100755
> --- a/tests/generic/411
> +++ b/tests/generic/411
> @@ -53,7 +53,6 @@ _supported_fs generic
>  _supported_os Linux
>  _require_test
>  _require_scratch
> -_require_block_device $SCRATCH_DEV
>  
>  fs_stress()
>  {
> -- 
> 2.12.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Add support for UBIFS

[Eric Biggers]

On Wed, May 17, 2017 at 07:53:55PM +0800, Eryu Guan wrote:
> On Wed, May 17, 2017 at 11:55:29AM +0200, David Oberhollenzer wrote:
> > This patch is mostly based on the previous attempts of Eric Biggers
> > and Dongsheng Yang at adding UBIFS support to xfstests.
> > 
> > In addition to rebasing the previous attempts to recent xfstests,
> > most of the encryption tests now also support UBIFS with this patch.
> > 
> > Since mkfs.ubifs doesn't support creating encryption-capable filesystems
> > yet, _scratch_mkfs_encrypted() is modified to wipe the underlying UBI
> > volume. The filesystem is then created when mounting the emtpy volume.
> > 
> > Some tests that require SCRATCH_DEV to be a block device, despite
> > not using device-mapper or otherwise doing something block device
> > specific have that requirement removed.
> > 
> > Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> 
> Thanks for the patch! I know only very little about ubifs (from previous
> reviews to attempts to add ubifs support to fstests), but it's still
> good to have some words to introduce ubifs briefly in commit log,
> especially the fact that it uses char device not block device.
> 
> > ---
> >  check             |  2 ++
> >  common/config     |  7 +++++++
> >  common/encrypt    |  3 +++
> >  common/rc         | 24 ++++++++++++++++++++++++
> >  tests/generic/076 |  1 -
> >  tests/generic/409 |  1 -
> >  tests/generic/410 |  1 -
> >  tests/generic/411 |  1 -
> >  8 files changed, 36 insertions(+), 4 deletions(-)
> > 
> > diff --git a/check b/check
> > index 9cef58b4..f8db3cd6 100755
> > --- a/check
> > +++ b/check
> > @@ -70,6 +70,7 @@ check options
> >      -overlay		test overlay
> >      -pvfs2          test PVFS2
> >      -tmpfs              test TMPFS
> > +    -ubifs              test ubifs
> >      -l			line mode diff
> >      -udiff		show unified diff (default)
> >      -n			show me, do not run tests
> > @@ -267,6 +268,7 @@ while [ $# -gt 0 ]; do
> >  	-overlay)	FSTYP=overlay; export OVERLAY=true ;;
> >  	-pvfs2)		FSTYP=pvfs2 ;;
> >  	-tmpfs)		FSTYP=tmpfs ;;
> > +	-ubifs)		FSTYP=ubifs ;;
> 
> As being pointed out in previous reviews, it'll be great if we can probe
> ubifs from the char device if possible instead of adding new fs-specific
> option, just as what we're doing at the end of common/config for other
> local filesystems. But I'm not sure if blkid works for char device and
> ubifs (probably not).
> 

It seems to work fine without the -ubifs option:

# blkid -o value -s TYPE /dev/ubi0_0
ubifs

# TEST_DEV=/dev/ubi0_0 TEST_DIR=/vdb ./check  generic/001
FSTYP         -- ubifs
PLATFORM      -- Linux/x86_64 kvm-xfstests 4.12.0-rc1-xfstests-00083-ga844e08648f0-dirty

generic/001	[  372.213194] run fstests generic/001 at 2017-05-17 11:43:47
 8s
Ran: generic/001
Passed all 1 tests

- Eric

Re: Add UBIFS support to xfstests

[Eric Biggers]

Hi David,

On Wed, May 17, 2017 at 11:55:28AM +0200, David Oberhollenzer wrote:
> 
> Modifying the xfstests-bld patch to use nandsim (which simulates a NAND
> flash MTD device) instead of block2mtd has been considered, however the
> default size of 128MiB is slightly too small for generic/129.
> 
> Nandsim does not have an option to directly specify the size of the
> backing buffer but instead requires a set of NAND ID bytes that imply
> (among other things) the size of the chip (example, see 1). The ID bytes
> would have to be added to the kernel arguments for the UBIFS case. It
> might be desirable to document this somehow, for the next person who
> has to adjust the size, once a new test exhausts it. Also, it would be
> ultimately limted by available RAM and what can be encoded using the
> NAND ID bytes.
> 
> So right now, it looks like block2mtd is the simplest option for getting
> xfstests-bld to work with UBIFS.
>

It probably would be a good idea to document this decision somewhere more
permanent, maybe in a comment in
kvm-xfstests/test-appliance/files/root/fs/ubifs/config, which is where the
block2mtd devices get set up.

Or, it could be useful to add a documentation file for UBIFS in the xfstests-bld
documentation directory, which would also document any "gotchas" specific to how
UBIFS support is implemented in the test appliances, e.g the two issues I listed
in my patch description.  Also the fact that you need to configure your kernel
with UBIFS and block2mtd support.  Ideally we'd just fix these issues, though,
so there would be nothing special to document.  (We could at least solve the
kernel configuration issue by renaming the example configs in the
kernel-configs/ directory from "ext4-" to "xfstests-", then enabling support for
ubifs, f2fs, btrfs, etc.  in them.)

Eric

Re: [PATCH 2/2] Accept failing with EPERM in addition to ENOKEY for rename without key

[Eric Biggers]

Hi David,

On Wed, May 17, 2017 at 11:55:30AM +0200, David Oberhollenzer wrote:
> Some filesystems (e.g. UBIFS) fail with EPERM when trying to move a
> file into an encrypted directory via cross rename, without having
> access to the encryption key.
> 
> Since rename is perfectly allowed to return EPERM, which is also tested
> for, and no precise specification seems to exist that clarifys when to
> expect EPERM and when ENOKEY, this patch modifies the generic/398 test
> to accept both, for the test case where the key is not available.
> 

Specifically, the operation is expected to fail for two logically independent
reasons:
- It shouldn't be possible to link or rename an unencrypted file into an
  encrypted directory, as this violates the "one encryption policy per directory
  tree" constraint (EPERM)
- It shouldn't be possible to link or rename files into an encrypted directory
  without the directory's encryption key (ENOKEY)

Therefore it's not an ideal test, though it seemed worthwhile originally because
it was, in fact, broken in both ways, causing the operation to succeed :)

> Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> ---
>  tests/generic/398 | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/generic/398 b/tests/generic/398
> index b486b9c5..390c4380 100755
> --- a/tests/generic/398
> +++ b/tests/generic/398
> @@ -43,6 +43,12 @@ _cleanup()
>  	rm -f $tmp.*
>  }
>  
> +_filter_rename()
> +{
> +	# rename without key can also fail with EPERM instead of ENOKEY
> +	sed -e "s/Operation not permitted/Required key not available/g"
> +}
> +

It doesn't matter much, but IMO it would make a little more sense to substitute
ENOKEY with EPERM instead of the other way around, then updating 398.out to
expect "Operation not permitted".  The reason for this is that everything else
in generic/398 is testing for EPERM, because the test script is checking for
situations in which the "one encryption policy per directory tree" constraint
can be violated, and such violations normally result in EPERM.

Eric

Re: [PATCH 1/2] Add support for UBIFS

[David Oberhollenzer]

On 05/17/2017 01:53 PM, Eryu Guan wrote:
> On Wed, May 17, 2017 at 11:55:29AM +0200, David Oberhollenzer wrote:
>>  	-g)	group=$2 ; shift ;
>>  		GROUP_LIST="$GROUP_LIST ${group//,/ }"
>> diff --git a/common/config b/common/config
>> index 59041a39..6c58e888 100644
>> --- a/common/config
>> +++ b/common/config
>> @@ -336,6 +336,9 @@ _mount_opts()
>>  		# We need to specify the size at mount, use 1G by default
>>  		export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS"
>>  		;;
>> +	ubifs)
>> +		export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS
>> +		;;
>>  	*)
>>  		;;
>>  	esac
>> @@ -475,6 +478,10 @@ _check_device()
>>  		if [ ! -d "$dev" ]; then
>>  			_fatal "common/config: $name ($dev) is not a directory for overlay"
>>  		fi
>> +	elif [ "$FSTYP" == "ubifs" ]; then
>> +		if [ ! -c "$dev" ]; then
>> +			_fatal "common/config: $name ($dev) is not a character device"
>> +		fi
>>  	else
>>  		_fatal "common/config: $name ($dev) is not a block device or a network filesystem"
> 
> This error message should be updated too. And turning this if-elif-fi
> block to a case switch on $FSTYP seems cleaner.
> 
> And you need to setup MKFS_UBIFS_PROG and all other needed tools in
> common/config too, and check if the tools are available in common/rc and
> abort if the required tools are not met. e.g.
> 
> [ "$MKFS_EXT4_PROG" = "" ] && _fatal "mkfs.ext4 not found"
> 

mkfs.ubifs itself isn't needed as empty ubi volumes are formated when
mounting them with UBIFS.

I think it would make sense to add a check for ubiupdatevol to
_require_scratch_encryption (used in _scratch_mkfs_encrypted to
whipe an existing volume).


On 05/17/2017 08:45 PM, Eric Biggers wrote:
> On Wed, May 17, 2017 at 07:53:55PM +0800, Eryu Guan wrote:
>> As being pointed out in previous reviews, it'll be great if we can probe
>> ubifs from the char device if possible instead of adding new fs-specific
>> option, just as what we're doing at the end of common/config for other
>> local filesystems. But I'm not sure if blkid works for char device and
>> ubifs (probably not).
>>
> 
> It seems to work fine without the -ubifs option:
> 
> # blkid -o value -s TYPE /dev/ubi0_0
> ubifs

I can't really reproduce this on my end. Neither on my Debian test VM,
nor on the OpenSUSE system that I'm working on right now. I get no
output from blkid, neither before nor after mounting the ubi volume.

To be fair, the Debian version (and thus its blkid version) is rather
old (blkid 1.0.0 (12-Feb-2003)), but the one on OpenSUSE seems to be
fairly recent:

$ blkid -v
blkid from util-linux 2.28  (libblkid 2.28., 12-Apr-2016)


Would it make sense to patch the check in common/config instead,
to default to ubifs if FSTYP is not set and the target is a
character device? Or simply require on FSTYP to be set in the
config file?


Thanks,

David

Re: [PATCH 1/2] Add support for UBIFS

[Eryu Guan]

On Thu, May 18, 2017 at 10:41:27AM +0200, David Oberhollenzer wrote:
> On 05/17/2017 01:53 PM, Eryu Guan wrote:
> > On Wed, May 17, 2017 at 11:55:29AM +0200, David Oberhollenzer wrote:
> >>  	-g)	group=$2 ; shift ;
> >>  		GROUP_LIST="$GROUP_LIST ${group//,/ }"
> >> diff --git a/common/config b/common/config
> >> index 59041a39..6c58e888 100644
> >> --- a/common/config
> >> +++ b/common/config
> >> @@ -336,6 +336,9 @@ _mount_opts()
> >>  		# We need to specify the size at mount, use 1G by default
> >>  		export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS"
> >>  		;;
> >> +	ubifs)
> >> +		export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS
> >> +		;;
> >>  	*)
> >>  		;;
> >>  	esac
> >> @@ -475,6 +478,10 @@ _check_device()
> >>  		if [ ! -d "$dev" ]; then
> >>  			_fatal "common/config: $name ($dev) is not a directory for overlay"
> >>  		fi
> >> +	elif [ "$FSTYP" == "ubifs" ]; then
> >> +		if [ ! -c "$dev" ]; then
> >> +			_fatal "common/config: $name ($dev) is not a character device"
> >> +		fi
> >>  	else
> >>  		_fatal "common/config: $name ($dev) is not a block device or a network filesystem"
> > 
> > This error message should be updated too. And turning this if-elif-fi
> > block to a case switch on $FSTYP seems cleaner.
> > 
> > And you need to setup MKFS_UBIFS_PROG and all other needed tools in
> > common/config too, and check if the tools are available in common/rc and
> > abort if the required tools are not met. e.g.
> > 
> > [ "$MKFS_EXT4_PROG" = "" ] && _fatal "mkfs.ext4 not found"
> > 
> 
> mkfs.ubifs itself isn't needed as empty ubi volumes are formated when
> mounting them with UBIFS.

But there's still a mkfs.ubifs binary, right? I searched fstests mail
archive and found that Dongsheng Yang did set MKFS_UBIFS_PROG in his
patch in 2015. Then I suspect if mkfs.ubifs binary is unavailable, then
_scratch_mkfs would complain mkfs.ubifs not found and fail the test.

If it's not needed or there's no mkfs.ubifs exists, then _scratch_mkfs
needs some updates I guess, and describe this in commit log too, because
it's too different from other block device based local filesystems.

> 
> I think it would make sense to add a check for ubiupdatevol to
> _require_scratch_encryption (used in _scratch_mkfs_encrypted to
> whipe an existing volume).

Agreed.

> 
> 
> On 05/17/2017 08:45 PM, Eric Biggers wrote:
> > On Wed, May 17, 2017 at 07:53:55PM +0800, Eryu Guan wrote:
> >> As being pointed out in previous reviews, it'll be great if we can probe
> >> ubifs from the char device if possible instead of adding new fs-specific
> >> option, just as what we're doing at the end of common/config for other
> >> local filesystems. But I'm not sure if blkid works for char device and
> >> ubifs (probably not).
> >>
> > 
> > It seems to work fine without the -ubifs option:
> > 
> > # blkid -o value -s TYPE /dev/ubi0_0
> > ubifs
> 
> I can't really reproduce this on my end. Neither on my Debian test VM,
> nor on the OpenSUSE system that I'm working on right now. I get no
> output from blkid, neither before nor after mounting the ubi volume.
> 
> To be fair, the Debian version (and thus its blkid version) is rather
> old (blkid 1.0.0 (12-Feb-2003)), but the one on OpenSUSE seems to be
> fairly recent:
> 
> $ blkid -v
> blkid from util-linux 2.28  (libblkid 2.28., 12-Apr-2016)
> 
> 
> Would it make sense to patch the check in common/config instead,
> to default to ubifs if FSTYP is not set and the target is a
> character device? Or simply require on FSTYP to be set in the
> config file?

If there's really no standard way to probe for ubifs, a "-ubifs" option
would be the only choice I think, and better to have some comments too.

Thanks,
Eryu