FAILED: patch "[PATCH] dax: fix PMD data corruption when fault races with write" failed to apply to 4.10-stable tree

PATCH [email protected] 2 participants 3 messages May 18, 2017 – May 23, 2017

Subsystems (3)

Stable branch VFS Filesystem direct access (DAX)

lore

Share & tools

lore

[email protected]

author maintainer

2017-05-18 07:26 UTC #0 lore raw

The patch below does not apply to the 4.10-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

Quoted 1 line

From 876f29460cbd4086b43475890c1bf2488fa11d40 Mon Sep 17 00:00:00 2001

From: Ross Zwisler <[email protected]>
Date: Fri, 12 May 2017 15:47:00 -0700
Subject: [PATCH] dax: fix PMD data corruption when fault races with write

This is based on a patch from Jan Kara that fixed the equivalent race in the DAX PTE fault path.

Currently DAX PMD read fault can race with write(2) in the following way:

CPU1 - write(2) CPU2 - read fault

                                dax_iomap_pmd_fault()
                                  ->iomap_begin() - sees hole

dax_iomap_rw()
iomap_apply()
->iomap_begin - allocates blocks
dax_iomap_actor()
invalidate_inode_pages2_range()
- there's nothing to invalidate

grab_mapping_entry()
- we add huge zero page to the radix tree and map it to page tables

The result is that hole page is mapped into page tables (and thus zeros are seen in mmap) while file has data written in that place.

Fix the problem by locking exception entry before mapping blocks for the fault. That way we are sure invalidate_inode_pages2_range() call for racing write will either block on entry lock waiting for the fault to finish (and unmap stale page tables after that) or read fault will see already allocated blocks by write(2).

Fixes 9f141d6ef6258 ("dax: Call ->iomap_begin without entry lock during dax fault")

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ross Zwisler <[email protected]>

Reviewed-by Jan Kara <[email protected]>

Cc Dan Williams <[email protected]>

Cc <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

fs/dax.c +14 ~~-14~~

index 32f020c9cedf..93ae87297ffa 100644
@@ -1388,6 +1388,16 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf,
 		goto fallback;
 
 	/*
+	 * grab_mapping_entry() will make sure we get a 2M empty entry, a DAX
+	 * PMD or a HZP entry.  If it can't (because a 4k page is already in
+	 * the tree, for instance), it will return -EEXIST and we just fall
+	 * back to 4k entries.
+	 */
+	entry = grab_mapping_entry(mapping, pgoff, RADIX_DAX_PMD);
+	if (IS_ERR(entry))
+		goto fallback;
+
+	/*
 	 * Note that we don't use iomap_apply here.  We aren't doing I/O, only
 	 * setting up a mapping, so really we're using iomap_begin() as a way
 	 * to look up our filesystem block.

@@ -1395,21 +1405,11 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf,
 	pos = (loff_t)pgoff << PAGE_SHIFT;
 	error = ops->iomap_begin(inode, pos, PMD_SIZE, iomap_flags, &iomap);
 	if (error)
-		goto fallback;
+		goto unlock_entry;
 
 	if (iomap.offset + iomap.length < pos + PMD_SIZE)
 		goto finish_iomap;
 
-	/*
-	 * grab_mapping_entry() will make sure we get a 2M empty entry, a DAX
-	 * PMD or a HZP entry.  If it can't (because a 4k page is already in
-	 * the tree, for instance), it will return -EEXIST and we just fall
-	 * back to 4k entries.
-	 */
-	entry = grab_mapping_entry(mapping, pgoff, RADIX_DAX_PMD);
-	if (IS_ERR(entry))
-		goto finish_iomap;
-
 	switch (iomap.type) {
 	case IOMAP_MAPPED:
 		result = dax_pmd_insert_mapping(vmf, &iomap, pos, &entry);

@@ -1417,7 +1417,7 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf,
 	case IOMAP_UNWRITTEN:
 	case IOMAP_HOLE:
 		if (WARN_ON_ONCE(write))
-			goto unlock_entry;
+			break;
 		result = dax_pmd_load_hole(vmf, &iomap, &entry);
 		break;
 	default:

@@ -1425,8 +1425,6 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf,
 		break;
 	}
 
- unlock_entry:
-	put_locked_mapping_entry(mapping, pgoff, entry);
  finish_iomap:
 	if (ops->iomap_end) {
 		int copied = PMD_SIZE;

@@ -1442,6 +1440,8 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf,
 		ops->iomap_end(inode, pos, PMD_SIZE, copied, iomap_flags,
 				&iomap);
 	}
