linux-kernel - [RFC PATCH] xfs: check shared state of when CoW, update reflink flag when io ends

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <1679025588-21-1-git-send-email-ruansy.fnst@fujitsu.com>
Date:   Fri, 17 Mar 2023 03:59:48 +0000
From:   Shiyang Ruan <ruansy.fnst@...itsu.com>
To:     <linux-xfs@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>,
        <nvdimm@...ts.linux.dev>, <linux-kernel@...r.kernel.org>
CC:     <djwong@...nel.org>, <david@...morbit.com>,
        <dan.j.williams@...el.com>, <akpm@...ux-foundation.org>
Subject: [RFC PATCH] xfs: check shared state of when CoW, update reflink flag when io ends

As is mentioned[1] before, the generic/388 will randomly fail with dmesg
warning.  This case uses fsstress with a lot of random operations.  It is hard
to  reproduce.  Finally I found a 100% reproduce condition, which is setting
the seed to 1677104360.  So I changed the generic/388 code: removed the loop
and used the code below instad:
```
($FSSTRESS_PROG $FSSTRESS_AVOID -d $SCRATCH_MNT -v -s 1677104360 -n 221 -p 1 >> $seqres.full) > /dev/null 2>&1
($FSSTRESS_PROG $FSSTRESS_AVOID -d $SCRATCH_MNT -v -s 1677104360 -n 221 -p 1 >> $seqres.full) > /dev/null 2>&1
_check_dmesg_for dax_insert_entry
```

According to the operations log, and kernel debug log I added, I found that
the reflink flag of one inode won't be unset even if there's no more shared
extents any more.
  Then write to this file again.  Because of the reflink flag, xfs thinks it
    needs cow, and extent(called it extA) will be CoWed to a new
    extent(called it extB) incorrectly.  And extA is not used any more,
    but didn't be unmapped (didn't do dax_disassociate_entry()).
  The next time we mapwrite to another file, xfs will allocate extA for it,
    page fault handler do dax_associate_entry().  BUT bucause the extA didn't
    be unmapped, it still stores old file's info in page->mapping,->index.
    Then, It reports dmesg warning when it try to sotre the new file's info.

So, I think:
  1. reflink flag should be updated after CoW operations.
  2. xfs_reflink_allocate_cow() should add "if extent is shared" to determine
     xfs do CoW or not.

I made the fix patch, it can resolve the fail of generic/388.  But it causes
other cases fail: generic/127, generic/263, generic/616, xfs/315 xfs/421. I'm
not sure if the fix is right, or I have missed something somewhere.  Please
give me some advice.

Thank you very much!!

[1]: https://lore.kernel.org/linux-xfs/1669908538-55-1-git-send-email-ruansy.fnst@fujitsu.com/

Signed-off-by: Shiyang Ruan <ruansy.fnst@...itsu.com>
---
 fs/xfs/xfs_reflink.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_reflink.h |  2 ++
 2 files changed, 46 insertions(+)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index f5dc46ce9803..a6b07f5c1db2 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -154,6 +154,40 @@ xfs_reflink_find_shared(
 	return error;
 }
 
+int xfs_reflink_extent_is_shared(
+	struct xfs_inode	*ip,
+	struct xfs_bmbt_irec	*irec,
+	bool			*shared)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_perag	*pag;
+	xfs_agblock_t		agbno;
+	xfs_extlen_t		aglen;
+	xfs_agblock_t		fbno;
+	xfs_extlen_t		flen;
+	int			error = 0;
+
+	*shared = false;
+
+	/* Holes, unwritten, and delalloc extents cannot be shared */
+	if (!xfs_bmap_is_written_extent(irec))
+		return 0;
+
+	pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, irec->br_startblock));
+	agbno = XFS_FSB_TO_AGBNO(mp, irec->br_startblock);
+	aglen = irec->br_blockcount;
+	error = xfs_reflink_find_shared(pag, NULL, agbno, aglen, &fbno, &flen,
+			true);
+	xfs_perag_put(pag);
+	if (error)
+		return error;
+
+	if (fbno != NULLAGBLOCK)
+		*shared = true;
+
+	return 0;
+}
+
 /*
  * Trim the mapping to the next block where there's a change in the
  * shared/unshared status.  More specifically, this means that we
@@ -533,6 +567,12 @@ xfs_reflink_allocate_cow(
 		xfs_ifork_init_cow(ip);
 	}
 
+	error = xfs_reflink_extent_is_shared(ip, imap, shared);
+	if (error)
+		return error;
+	if (!*shared)
+		return 0;
+
 	error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
 	if (error || !*shared)
 		return error;
@@ -834,6 +874,10 @@ xfs_reflink_end_cow_extent(
 	/* Remove the mapping from the CoW fork. */
 	xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
 
+	error = xfs_reflink_clear_inode_flag(ip, &tp);
+	if (error)
+		goto out_cancel;
+
 	error = xfs_trans_commit(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	if (error)
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index 65c5dfe17ecf..d5835814bce6 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -16,6 +16,8 @@ static inline bool xfs_is_cow_inode(struct xfs_inode *ip)
 	return xfs_is_reflink_inode(ip) || xfs_is_always_cow_inode(ip);
 }
 
+int xfs_reflink_extent_is_shared(struct xfs_inode *ip,
+		struct xfs_bmbt_irec *irec, bool *shared);
 extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
 		struct xfs_bmbt_irec *irec, bool *shared);
 int xfs_bmap_trim_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
-- 
2.39.2