[fstests PATCH] generic: add test for executables on read-only DAX mounts

[fstests PATCH] generic: add test for executables on read-only DAX mounts

[Ross Zwisler]

This adds a regression test for the following kernel patch:

commit 42d4a99b09cb ("ext4: fix fault handling when mounted with -o dax,ro")

The above patch fixes an issue with ext4 where executables cannot be run on
read-only filesystems mounted with the DAX option.

This issue does not appear to be present in ext2 or XFS, as they both pass
the test.  I've also confirmed outside of the test that they are both
indeed able to execute binaries on read-only DAX mounts.

Thanks to Randy Dodgen for the bug report and reproduction steps.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Randy Dodgen <rdodgen@gmail.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Theodore Ts'o <tytso@mit.edu>

---

Sorry if we collided, Randy, but I was 90% done with the test by the time I
saw your mail. :)
---
 tests/generic/451     | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/451.out |  2 ++
 tests/generic/group   |  1 +
 3 files changed, 90 insertions(+)
 create mode 100755 tests/generic/451
 create mode 100644 tests/generic/451.out

diff --git a/tests/generic/451 b/tests/generic/451
new file mode 100755
index 0000000..54dda8c
--- /dev/null
+++ b/tests/generic/451
@@ -0,0 +1,87 @@
+#! /bin/bash
+# FS QA Test 451
+#
+# This is a regression test for kernel patch:
+#   commit 42d4a99b09cb ("ext4: fix fault handling when mounted with -o dax,ro")
+# created by Ross Zwisler <ross.zwisler@linux.intel.com>
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2017 Intel Corporation.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+
+# real QA test starts here
+# format and mount
+_scratch_mkfs > $seqres.full 2>&1
+_scratch_mount >> $seqres.full 2>&1
+
+LS=$(which ls --skip-alias --skip-functions) 2>/dev/null
+if [ $? -ne 0 ]; then
+	status=$?
+	echo "Couldn't find 'ls'!?"
+	exit
+fi
+
+cp $LS $SCRATCH_MNT
+SCRATCH_LS=$SCRATCH_MNT/ls
+
+$SCRATCH_LS >/dev/null 2>&1
+if [ $? -ne 0 ]; then
+	status=$?
+	echo "$SCRATCH_LS not working before remount"
+	exit
+fi
+
+_scratch_remount ro
+
+$SCRATCH_LS >/dev/null 2>&1
+if [ $? -ne 0 ]; then
+	status=$?
+	echo "$SCRATCH_LS not working after remount"
+	exit
+fi
+
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/generic/451.out b/tests/generic/451.out
new file mode 100644
index 0000000..db92441
--- /dev/null
+++ b/tests/generic/451.out
@@ -0,0 +1,2 @@
+QA output created by 451
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index 044ec3f..45f5789 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -453,3 +453,4 @@
 448 auto quick rw
 449 auto quick acl enospc
 450 auto quick rw
+451 auto quick
-- 
2.9.5

Re: [fstests PATCH] generic: add test for executables on read-only DAX mounts

[Eryu Guan]

On Tue, Aug 29, 2017 at 04:37:15PM -0600, Ross Zwisler wrote:
> This adds a regression test for the following kernel patch:
> 
> commit 42d4a99b09cb ("ext4: fix fault handling when mounted with -o dax,ro")
> 
> The above patch fixes an issue with ext4 where executables cannot be run on
> read-only filesystems mounted with the DAX option.
> 
> This issue does not appear to be present in ext2 or XFS, as they both pass
> the test.  I've also confirmed outside of the test that they are both
> indeed able to execute binaries on read-only DAX mounts.
> 
> Thanks to Randy Dodgen for the bug report and reproduction steps.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: Randy Dodgen <rdodgen@gmail.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> 
> ---
> 
> Sorry if we collided, Randy, but I was 90% done with the test by the time I
> saw your mail. :)
> ---
>  tests/generic/452     | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/452.out |  2 ++
>  tests/generic/group   |  1 +
>  3 files changed, 90 insertions(+)
>  create mode 100755 tests/generic/452
>  create mode 100644 tests/generic/452.out
> 
> diff --git a/tests/generic/452 b/tests/generic/452
> new file mode 100755
> index 0000000..54dda8c
> --- /dev/null
> +++ b/tests/generic/452
> @@ -0,0 +1,87 @@
> +#! /bin/bash
> +# FS QA Test 452
> +#
> +# This is a regression test for kernel patch:
> +#   commit 42d4a99b09cb ("ext4: fix fault handling when mounted with -o dax,ro")
> +# created by Ross Zwisler <ross.zwisler@linux.intel.com>
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017 Intel Corporation.  All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_supported_os Linux
> +_require_scratch
> +
> +# real QA test starts here
> +# format and mount
> +_scratch_mkfs > $seqres.full 2>&1
> +_scratch_mount >> $seqres.full 2>&1

