[PATCHSET] fsdax/xfs: unshare range fixes for 6.12

[PATCHSET] fsdax/xfs: unshare range fixes for 6.12

[Darrick J. Wong]

Hi all,

This patchset fixes multiple data corruption bugs in the fallocate unshare
range implementation for fsdax.

With a bit of luck, this should all go splendidly.
Comments and questions are, as always, welcome.

--D
---
Commits in this patchset:
 * xfs: don't allocate COW extents when unsharing a hole
 * iomap: share iomap_unshare_iter predicate code with fsdax
 * fsdax: remove zeroing code from dax_unshare_iter
 * fsdax: dax_unshare_iter needs to copy entire blocks
---
 fs/dax.c               |   49 +++++++++++++++++++++++++++++-------------------
 fs/iomap/buffered-io.c |   30 ++++++++++++++++-------------
 fs/xfs/xfs_iomap.c     |    2 +-
 include/linux/iomap.h  |    1 +
 4 files changed, 48 insertions(+), 34 deletions(-)

[PATCH 1/4] xfs: don't allocate COW extents when unsharing a hole

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

It doesn't make sense to allocate a COW extent when unsharing a hole
because holes cannot be shared.

Fixes: 1f1397b7218d7 ("xfs: don't allocate into the data fork for an unshare request")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_iomap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 1e11f48814c0..dc0443848485 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -707,7 +707,7 @@ imap_needs_cow(
 		return false;
 
 	/* when zeroing we don't have to COW holes or unwritten extents */
-	if (flags & IOMAP_ZERO) {
+	if (flags & (IOMAP_UNSHARE | IOMAP_ZERO)) {
 		if (!nimaps ||
 		    imap->br_startblock == HOLESTARTBLOCK ||
 		    imap->br_state == XFS_EXT_UNWRITTEN)

[PATCH 2/4] iomap: share iomap_unshare_iter predicate code with fsdax

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

The predicate code that iomap_unshare_iter uses to decide if it's really
needs to unshare a file range mapping should be shared with the fsdax
version, because right now they're opencoded and inconsistent.

Note that we simplify the predicate logic a bit -- we no longer allow
unsharing of inline data mappings, but there aren't any filesystems that
allow shared inline data currently.

This is a fix in the sense that it should have been ported to fsdax.

Fixes: b53fdb215d13 ("iomap: improve shared block detection in iomap_unshare_iter")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/dax.c               |    3 +--
 fs/iomap/buffered-io.c |   30 ++++++++++++++++--------------
 include/linux/iomap.h  |    1 +
 3 files changed, 18 insertions(+), 16 deletions(-)


diff --git a/fs/dax.c b/fs/dax.c
index c62acd2812f8..5064eefb1c1e 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1268,8 +1268,7 @@ static s64 dax_unshare_iter(struct iomap_iter *iter)
 	s64 ret = 0;
 	void *daddr = NULL, *saddr = NULL;
 
-	/* don't bother with blocks that are not shared to start with */
-	if (!(iomap->flags & IOMAP_F_SHARED))
+	if (!iomap_want_unshare_iter(iter))
 		return length;
 
 	id = dax_read_lock();
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 78ebd265f425..3899169b2cf7 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1309,19 +1309,12 @@ void iomap_file_buffered_write_punch_delalloc(struct inode *inode,
 }
 EXPORT_SYMBOL_GPL(iomap_file_buffered_write_punch_delalloc);
 
-static loff_t iomap_unshare_iter(struct iomap_iter *iter)
+bool iomap_want_unshare_iter(const struct iomap_iter *iter)
 {
-	struct iomap *iomap = &iter->iomap;
-	loff_t pos = iter->pos;
-	loff_t length = iomap_length(iter);
-	loff_t written = 0;
-
-	/* Don't bother with blocks that are not shared to start with. */
-	if (!(iomap->flags & IOMAP_F_SHARED))
-		return length;
-
 	/*
-	 * Don't bother with delalloc reservations, holes or unwritten extents.
+	 * Don't bother with blocks that are not shared to start with; or
+	 * mappings that cannot be shared, such as inline data, delalloc
+	 * reservations, holes or unwritten extents.
 	 *
 	 * Note that we use srcmap directly instead of iomap_iter_srcmap as
 	 * unsharing requires providing a separate source map, and the presence
@@ -1329,9 +1322,18 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter)
 	 * IOMAP_F_SHARED which can be set for any data that goes into the COW
 	 * fork for XFS.
 	 */
-	if (iter->srcmap.type == IOMAP_HOLE ||
-	    iter->srcmap.type == IOMAP_DELALLOC ||
-	    iter->srcmap.type == IOMAP_UNWRITTEN)
+	return (iter->iomap.flags & IOMAP_F_SHARED) &&
+		iter->srcmap.type == IOMAP_MAPPED;
+}
+
+static loff_t iomap_unshare_iter(struct iomap_iter *iter)
+{
+	struct iomap *iomap = &iter->iomap;
+	loff_t pos = iter->pos;
+	loff_t length = iomap_length(iter);
+	loff_t written = 0;
+
+	if (!iomap_want_unshare_iter(iter))
 		return length;
 
 	do {
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 4ad12a3c8bae..d8a7fc84348c 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -267,6 +267,7 @@ void iomap_invalidate_folio(struct folio *folio, size_t offset, size_t len);
 bool iomap_dirty_folio(struct address_space *mapping, struct folio *folio);
 int iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len,
 		const struct iomap_ops *ops);
+bool iomap_want_unshare_iter(const struct iomap_iter *iter);
 int iomap_zero_range(struct inode *inode, loff_t pos, loff_t len,
 		bool *did_zero, const struct iomap_ops *ops);
 int iomap_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,

[PATCH 3/4] fsdax: remove zeroing code from dax_unshare_iter

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Remove the code in dax_unshare_iter that zeroes the destination memory
because it's not necessary.

If srcmap is unwritten, we don't have to do anything because that
unwritten extent came from the regular file mapping, and unwritten
extents cannot be shared.  The same applies to holes.

Furthermore, zeroing to unshare a mapping is just plain wrong because
unsharing means copy on write, and we should be copying data.

This is effectively a revert of commit 13dd4e04625f ("fsdax: unshare:
zero destination if srcmap is HOLE or UNWRITTEN")

Cc: ruansy.fnst@fujitsu.com
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/dax.c |    8 --------
 1 file changed, 8 deletions(-)


diff --git a/fs/dax.c b/fs/dax.c
index 5064eefb1c1e..9fbbdaa784b4 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1276,14 +1276,6 @@ static s64 dax_unshare_iter(struct iomap_iter *iter)
 	if (ret < 0)
 		goto out_unlock;
 
-	/* zero the distance if srcmap is HOLE or UNWRITTEN */
-	if (srcmap->flags & IOMAP_F_SHARED || srcmap->type == IOMAP_UNWRITTEN) {
-		memset(daddr, 0, length);
-		dax_flush(iomap->dax_dev, daddr, length);
-		ret = length;
-		goto out_unlock;
-	}
-
 	ret = dax_iomap_direct_access(srcmap, pos, length, &saddr, NULL);
 	if (ret < 0)
 		goto out_unlock;

[PATCH 4/4] fsdax: dax_unshare_iter needs to copy entire blocks

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

The code that copies data from srcmap to iomap in dax_unshare_iter is
very very broken, which bfoster's recent fsx changes have exposed.

If the pos and len passed to dax_file_unshare are not aligned to an
fsblock boundary, the iter pos and length in the _iter function will
reflect this unalignment.

dax_iomap_direct_access always returns a pointer to the start of the
kmapped fsdax page, even if its pos argument is in the middle of that
page.  This is catastrophic for data integrity when iter->pos is not
aligned to a page, because daddr/saddr do not point to the same byte in
the file as iter->pos.  Hence we corrupt user data by copying it to the
wrong place.

If iter->pos + iomap_length() in the _iter function not aligned to a
page, then we fail to copy a full block, and only partially populate the
destination block.  This is catastrophic for data confidentiality
because we expose stale pmem contents.

Fix both of these issues by aligning copy_pos/copy_len to a page
boundary (remember, this is fsdax so 1 fsblock == 1 base page) so that
we always copy full blocks.

We're not done yet -- there's no call to invalidate_inode_pages2_range,
so programs that have the file range mmap'd will continue accessing the
old memory mapping after the file metadata updates have completed.

Be careful with the return value -- if the unshare succeeds, we still
need to return the number of bytes that the iomap iter thinks we're
operating on.

Cc: ruansy.fnst@fujitsu.com
Fixes: d984648e428b ("fsdax,xfs: port unshare to fsdax")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/dax.c |   34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)


diff --git a/fs/dax.c b/fs/dax.c
index 9fbbdaa784b4..21b47402b3dc 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1262,26 +1262,46 @@ static s64 dax_unshare_iter(struct iomap_iter *iter)
 {
 	struct iomap *iomap = &iter->iomap;
 	const struct iomap *srcmap = iomap_iter_srcmap(iter);
-	loff_t pos = iter->pos;
-	loff_t length = iomap_length(iter);
+	loff_t copy_pos = iter->pos;
+	u64 copy_len = iomap_length(iter);
+	u32 mod;
 	int id = 0;
 	s64 ret = 0;
 	void *daddr = NULL, *saddr = NULL;
 
 	if (!iomap_want_unshare_iter(iter))
-		return length;
+		return iomap_length(iter);
+
+	/*
+	 * Extend the file range to be aligned to fsblock/pagesize, because
+	 * we need to copy entire blocks, not just the byte range specified.
+	 * Invalidate the mapping because we're about to CoW.
+	 */
+	mod = offset_in_page(copy_pos);
+	if (mod) {
+		copy_len += mod;
+		copy_pos -= mod;
+	}
+
+	mod = offset_in_page(copy_pos + copy_len);
+	if (mod)
+		copy_len += PAGE_SIZE - mod;
+
+	invalidate_inode_pages2_range(iter->inode->i_mapping,
+				      copy_pos >> PAGE_SHIFT,
+				      (copy_pos + copy_len - 1) >> PAGE_SHIFT);
 
 	id = dax_read_lock();
-	ret = dax_iomap_direct_access(iomap, pos, length, &daddr, NULL);
+	ret = dax_iomap_direct_access(iomap, copy_pos, copy_len, &daddr, NULL);
 	if (ret < 0)
 		goto out_unlock;
 
-	ret = dax_iomap_direct_access(srcmap, pos, length, &saddr, NULL);
+	ret = dax_iomap_direct_access(srcmap, copy_pos, copy_len, &saddr, NULL);
 	if (ret < 0)
 		goto out_unlock;
 
-	if (copy_mc_to_kernel(daddr, saddr, length) == 0)
-		ret = length;
+	if (copy_mc_to_kernel(daddr, saddr, copy_len) == 0)
+		ret = iomap_length(iter);
 	else
 		ret = -EIO;
 

Re: [PATCH 1/4] xfs: don't allocate COW extents when unsharing a hole

[Christoph Hellwig]

On Thu, Oct 03, 2024 at 08:09:01AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> It doesn't make sense to allocate a COW extent when unsharing a hole
> because holes cannot be shared.
> 
> Fixes: 1f1397b7218d7 ("xfs: don't allocate into the data fork for an unshare request")

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/4] iomap: share iomap_unshare_iter predicate code with fsdax

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/4] fsdax: remove zeroing code from dax_unshare_iter

[Christoph Hellwig]

On Thu, Oct 03, 2024 at 08:09:32AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Remove the code in dax_unshare_iter that zeroes the destination memory
> because it's not necessary.
> 
> If srcmap is unwritten, we don't have to do anything because that
> unwritten extent came from the regular file mapping, and unwritten
> extents cannot be shared.  The same applies to holes.
> 
> Furthermore, zeroing to unshare a mapping is just plain wrong because
> unsharing means copy on write, and we should be copying data.
> 
> This is effectively a revert of commit 13dd4e04625f ("fsdax: unshare:
> zero destination if srcmap is HOLE or UNWRITTEN")

The original commit claims it fixed a bug, so I'm curious how
that happend and got fixed differently now.  But manually zeroing
data on an unshare does seem very wrong to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/4] fsdax: dax_unshare_iter needs to copy entire blocks

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHSET] fsdax/xfs: unshare range fixes for 6.12

[Christian Brauner]

On Thu, 03 Oct 2024 08:08:55 -0700, Darrick J. Wong wrote:
> This patchset fixes multiple data corruption bugs in the fallocate unshare
> range implementation for fsdax.
> 
> With a bit of luck, this should all go splendidly.
> Comments and questions are, as always, welcome.
> 
> --D
> 
> [...]

Applied to the vfs.iomap branch of the vfs/vfs.git tree.
Patches in the vfs.iomap branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.iomap

[1/4] xfs: don't allocate COW extents when unsharing a hole
      https://git.kernel.org/vfs/vfs/c/b8c4076db5fd
[2/4] iomap: share iomap_unshare_iter predicate code with fsdax
      https://git.kernel.org/vfs/vfs/c/6ef6a0e821d3
[3/4] fsdax: remove zeroing code from dax_unshare_iter
      https://git.kernel.org/vfs/vfs/c/95472274b6fe
[4/4] fsdax: dax_unshare_iter needs to copy entire blocks
      https://git.kernel.org/vfs/vfs/c/50793801fc7f

Re: [PATCHSET] fsdax/xfs: unshare range fixes for 6.12

[Darrick J. Wong]

On Mon, Oct 07, 2024 at 01:52:18PM +0200, Christian Brauner wrote:
> On Thu, 03 Oct 2024 08:08:55 -0700, Darrick J. Wong wrote:
> > This patchset fixes multiple data corruption bugs in the fallocate unshare
> > range implementation for fsdax.
> > 
> > With a bit of luck, this should all go splendidly.
> > Comments and questions are, as always, welcome.
> > 
> > --D
> > 
> > [...]
> 
> Applied to the vfs.iomap branch of the vfs/vfs.git tree.
> Patches in the vfs.iomap branch should appear in linux-next soon.

Er, this has been soaking for 18 days, is it going in soon?
(Apologies, I just saw that you've been under the weather.)

--D

> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
> 
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
> 
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs.iomap
> 
> [1/4] xfs: don't allocate COW extents when unsharing a hole
>       https://git.kernel.org/vfs/vfs/c/b8c4076db5fd
> [2/4] iomap: share iomap_unshare_iter predicate code with fsdax
>       https://git.kernel.org/vfs/vfs/c/6ef6a0e821d3
> [3/4] fsdax: remove zeroing code from dax_unshare_iter
>       https://git.kernel.org/vfs/vfs/c/95472274b6fe
> [4/4] fsdax: dax_unshare_iter needs to copy entire blocks
>       https://git.kernel.org/vfs/vfs/c/50793801fc7f
> 

Re: [PATCHSET] fsdax/xfs: unshare range fixes for 6.12

[Christian Brauner]

On Fri, Oct 25, 2024 at 03:24:44PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 07, 2024 at 01:52:18PM +0200, Christian Brauner wrote:
> > On Thu, 03 Oct 2024 08:08:55 -0700, Darrick J. Wong wrote:
> > > This patchset fixes multiple data corruption bugs in the fallocate unshare
> > > range implementation for fsdax.
> > > 
> > > With a bit of luck, this should all go splendidly.
> > > Comments and questions are, as always, welcome.
> > > 
> > > --D
> > > 
> > > [...]
> > 
> > Applied to the vfs.iomap branch of the vfs/vfs.git tree.
> > Patches in the vfs.iomap branch should appear in linux-next soon.
> 
> Er, this has been soaking for 18 days, is it going in soon?
> (Apologies, I just saw that you've been under the weather.)

Sorry about this. I'll get it out this week.