On Tue, Apr 11, 2017 at 10:03:13PM -0600, Ross Zwisler wrote:
> On Mon, Apr 10, 2017 at 02:05:53PM +0800, Xiong Zhou wrote:
> > Regression case that one can write to read-only
> > file in a DAX mountpoint.
> >
> > Signed-off-by: Xiong Zhou <xzhou@xxxxxxxxxx>
> > ---
> >
> > v2:
> > compile test programme manually in this test because default
> > cc option -O2 prevents this issue reproduction;
> > use md5sum instead of seq numbers to check file consistence;
> > umount/check scratch fs after test
> > restore SCRATCH_DEV mode in cleanup
> > fix variable names
> >
> > .gitignore | 1 +
> > src/Makefile | 2 +-
> > src/t_mmap_write_ro.c | 56 +++++++++++++++++++++++++
> > tests/generic/422 | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > tests/generic/422.out | 2 +
> > tests/generic/group | 1 +
> > 6 files changed, 172 insertions(+), 1 deletion(-)
> > create mode 100644 src/t_mmap_write_ro.c
> > create mode 100755 tests/generic/422
> > create mode 100644 tests/generic/422.out
> >
> > diff --git a/.gitignore b/.gitignore
> > index 1ed2a92..ee9329f 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -131,6 +131,7 @@
> > /src/renameat2
> > /src/t_rename_overwrite
> > /src/t_mmap_dio
> > +/src/t_mmap_write_ro
> >
> > # dmapi/ binaries
> > /dmapi/src/common/cmd/read_invis
> > diff --git a/src/Makefile b/src/Makefile
> > index a7f27f0..2604f52 100644
> > --- a/src/Makefile
> > +++ b/src/Makefile
> > @@ -12,7 +12,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
> > godown resvtest writemod makeextents itrash rename \
> > multi_open_unlink dmiperf unwritten_sync genhashnames t_holes \
> > t_mmap_writev t_truncate_cmtime dirhash_collide t_rename_overwrite \
> > - holetest t_truncate_self t_mmap_dio
> > + holetest t_truncate_self t_mmap_dio t_mmap_write_ro
> >
> > LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> > preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> > diff --git a/src/t_mmap_write_ro.c b/src/t_mmap_write_ro.c
> > new file mode 100644
> > index 0000000..cce6e0d
> > --- /dev/null
> > +++ b/src/t_mmap_write_ro.c
> > @@ -0,0 +1,56 @@
> > +#define _GNU_SOURCE
>
> When you're compiling this with 'make' at the top level of xfstests, I think
> _GNU_SOURCE is always defined, so you get the following warning:
>
> t_mmap_write_ro.c:1:0: warning: "_GNU_SOURCE" redefined
> #define _GNU_SOURCE
>
> Now that we're on track to just use the normal compilation via using
> 'volatile' for 'foo', we can just drop this #define.
Okay.
>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +#include <libgen.h>
> > +#include <sys/mman.h>
> > +
> > +int
> > +main(int argc, char **argv)
> > +{
> > + int fd, pfd, ret;
> > + char *buf, foo;
> > + int pagesize = getpagesize();
> > +
> > + if (argc < 2) {
> > + printf("Usage: %s <file> <pmem file>\n", basename(argv[0]));
> > + exit(0);
> > + }
> > +
> > + fd = open(argv[1], O_RDONLY|O_DIRECT);
> > + if (fd < 0) {
> > + perror("open");
>
> For me at least this open() fails because the file we are trying to open with
> O_DIRECT is on /tmp, which is tmpfs, and I don't think tmpfs supports
> O_DIRECT? Running against a copy of the same file that lives on XFS works
> fine:
>
> [ root@alara ~/xfstests ]
> # ./src/t_mmap_write_ro /tmp/30349.largefile ~/dax/data # no O_DIRECT on tmpfs
> open: Invalid argument
> [ root@alara ~/xfstests ]
> # ./src/t_mmap_write_ro ~/30349.largefile ~/dax/data # the read failure is expected
> read: Bad address
> [ root@alara ~/xfstests ]
> # ls -la ~/30349.largefile /tmp/30349.largefile
> -rwxrwxrwx. 1 fsgqa fsgqa 40960 Apr 11 21:09 /root/30349.largefile
> -rwxrwxrwx. 1 fsgqa fsgqa 40960 Apr 11 21:09 /tmp/30349.largefile
>
> The open() also succeeds if I get rid of the O_DIRECT argument.
>
> So, I think we need our O_DIRECT data file to be on something other than
> tmpfs? I don't know the limitations we are under in xfstests - we are
> currently using our scratch device as our memory mode pmem device, and
> mounting it with DAX. In my setup at least the test device also is often
> mounted with DAX...
>
> How can we get a non-DAX file that isn't on tmpfs that we can use for
> O_DIRECT? It only needs to be a page in size...can we throw it into the
> xfstests directory somewhere? Something else?
Remount TEST_DEV wo/ dax for this case, and throw it there.
>
> > + exit(1);
> > + }
> > +
> > + pfd = open(argv[2], O_RDONLY);
> > + if (pfd < 0) {
> > + perror("pmem open");
> > + exit(1);
> > + }
> > +
> > + buf = mmap(NULL, pagesize, PROT_READ, MAP_SHARED, pfd, 0);
> > + if (buf == MAP_FAILED) {
> > + perror("mmap");
> > + exit(1);
> > + }
> > +
> > + /* fault in the page */
> > + foo = *buf;
> > +
> > + ret = read(fd, buf, pagesize);
> > + if (ret != pagesize) {
> > + perror("read");
> > + exit(1);
> > + }
> > +
> > + ret = msync(buf, pagesize, MS_SYNC);
> > + if (ret != 0) {
> > + perror("msync");
> > + exit(1);
> > + }
>
> This msync() isn't needed for the test, and we also get an unused variable
> warning for 'foo'. I'll throw a patch at the end of this mail which fixes
> these and adds some comments from my version of this test.
Aha, I'd prefer keep this msync, and add munmap and close. It doesn't hurt
to test more. :)
>
> > +
> > + exit(0);
> > +}
> > diff --git a/tests/generic/422 b/tests/generic/422
> > new file mode 100755
> > index 0000000..93e10fe
> > --- /dev/null
> > +++ b/tests/generic/422
> > @@ -0,0 +1,111 @@
> > +#! /bin/bash
> > +# FS QA Test 422
> > +#
> > +# This is a regression test for kernel commit
> > +# ef947b2 x86, mm: fix gup_pte_range() vs DAX mappings
> > +# created by Jeffrey Moyer <jmoyer@xxxxxxxxxx>
> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright (c) 2017 Red Hat. 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 -rf $tmp.*
> > + rm -f src/t_mmap_write_ro_no_optim
>
> Yay, we can get rid of all the t_mmap_write_ro_no_optim stuff.
>
> > + # restore SCRATCH_DEV to original mode
> > + $NDCTL_PROG create-namespace -f -e $scratch_dev_namespace \
> > + -m $scratch_dev_mode > /dev/null 2>&1
>
> As with the other test we only want to require NDCTL & JSON and do anything
> with them if we are using PMEM from an NFIT.
I agree with you. Will try.
>
> > +}
> > +
> > +# 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_dax
> > +# We need reconfig SCRATCH_DEV in cleanup, which will
> > +# corrupt the fs on it. fsck manually after tests.
> > +_require_scratch_nocheck
> > +_require_ndctl
> > +_require_jq
> > +_require_test_program "t_mmap_write_ro"
> > +_require_user
> > +
> > +# real QA test starts here
> > +
> > +# config SCRATCH_DEV to memory mode.
> > +
> > +# save original mode
> > +scratch_dev_mode=$(_ndctl_get_pmem_key_value $SCRATCH_DEV mode)
> > +
> > +# get its namespace
> > +scratch_dev_namespace=$(_ndctl_get_pmem_key_value $SCRATCH_DEV dev)
> > +
> > +# notrun if config fails.
> > +$NDCTL_PROG create-namespace -f -e $scratch_dev_namespace \
> > + -m memory > /dev/null 2>&1 || \
> > + _notrun "config $SCRATCH_DEV fail"
> > +
> > +_scratch_mkfs >>$seqres.full 2>&1
> > +_scratch_mount "-o dax"
> > +
> > +# write a 4k read-only file, save its md5sum
> > +$XFS_IO_PROG -f -c "pwrite -S 0xFFFF 0 4096" \
> > + $SCRATCH_MNT/readonlyfile >> $seqres.full 2>&1
> > +chmod 0644 $SCRATCH_MNT/readonlyfile
> > +rofile_md5_1="$(_md5_checksum $SCRATCH_MNT/readonlyfile)"
> > +
> > +# can't reproduce this issue if cc with -O2 option
> > +cc src/t_mmap_write_ro.c -o src/t_mmap_write_ro_no_optim \
> > + >> $seqres.full 2>&1
> > +
> > +# run test programme, read another larger file writing into
> > +# the read-only file with mmap.
> > +_user_do "$XFS_IO_PROG -f -c \"pwrite -S 0x0000 0 40960\" \
> > + ${tmp}.largefile > /dev/null 2>&1"
> > +
> > +# read/write should fail.
> > +_user_do "src/t_mmap_write_ro_no_optim ${tmp}.largefile \
> > + $SCRATCH_MNT/readonlyfile"
> > +
> > +# read-only file should not get updated, md5sum again.
> > +rofile_md5_2="$(_md5_checksum $SCRATCH_MNT/readonlyfile)"
>
> I don't think you need to do anything with the file's md5sum. We know whether
> we've passed or not based on whether the read() call (which was actually
> writing to the DAX mapping) succeeded or failed.
For now, it is true. We can keep this check to win jackpot that read does
fail but file gets changed.
>
> > +
> > +[ $rofile_md5_1 != $rofile_md5_2 ] && echo "read only file changed"
> > +
> > +_scratch_unmount
> > +_check_scratch_fs
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/generic/422.out b/tests/generic/422.out
> > new file mode 100644
> > index 0000000..dc5bca6
> > --- /dev/null
> > +++ b/tests/generic/422.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 422
> > +read: Bad address
> > diff --git a/tests/generic/group b/tests/generic/group
> > index 3c7c5e4..d747385 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -424,3 +424,4 @@
> > 419 auto quick encrypt
> > 420 auto quick punch
> > 421 auto quick encrypt dangerous
> > +422 auto quick
> > --
> > 1.8.3.1
> >
>
> And my patch to clean up a few things and add some comments:
>
> ---
> diff --git a/src/t_mmap_write_ro.c b/src/t_mmap_write_ro.c
> index cce6e0d..61e6f25 100644
> --- a/src/t_mmap_write_ro.c
> +++ b/src/t_mmap_write_ro.c
> @@ -1,4 +1,3 @@
> -#define _GNU_SOURCE
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -11,7 +10,8 @@ int
> main(int argc, char **argv)
> {
> int fd, pfd, ret;
> - char *buf, foo;
> + char *buf;
> + volatile char foo;
> int pagesize = getpagesize();
>
> if (argc < 2) {
> @@ -37,20 +37,24 @@ main(int argc, char **argv)
> exit(1);
> }
>
> - /* fault in the page */
> + /*
> + * Read from the DAX mmap to populate the first page in the
> + * address_space with a with a read-only mapping.
> + */
> foo = *buf;
>
> + /* quiet the compiler's "unused-but-set-variable" warning */
> + foo;
> +
> + /*
> + * Now write to the DAX mmap. This *should* fail, but if the bug is
> + * present in __get_user_pages_fast(), it will succeed.
> + */
Thanks very much!
--
Xiong
> ret = read(fd, buf, pagesize);
> if (ret != pagesize) {
> perror("read");
> exit(1);
> }
>
> - ret = msync(buf, pagesize, MS_SYNC);
> - if (ret != 0) {
> - perror("msync");
> - exit(1);
> - }
> -
> exit(0);
> }
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