Need to _notrun if scratch device was mounted with "noexec" option.

_exclude_scratch_mount_option "noexec"

> +
> +LS=$(which ls --skip-alias --skip-functions) 2>/dev/null
> +if [ $? -ne 0 ]; then
> +	status=$?
> +	echo "Couldn't find 'ls'!?"
> +	exit
> +fi
> +
> +cp $LS $SCRATCH_MNT
> +SCRATCH_LS=$SCRATCH_MNT/ls
> +
> +$SCRATCH_LS >/dev/null 2>&1
> +if [ $? -ne 0 ]; then
> +	status=$?
> +	echo "$SCRATCH_LS not working before remount"
> +	exit
> +fi
> +
> +_scratch_remount ro
> +
> +$SCRATCH_LS >/dev/null 2>&1
> +if [ $? -ne 0 ]; then
> +	status=$?
> +	echo "$SCRATCH_LS not working after remount"
> +	exit
> +fi

Hmm, I don't think all these checks on return value are needed, just
print out the error messages on failure and that will break the golden
image, as this is not a destructive test, continuing with errors only
fail the test :)

I think this can be simplified to something like:

LS=$(which ls ....)
SCRATCH_LS=$SCRATCH_MNT/ls_on_scratch
cp $LS $SCRATCH_LS

$SCRATCH_LS $SCRATCH_LS | _filter_scratch

_scratch_remount ro

$SCRATCH_LS $SCRATCH_LS | _filter_scratch

And update .out file accordingly.

Thanks,
Eryu

> +
> +# success, all done
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/generic/452.out b/tests/generic/452.out
> new file mode 100644
> index 0000000..db92441
> --- /dev/null
> +++ b/tests/generic/452.out
> @@ -0,0 +1,2 @@
> +QA output created by 452
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index 044ec3f..45f5789 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -453,3 +453,4 @@
>  449 auto quick acl enospc
>  450 auto quick rw
>  451 auto quick rw aio
> +452 auto quick
> -- 
> 2.9.5
> 
> --
> 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: [fstests PATCH] generic: add test for executables on read-only DAX mounts

[Ross Zwisler]

