LinuxLists.cc - [PATCH] Fix ext4 fault handling when mounted with -o dax,ro

[permalink] [raw]

Subject: [PATCH v2] Fix ext4 fault handling when mounted with -o dax,ro

From: Randy Dodgen <[email protected]>

If an ext4 filesystem is mounted with both the DAX and read-only
options, executables on that filesystem will fail to start (claiming
'Segmentation fault') due to the fault handler returning
VM_FAULT_SIGBUS.

This is due to the DAX fault handler (see ext4_dax_huge_fault)
attempting to write to the journal when FAULT_FLAG_WRITE is set. This is
the wrong behavior for write faults which will lead to a COW page; in
particular, this fails for readonly mounts.

This changes replicates some check from dax_iomap_fault to more
precisely reason about when a journal-write is needed.

It might be the case that this could be better handled in
ext4_iomap_begin / ext4_iomap_end (called via iomap_ops inside
dax_iomap_fault). These is some overlap already (e.g. grabbing journal
handles).

Signed-off-by: Randy Dodgen <[email protected]>
---

I'm resending for some DMARC-proofing (thanks Ted for the explanation), a
missing Signed-off-by, and some extra cc's. Oops!

fs/ext4/file.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 0d7cf0cc9b87..d512fb85a3e3 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -279,7 +279,31 @@ static int ext4_dax_huge_fault(struct vm_fault *vmf,
handle_t *handle = NULL;
struct inode *inode = file_inode(vmf->vma->vm_file);
struct super_block *sb = inode->i_sb;
- bool write = vmf->flags & FAULT_FLAG_WRITE;
+ bool write;
+
+ /*
+ * We have to distinguish real writes from writes which will result in a
+ * COW page
+ * - COW writes need to fall-back to installing PTEs. See
+ * dax_iomap_pmd_fault.
+ * - COW writes should *not* poke the journal (the file will not be
+ * changed). Doing so would cause unintended failures when mounted
+ * read-only.
+ */
+ if (pe_size == PE_SIZE_PTE) {
+ /* See dax_iomap_pte_fault. */
+ write = (vmf->flags & FAULT_FLAG_WRITE) && !vmf->cow_page;
+ } else if (pe_size == PE_SIZE_PMD) {
+ /* See dax_iomap_pmd_fault. */
+ write = vmf->flags & FAULT_FLAG_WRITE;
+ if (write && !(vmf->vma->vm_flags & VM_SHARED)) {
+ split_huge_pmd(vmf->vma, vmf->pmd, vmf->address);
+ count_vm_event(THP_FAULT_FALLBACK);
+ return VM_FAULT_FALLBACK;
+ }
+ } else {
+ return VM_FAULT_FALLBACK;
+ }

if (write) {
sb_start_pagefault(sb);
--
2.14.1.480.gb18f417b89-goog

2017-08-23 16:38:26

[permalink] [raw]

Subject: Re: [PATCH v2] Fix ext4 fault handling when mounted with -o dax,ro

On Tue, Aug 22, 2017 at 08:37:04PM -0700, [email protected] wrote:
> From: Randy Dodgen <[email protected]>
>
> If an ext4 filesystem is mounted with both the DAX and read-only
> options, executables on that filesystem will fail to start (claiming
> 'Segmentation fault') due to the fault handler returning
> VM_FAULT_SIGBUS.
>
> This is due to the DAX fault handler (see ext4_dax_huge_fault)
> attempting to write to the journal when FAULT_FLAG_WRITE is set. This is
> the wrong behavior for write faults which will lead to a COW page; in
> particular, this fails for readonly mounts.
>
> This changes replicates some check from dax_iomap_fault to more
> precisely reason about when a journal-write is needed.
>
> It might be the case that this could be better handled in
> ext4_iomap_begin / ext4_iomap_end (called via iomap_ops inside
> dax_iomap_fault). These is some overlap already (e.g. grabbing journal
> handles).
>
> Signed-off-by: Randy Dodgen <[email protected]>
> ---
>
> I'm resending for some DMARC-proofing (thanks Ted for the explanation), a
> missing Signed-off-by, and some extra cc's. Oops!
>
> fs/ext4/file.c | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 0d7cf0cc9b87..d512fb85a3e3 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -279,7 +279,31 @@ static int ext4_dax_huge_fault(struct vm_fault *vmf,
> handle_t *handle = NULL;
> struct inode *inode = file_inode(vmf->vma->vm_file);
> struct super_block *sb = inode->i_sb;
> - bool write = vmf->flags & FAULT_FLAG_WRITE;
> + bool write;
> +
> + /*
> + * We have to distinguish real writes from writes which will result in a
> + * COW page
> + * - COW writes need to fall-back to installing PTEs. See
> + * dax_iomap_pmd_fault.
> + * - COW writes should *not* poke the journal (the file will not be
> + * changed). Doing so would cause unintended failures when mounted
> + * read-only.
> + */
> + if (pe_size == PE_SIZE_PTE) {
> + /* See dax_iomap_pte_fault. */
> + write = (vmf->flags & FAULT_FLAG_WRITE) && !vmf->cow_page;
> + } else if (pe_size == PE_SIZE_PMD) {
> + /* See dax_iomap_pmd_fault. */
> + write = vmf->flags & FAULT_FLAG_WRITE;
> + if (write && !(vmf->vma->vm_flags & VM_SHARED)) {
> + split_huge_pmd(vmf->vma, vmf->pmd, vmf->address);
> + count_vm_event(THP_FAULT_FALLBACK);
> + return VM_FAULT_FALLBACK;
> + }
> + } else {
> + return VM_FAULT_FALLBACK;
> + }

This works in my setup, though the logic could be simpler.

For all fault sizes you can rely on the fact that a COW write will happen when
we have FAULT_FLAG_WRITE but not VM_SHARED. This is the logic that we use to
know to set up vmf->cow_page() in do_fault() by calling do_cow_fault(), and in
finish_fault().

I think your test can then just become:

write = (vmf->flags & FAULT_FLAG_WRITE) &&
(vmf->vma->vm_flags & VM_SHARED);

With some appropriate commenting.

You can then let the DAX fault handlers worry about validating the fault size
and splitting the PMD on fallback.

I'll let someone with more ext4-fu comment on whether it is okay to skip the
journal entry when doing a COW fault. This must be handled in ext4 for the
non-DAX case, but I don't see any more checks for VM_SHARED or
FAULT_FLAG_WRITE in fs/ext4, so maybe there is a better way?

- Ross

2017-08-23 20:11:23

[permalink] [raw]

Subject: Re: [PATCH v2] Fix ext4 fault handling when mounted with -o dax,ro

That's a nice simplification. I started cautiously by replicating the same
checks for dax.c (dax_iomap_pte_fault checks for cow_page specifically). I
recall that it used to be possible for COW pages to appear in VM_SHARED
mappings, but I'm glad to see that went away in cda540ace6a19. I'll send a new
version today.

One potential advantage hiding in the more complicated checks is that we avoid
repeatedly grabbing the journal as we fallback from PUD -> PMD or PUD -> PMD ->
PTE (see __handle_mm_fault and VM_FAULT_FALLBACK checks). I will defer to the
ext4 folks w.r.t. that being worthwhile; if so, there will need to be some
thought on how to tweak the new .huge_fault protocol, or how to move the
journal bits after the dax_iomap_fault fallbacks (maybe in ext4_iomap_{begin,
end}?)

Regarding ext4's behavior in the non-DAX case, note that those vm_ops don't
have a .huge_fault handler, and .fault delegates to filemap_fault (which as you
mention doesn't care about FAULT_FLAG_WRITE etc). Ignoring .huge_fault, we can
assume that .page_mkwrite will be called at just the right times (e.g. as part
of do_shared_fault but not do_cow_fault).

Meanwhile, implementing .huge_fault is much trickier; there is no
".huge_mkwrite" (so some prediction of COW is needed, as here) and one must
remember to split huge entries before returning VM_FAULT_FALLBACK (see 59bf4fb9
; not doing so in __dax_pmd_fault was resulting in repeated PMD faults not
making progress). Maybe there is room to improve this.

On Wed, Aug 23, 2017 at 9:38 AM, Ross Zwisler
<[email protected]> wrote:
> On Tue, Aug 22, 2017 at 08:37:04PM -0700, [email protected] wrote:
>> From: Randy Dodgen <[email protected]>
>>
>> If an ext4 filesystem is mounted with both the DAX and read-only
>> options, executables on that filesystem will fail to start (claiming
>> 'Segmentation fault') due to the fault handler returning
>> VM_FAULT_SIGBUS.
>>
>> This is due to the DAX fault handler (see ext4_dax_huge_fault)
>> attempting to write to the journal when FAULT_FLAG_WRITE is set. This is
>> the wrong behavior for write faults which will lead to a COW page; in
>> particular, this fails for readonly mounts.
>>
>> This changes replicates some check from dax_iomap_fault to more
>> precisely reason about when a journal-write is needed.
>>
>> It might be the case that this could be better handled in
>> ext4_iomap_begin / ext4_iomap_end (called via iomap_ops inside
>> dax_iomap_fault). These is some overlap already (e.g. grabbing journal
>> handles).
>>
>> Signed-off-by: Randy Dodgen <[email protected]>
>> ---
>>
>> I'm resending for some DMARC-proofing (thanks Ted for the explanation), a
>> missing Signed-off-by, and some extra cc's. Oops!
>>
>> fs/ext4/file.c | 26 +++++++++++++++++++++++++-
>> 1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
>> index 0d7cf0cc9b87..d512fb85a3e3 100644
>> --- a/fs/ext4/file.c
>> +++ b/fs/ext4/file.c
>> @@ -279,7 +279,31 @@ static int ext4_dax_huge_fault(struct vm_fault *vmf,
>> handle_t *handle = NULL;
>> struct inode *inode = file_inode(vmf->vma->vm_file);
>> struct super_block *sb = inode->i_sb;
>> - bool write = vmf->flags & FAULT_FLAG_WRITE;
>> + bool write;
>> +
>> + /*
>> + * We have to distinguish real writes from writes which will result in a
>> + * COW page
>> + * - COW writes need to fall-back to installing PTEs. See
>> + * dax_iomap_pmd_fault.
>> + * - COW writes should *not* poke the journal (the file will not be
>> + * changed). Doing so would cause unintended failures when mounted
>> + * read-only.
>> + */
>> + if (pe_size == PE_SIZE_PTE) {
>> + /* See dax_iomap_pte_fault. */
>> + write = (vmf->flags & FAULT_FLAG_WRITE) && !vmf->cow_page;
>> + } else if (pe_size == PE_SIZE_PMD) {
>> + /* See dax_iomap_pmd_fault. */
>> + write = vmf->flags & FAULT_FLAG_WRITE;
>> + if (write && !(vmf->vma->vm_flags & VM_SHARED)) {
>> + split_huge_pmd(vmf->vma, vmf->pmd, vmf->address);
>> + count_vm_event(THP_FAULT_FALLBACK);
>> + return VM_FAULT_FALLBACK;
>> + }
>> + } else {
>> + return VM_FAULT_FALLBACK;
>> + }
>
> This works in my setup, though the logic could be simpler.
>
> For all fault sizes you can rely on the fact that a COW write will happen when
> we have FAULT_FLAG_WRITE but not VM_SHARED. This is the logic that we use to
> know to set up vmf->cow_page() in do_fault() by calling do_cow_fault(), and in
> finish_fault().
>
> I think your test can then just become:
>
> write = (vmf->flags & FAULT_FLAG_WRITE) &&
> (vmf->vma->vm_flags & VM_SHARED);
>
> With some appropriate commenting.
>
> You can then let the DAX fault handlers worry about validating the fault size
> and splitting the PMD on fallback.
>
> I'll let someone with more ext4-fu comment on whether it is okay to skip the
> journal entry when doing a COW fault. This must be handled in ext4 for the
> non-DAX case, but I don't see any more checks for VM_SHARED or
> FAULT_FLAG_WRITE in fs/ext4, so maybe there is a better way?
>
> - Ross

2017-08-23 21:27:18

[permalink] [raw]

Subject: [PATCH v3] Fix ext4 fault handling when mounted with -o dax,ro

From: Randy Dodgen <[email protected]>

If an ext4 filesystem is mounted with both the DAX and read-only
options, executables on that filesystem will fail to start (claiming
'Segmentation fault') due to the fault handler returning
VM_FAULT_SIGBUS.

This is due to the DAX fault handler (see ext4_dax_huge_fault)
attempting to write to the journal when FAULT_FLAG_WRITE is set. This is
the wrong behavior for write faults which will lead to a COW page; in
particular, this fails for readonly mounts.

This change avoids journal writes for faults that are expected to COW.

It might be the case that this could be better handled in
ext4_iomap_begin / ext4_iomap_end (called via iomap_ops inside
dax_iomap_fault). These is some overlap already (e.g. grabbing journal
handles).

Signed-off-by: Randy Dodgen <[email protected]>
---

This version is simplified as suggested by Ross; all fault sizes and fallbacks
are handled by dax_iomap_fault.

fs/ext4/file.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 0d7cf0cc9b87..dc1e1fb6b54c 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -279,7 +279,20 @@ static int ext4_dax_huge_fault(struct vm_fault *vmf,
handle_t *handle = NULL;
struct inode *inode = file_inode(vmf->vma->vm_file);
struct super_block *sb = inode->i_sb;
- bool write = vmf->flags & FAULT_FLAG_WRITE;
+
+ /*
+ * We have to distinguish real writes from writes which will result in a
+ * COW page; COW writes should *not* poke the journal (the file will not
+ * be changed). Doing so would cause unintended failures when mounted
+ * read-only.
+ *
+ * We check for VM_SHARED rather than vmf->cow_page since the latter is
+ * unset for pe_size != PE_SIZE_PTE (i.e. only in do_cow_fault); for
+ * other sizes, dax_iomap_fault will handle splitting / fallback so that
+ * we eventually come back with a COW page.
+ */
+ bool write = (vmf->flags & FAULT_FLAG_WRITE) &&
+ (vmf->vma->vm_flags & VM_SHARED);

if (write) {
sb_start_pagefault(sb);
--
2.14.1.342.g6490525c54-goog

2017-08-24 14:45:44

by Jan Kara

[permalink] [raw]

Subject: Re: [PATCH v3] Fix ext4 fault handling when mounted with -o dax,ro

On Wed 23-08-17 14:26:52, [email protected] wrote:
> From: Randy Dodgen <[email protected]>
>
> If an ext4 filesystem is mounted with both the DAX and read-only
> options, executables on that filesystem will fail to start (claiming
> 'Segmentation fault') due to the fault handler returning
> VM_FAULT_SIGBUS.
>
> This is due to the DAX fault handler (see ext4_dax_huge_fault)
> attempting to write to the journal when FAULT_FLAG_WRITE is set. This is
> the wrong behavior for write faults which will lead to a COW page; in
> particular, this fails for readonly mounts.
>
> This change avoids journal writes for faults that are expected to COW.
>
> It might be the case that this could be better handled in
> ext4_iomap_begin / ext4_iomap_end (called via iomap_ops inside
> dax_iomap_fault). These is some overlap already (e.g. grabbing journal
> handles).
>
> Signed-off-by: Randy Dodgen <[email protected]>

Thanks for the verbose comment :). The patch looks good to me. You can add:

Reviewed-by: Jan Kara <[email protected]>

Honza

> ---
>
> This version is simplified as suggested by Ross; all fault sizes and fallbacks
> are handled by dax_iomap_fault.
>
> fs/ext4/file.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 0d7cf0cc9b87..dc1e1fb6b54c 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -279,7 +279,20 @@ static int ext4_dax_huge_fault(struct vm_fault *vmf,
> handle_t *handle = NULL;
> struct inode *inode = file_inode(vmf->vma->vm_file);
> struct super_block *sb = inode->i_sb;
> - bool write = vmf->flags & FAULT_FLAG_WRITE;
> +
> + /*
> + * We have to distinguish real writes from writes which will result in a
> + * COW page; COW writes should *not* poke the journal (the file will not
> + * be changed). Doing so would cause unintended failures when mounted
> + * read-only.
> + *
> + * We check for VM_SHARED rather than vmf->cow_page since the latter is
> + * unset for pe_size != PE_SIZE_PTE (i.e. only in do_cow_fault); for
> + * other sizes, dax_iomap_fault will handle splitting / fallback so that
> + * we eventually come back with a COW page.
> + */
> + bool write = (vmf->flags & FAULT_FLAG_WRITE) &&
> + (vmf->vma->vm_flags & VM_SHARED);
>
> if (write) {
> sb_start_pagefault(sb);
> --
> 2.14.1.342.g6490525c54-goog
>
--
Jan Kara <jack-IBi9RG/[email protected]>
SUSE Labs, CR

2017-08-24 15:11:38

[permalink] [raw]

Subject: Re: [PATCH v3] Fix ext4 fault handling when mounted with -o dax,ro

2017-08-24 16:01:44

[permalink] [raw]

Subject: Re: [PATCH v3] Fix ext4 fault handling when mounted with -o dax,ro

On Wed, Aug 23, 2017 at 02:26:52PM -0700, [email protected] wrote:
> From: Randy Dodgen <[email protected]>
>
> If an ext4 filesystem is mounted with both the DAX and read-only
> options, executables on that filesystem will fail to start (claiming
> 'Segmentation fault') due to the fault handler returning
> VM_FAULT_SIGBUS.
>
> This is due to the DAX fault handler (see ext4_dax_huge_fault)
> attempting to write to the journal when FAULT_FLAG_WRITE is set. This is
> the wrong behavior for write faults which will lead to a COW page; in
> particular, this fails for readonly mounts.
>
> This change avoids journal writes for faults that are expected to COW.
>
> It might be the case that this could be better handled in
> ext4_iomap_begin / ext4_iomap_end (called via iomap_ops inside
> dax_iomap_fault). These is some overlap already (e.g. grabbing journal
> handles).
>
> Signed-off-by: Randy Dodgen <[email protected]>
> ---
>
> This version is simplified as suggested by Ross; all fault sizes and fallbacks
> are handled by dax_iomap_fault.

We really need to do the same for ext2 and xfs. And we really should
be doing that in common VM code instead of the file system. See
my recent xfs synchronous page fault series on the mess the inconsistent
handling of the FAULT_FLAG_WRITE causes. I think we just need a new
FAULT_FLAG_ALLOC or similar for those page faults that needs to
allocate space instead of piling hacks over hacks, and make sure
it's only set over the method that will actually do the allocation,
as the DAX and non-DAX path are not consistent on that.

Also any chance you could write an xfstest for this?

2017-08-24 19:26:52

by Theodore Y. Ts'o

[permalink] [raw]

Subject: Re: [PATCH v3] Fix ext4 fault handling when mounted with -o dax,ro

2017-08-24 20:57:27

by Theodore Y. Ts'o

[permalink] [raw]

Subject: Re: [PATCH v3] Fix ext4 fault handling when mounted with -o dax,ro

On Thu, Aug 24, 2017 at 09:01:44AM -0700, Christoph Hellwig wrote:
>
> We really need to do the same for ext2 and xfs. And we really should
> be doing that in common VM code instead of the file system. See
> my recent xfs synchronous page fault series on the mess the inconsistent
> handling of the FAULT_FLAG_WRITE causes. I think we just need a new
> FAULT_FLAG_ALLOC or similar for those page faults that needs to
> allocate space instead of piling hacks over hacks, and make sure
> it's only set over the method that will actually do the allocation,
> as the DAX and non-DAX path are not consistent on that.

Yeah, agreed, that's the cleaner way of doing that. It does mean
we'll have sweep through all of the file systems so that they DTRT
with this new FAULT_FLAG_ALLOC, though, right?

- Ted

2017-08-25 07:28:49

[permalink] [raw]

Subject: Re: [PATCH v3] Fix ext4 fault handling when mounted with -o dax,ro

On Thu, Aug 24, 2017 at 04:57:27PM -0400, Theodore Ts'o wrote:
> Yeah, agreed, that's the cleaner way of doing that. It does mean
> we'll have sweep through all of the file systems so that they DTRT
> with this new FAULT_FLAG_ALLOC, though, right?

I think the only ones that it matters for for now are the DAX
fault handlers. So we can add the new flag, check it in ext2, ext4
and xfs for now and we're done. But if we eventually want to
phase out the ->page_mkwrite handler it would be useful for other
file systems as well.

2017-08-29 21:20:04

[permalink] [raw]

Subject: Re: [PATCH v3] Fix ext4 fault handling when mounted with -o dax,ro

Randy, any chance yto at least share a test script so that others
can wire it up for the test suite?

2017-08-29 21:37:21

[permalink] [raw]

Subject: Re: [PATCH v3] Fix ext4 fault handling when mounted with -o dax,ro

On Tue, Aug 29, 2017 at 02:20:02PM -0700, Christoph Hellwig wrote:
> Randy, any chance yto at least share a test script so that others
> can wire it up for the test suite?

I made a reproducer for my testing. I'll make an xfstest if Randy isn't able
to.

2017-08-29 22:07:08

[permalink] [raw]

Subject: Re: [PATCH v3] Fix ext4 fault handling when mounted with -o dax,ro

An xfstest is a fine idea. I've started some work on that.

On Tue, Aug 29, 2017 at 2:37 PM, Ross Zwisler
<ross.zwisler-VuQAYsv1563Yd54FQh9/[email protected]> wrote:
> On Tue, Aug 29, 2017 at 02:20:02PM -0700, Christoph Hellwig wrote:
>> Randy, any chance yto at least share a test script so that others
>> can wire it up for the test suite?
>
> I made a reproducer for my testing. I'll make an xfstest if Randy isn't able
> to.

2017-08-29 22:37:15

[permalink] [raw]

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

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 <[email protected]>
Cc: Randy Dodgen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Theodore Ts'o <[email protected]>

---

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 <[email protected]>
+#
+#-----------------------------------------------------------------------
+# 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

2017-08-30 10:59:30

by Eryu Guan

[permalink] [raw]

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

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 <[email protected]>
> Cc: Randy Dodgen <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Theodore Ts'o <[email protected]>
>
> ---
>
> 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 <[email protected]>
> +#
> +#-----------------------------------------------------------------------
> +# 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2017-08-30 14:51:03

[permalink] [raw]

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

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

2017-08-31 04:01:46

[permalink] [raw]

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

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-VuQAYsv1563Yd54FQh9/[email protected]>
> > Cc: Randy Dodgen <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Theodore Ts'o <[email protected]>
> >
> > ---
> >
> > 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-VuQAYsv1563Yd54FQh9/[email protected]>
> > +#
> > +#-----------------------------------------------------------------------
> > +# 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.

2017-08-31 04:02:55

[permalink] [raw]

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

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?

2017-08-31 04:09:10

[permalink] [raw]

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

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 <[email protected]>
Cc: Randy Dodgen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Theodore Ts'o <[email protected]>

---

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 <[email protected]>
+#
+#-----------------------------------------------------------------------
+# 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

2017-08-31 13:09:33