+ unlock_entry:
+	put_locked_mapping_entry(mapping, pgoff, entry);
  fallback:
 	if (result == VM_FAULT_FALLBACK) {
 		split_huge_pmd(vma, vmf->pmd, vmf->address);

Ross Zwisler

Reviewed-by Jan Kara

2017-05-19 19:54 UTC #1 lore raw

Quoted 9 lines · On Thu, May 18, 2017 at 09:26:31AM +0200, [email protected] wrote:

The patch below does not apply to the 4.10-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
thanks,
greg k-h

The patch below is a backport of:

commit 876f29460cbd ("dax: fix PMD data corruption when fault races with write")

to v4.10.16. This was patch 5 in the series.

This patch still needs a backport of the following ext4 commit (which was patch 3 in the series) from Jan to work correctly:

commit fb26a1cbed8c ("ext4: return to starting transaction in ext4_dax_huge_fault()")

--- >8 ---

Quoted 1 line

From eddc74225a0414af64ac59c09a44b9dae23b16b8 Mon Sep 17 00:00:00 2001

From: Ross Zwisler <[email protected]>
Date: Fri, 12 May 2017 15:47:00 -0700
Subject: [PATCH] dax: fix PMD data corruption when fault races with write

This is based on a patch from Jan Kara that fixed the equivalent race in the DAX PTE fault path.

Currently DAX PMD read fault can race with write(2) in the following way:

CPU1 - write(2) CPU2 - read fault

                                dax_iomap_pmd_fault()
                                  ->iomap_begin() - sees hole

dax_iomap_rw()
iomap_apply()
->iomap_begin - allocates blocks
dax_iomap_actor()
invalidate_inode_pages2_range()
- there's nothing to invalidate

grab_mapping_entry()
- we add huge zero page to the radix tree and map it to page tables

The result is that hole page is mapped into page tables (and thus zeros are seen in mmap) while file has data written in that place.

Fixes 9f141d6ef6258 ("dax: Call ->iomap_begin without entry lock during dax fault")

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ross Zwisler <[email protected]>

Reviewed-by Jan Kara <[email protected]>

Cc Dan Williams <[email protected]>

Cc <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

---
 fs/dax.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

fs/dax.c +14 ~~-13~~

index c83a0d8..c69ad4a 100644
@@ -1343,6 +1343,16 @@ int dax_iomap_pmd_fault(struct vm_area_struct *vma, unsigned long address,
 		goto fallback;
 
 	/*
+	 * grab_mapping_entry() will make sure we get a 2M empty entry, a DAX
+	 * PMD or a HZP entry.  If it can't (because a 4k page is already in
+	 * the tree, for instance), it will return -EEXIST and we just fall
+	 * back to 4k entries.
+	 */
+	entry = grab_mapping_entry(mapping, pgoff, RADIX_DAX_PMD);
+	if (IS_ERR(entry))
+		goto fallback;
+
+	/*
 	 * Note that we don't use iomap_apply here.  We aren't doing I/O, only
 	 * setting up a mapping, so really we're using iomap_begin() as a way
 	 * to look up our filesystem block.

@@ -1350,20 +1360,11 @@ int dax_iomap_pmd_fault(struct vm_area_struct *vma, unsigned long address,
 	pos = (loff_t)pgoff << PAGE_SHIFT;
 	error = ops->iomap_begin(inode, pos, PMD_SIZE, iomap_flags, &iomap);
 	if (error)
-		goto fallback;
+		goto unlock_entry;
 
 	if (iomap.offset + iomap.length < pos + PMD_SIZE)
 		goto finish_iomap;
 
-	/*
-	 * grab_mapping_entry() will make sure we get a 2M empty entry, a DAX
-	 * PMD or a HZP entry.  If it can't (because a 4k page is already in
-	 * the tree, for instance), it will return -EEXIST and we just fall
-	 * back to 4k entries.
-	 */
-	entry = grab_mapping_entry(mapping, pgoff, RADIX_DAX_PMD);
-	if (IS_ERR(entry))
-		goto finish_iomap;
 
 	vmf.pgoff = pgoff;
 	vmf.flags = flags;

@@ -1377,7 +1378,7 @@ int dax_iomap_pmd_fault(struct vm_area_struct *vma, unsigned long address,
 	case IOMAP_UNWRITTEN:
 	case IOMAP_HOLE:
 		if (WARN_ON_ONCE(write))
-			goto unlock_entry;
+			break;
 		result = dax_pmd_load_hole(vma, pmd, &vmf, address, &iomap,
 				&entry);
 		break;

@@ -1386,8 +1387,6 @@ int dax_iomap_pmd_fault(struct vm_area_struct *vma, unsigned long address,
 		break;
 	}
 
- unlock_entry:
-	put_locked_mapping_entry(mapping, pgoff, entry);
  finish_iomap:
 	if (ops->iomap_end) {
 		int copied = PMD_SIZE;

@@ -1403,6 +1402,8 @@ int dax_iomap_pmd_fault(struct vm_area_struct *vma, unsigned long address,
 		ops->iomap_end(inode, pos, PMD_SIZE, copied, iomap_flags,
 				&iomap);
 	}
+ unlock_entry:
+	put_locked_mapping_entry(mapping, pgoff, entry);
  fallback:
 	if (result == VM_FAULT_FALLBACK) {
 		split_huge_pmd(vma, pmd, address);

Signature · 1 line

2.9.4

Greg KH

author maintainer

2017-05-23 10:26 UTC #2 lore raw

Quoted 13 lines · Ross Zwisler

Quoted 9 lines · On Thu, May 18, 2017 at 09:26:31AM +0200, [email protected] wrote:
The patch below does not apply to the 4.10-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
thanks,
greg k-h
The patch below is a backport of:

Quoted 8 lines

commit 876f29460cbd ("dax: fix PMD data corruption when fault races with write")
to v4.10.16. This was patch 5 in the series.
This patch still needs a backport of the following ext4 commit (which was patch 3 in the series) from Jan to work correctly:
commit fb26a1cbed8c ("ext4: return to starting transaction in ext4_dax_huge_fault()")

Thanks for this, but 4.10 is now end-of-life, so I'm not going to worry about the ext4 patch :)

greg k-h