On Wed, Aug 30, 2017 at 06:59:30PM +0800, Eryu Guan wrote:
> On Tue, Aug 29, 2017 at 04:37:15PM -0600, Ross Zwisler wrote:
> > This adds a regression test for the following kernel patch:
> > 
> > commit 42d4a99b09cb ("ext4: fix fault handling when mounted with -o dax,ro")
> > 
> > The above patch fixes an issue with ext4 where executables cannot be run on
> > read-only filesystems mounted with the DAX option.
> > 
> > This issue does not appear to be present in ext2 or XFS, as they both pass
> > the test.  I've also confirmed outside of the test that they are both
> > indeed able to execute binaries on read-only DAX mounts.
> > 
> > Thanks to Randy Dodgen for the bug report and reproduction steps.
> > 
> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > Cc: Randy Dodgen <rdodgen@gmail.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Cc: Theodore Ts'o <tytso@mit.edu>
> > 
> > ---
> > 
> > Sorry if we collided, Randy, but I was 90% done with the test by the time I
> > saw your mail. :)
> > ---
> >  tests/generic/452     | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/452.out |  2 ++
> >  tests/generic/group   |  1 +
> >  3 files changed, 90 insertions(+)
> >  create mode 100755 tests/generic/452
> >  create mode 100644 tests/generic/452.out
> > 
> > diff --git a/tests/generic/452 b/tests/generic/452
> > new file mode 100755
> > index 0000000..54dda8c
> > --- /dev/null
> > +++ b/tests/generic/452
> > @@ -0,0 +1,87 @@
> > +#! /bin/bash
> > +# FS QA Test 452
> > +#
> > +# This is a regression test for kernel patch:
> > +#   commit 42d4a99b09cb ("ext4: fix fault handling when mounted with -o dax,ro")
> > +# created by Ross Zwisler <ross.zwisler@linux.intel.com>
> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright (c) 2017 Intel Corporation.  All Rights Reserved.
> > +#
> > +# This program is free software; you can redistribute it and/or
> > +# modify it under the terms of the GNU General Public License as
> > +# published by the Free Software Foundation.
> > +#
> > +# This program is distributed in the hope that it would be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program; if not, write the Free Software Foundation,
> > +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > +#-----------------------------------------------------------------------
> > +#
> > +
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1	# failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> > +_supported_os Linux
> > +_require_scratch
> > +
> > +# real QA test starts here
> > +# format and mount
> > +_scratch_mkfs > $seqres.full 2>&1
> > +_scratch_mount >> $seqres.full 2>&1
> 
> Need to _notrun if scratch device was mounted with "noexec" option.
> 
> _exclude_scratch_mount_option "noexec"
> 
> > +
> > +LS=$(which ls --skip-alias --skip-functions) 2>/dev/null
> > +if [ $? -ne 0 ]; then
> > +	status=$?
> > +	echo "Couldn't find 'ls'!?"
> > +	exit
> > +fi
> > +
> > +cp $LS $SCRATCH_MNT
> > +SCRATCH_LS=$SCRATCH_MNT/ls
> > +
> > +$SCRATCH_LS >/dev/null 2>&1
> > +if [ $? -ne 0 ]; then
> > +	status=$?
> > +	echo "$SCRATCH_LS not working before remount"
> > +	exit
> > +fi
> > +
> > +_scratch_remount ro
> > +
> > +$SCRATCH_LS >/dev/null 2>&1
> > +if [ $? -ne 0 ]; then
> > +	status=$?
> > +	echo "$SCRATCH_LS not working after remount"
> > +	exit
> > +fi
> 
> Hmm, I don't think all these checks on return value are needed, just
> print out the error messages on failure and that will break the golden
> image, as this is not a destructive test, continuing with errors only
> fail the test :)
> 
> I think this can be simplified to something like:
> 
> LS=$(which ls ....)
> SCRATCH_LS=$SCRATCH_MNT/ls_on_scratch
> cp $LS $SCRATCH_LS
> 
> $SCRATCH_LS $SCRATCH_LS | _filter_scratch
> 
> _scratch_remount ro
> 
> $SCRATCH_LS $SCRATCH_LS | _filter_scratch
> 
> And update .out file accordingly.
> 
> Thanks,
> Eryu

Awesome, thanks for the review!  I'll fix all of these in v2.

[fstests v2] generic: add test for executables on read-only DAX mounts

[Ross Zwisler]

This adds a regression test for the following kernel patch:

commit 42d4a99b09cb ("ext4: fix fault handling when mounted with -o dax,ro")

The above patch fixes an issue with ext4 where executables cannot be run on
read-only filesystems mounted with the DAX option.

This issue does not appear to be present in ext2 or XFS, as they both pass
the test.  I've also confirmed outside of the test that they are both
indeed able to execute binaries on read-only DAX mounts.

Thanks to Randy Dodgen for the bug report and reproduction steps.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Randy Dodgen <rdodgen@gmail.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Theodore Ts'o <tytso@mit.edu>

---

Changes in v2:
 - A bunch of cleanup and simplifications from Eryu.
---
 tests/generic/451     | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/451.out |  3 +++
 tests/generic/group   |  1 +
 3 files changed, 77 insertions(+)
 create mode 100755 tests/generic/451
 create mode 100644 tests/generic/451.out

diff --git a/tests/generic/451 b/tests/generic/451
new file mode 100755
index 0000000..785c52e
--- /dev/null
+++ b/tests/generic/451
@@ -0,0 +1,73 @@
+#! /bin/bash
+# FS QA Test 451
+#
+# This is a regression test for kernel patch:
+#   commit 42d4a99b09cb ("ext4: fix fault handling when mounted with -o dax,ro")
+# created by Ross Zwisler <ross.zwisler@linux.intel.com>
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2017 Intel Corporation.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+
+# we need to be able to execute binaries on scratch
+_exclude_scratch_mount_option "noexec"
+
+# real QA test starts here
+# format and mount
+_scratch_mkfs > $seqres.full 2>&1
+_scratch_mount >> $seqres.full 2>&1
+
+LS=$(which ls --skip-alias --skip-functions)
+SCRATCH_LS=$SCRATCH_MNT/ls_on_scratch
+cp $LS $SCRATCH_LS
+
+$SCRATCH_LS $SCRATCH_LS | _filter_scratch
+
+_scratch_remount ro
+
+$SCRATCH_LS $SCRATCH_LS | _filter_scratch
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/451.out b/tests/generic/451.out
new file mode 100644
index 0000000..70e3126
--- /dev/null
+++ b/tests/generic/451.out
@@ -0,0 +1,3 @@
+QA output created by 451
+SCRATCH_MNT/ls_on_scratch
+SCRATCH_MNT/ls_on_scratch
diff --git a/tests/generic/group b/tests/generic/group
index 044ec3f..45f5789 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -453,3 +453,4 @@
 448 auto quick rw
 449 auto quick acl enospc
 450 auto quick rw
+451 auto quick
-- 
2.9.5

Re: [fstests PATCH] generic: add test for executables on read-only DAX mounts

[Christoph Hellwig]

> The above patch fixes an issue with ext4 where executables cannot be run on
> read-only filesystems mounted with the DAX option.
> 
> This issue does not appear to be present in ext2 or XFS, as they both pass
> the test.  I've also confirmed outside of the test that they are both
> indeed able to execute binaries on read-only DAX mounts.

It works for me on XFS.  But I don't really understand why, as the fault
handler doesn't look very different.

Maybe the problem is that in ext4_journal_start_sb will fail on
a read-only fs?

Even for xfs/ext2 it would seem odd that things like sb_start_pagefault
just work. 

> +LS=$(which ls --skip-alias --skip-functions) 2>/dev/null
> +if [ $? -ne 0 ]; then
> +	status=$?
> +	echo "Couldn't find 'ls'!?"
> +	exit
> +fi

These checks all fail for me..

Re: [fstests PATCH] generic: add test for executables on read-only DAX mounts

[Ross Zwisler]

On Wed, Aug 30, 2017 at 07:51:03AM -0700, Christoph Hellwig wrote:
> > The above patch fixes an issue with ext4 where executables cannot be run on
> > read-only filesystems mounted with the DAX option.
> > 
> > This issue does not appear to be present in ext2 or XFS, as they both pass
> > the test.  I've also confirmed outside of the test that they are both
> > indeed able to execute binaries on read-only DAX mounts.
> 
> It works for me on XFS.  But I don't really understand why, as the fault
> handler doesn't look very different.
> 
> Maybe the problem is that in ext4_journal_start_sb will fail on
> a read-only fs?
> 
> Even for xfs/ext2 it would seem odd that things like sb_start_pagefault
> just work. 
> 
> > +LS=$(which ls --skip-alias --skip-functions) 2>/dev/null
> > +if [ $? -ne 0 ]; then
> > +	status=$?
> > +	echo "Couldn't find 'ls'!?"
> > +	exit
> > +fi
> 
> These checks all fail for me..

Huh...really?  I'll send out v2 in a second, but if that fails for you as well
can you try and give me a hint as to what's going wrong with the test in your
setup?

Re: [fstests PATCH] generic: add test for executables on read-only DAX mounts

[Christoph Hellwig]

On Wed, Aug 30, 2017 at 10:02:55PM -0600, Ross Zwisler wrote:
> > > +LS=$(which ls --skip-alias --skip-functions) 2>/dev/null
> > > +if [ $? -ne 0 ]; then
> > > +	status=$?
> > > +	echo "Couldn't find 'ls'!?"
> > > +	exit
> > > +fi
> > 
> > These checks all fail for me..
> 
> Huh...really?  I'll send out v2 in a second, but if that fails for you as well
> can you try and give me a hint as to what's going wrong with the test in your
> setup?

The v2 one works fine for me.