| 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
| ||
|
Message-ID: <20230317203505.GK11394@frogsfrogsfrogs>
Date: Fri, 17 Mar 2023 13:35:05 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: Shiyang Ruan <ruansy.fnst@...itsu.com>
Cc: linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
nvdimm@...ts.linux.dev, linux-kernel@...r.kernel.org,
david@...morbit.com, dan.j.williams@...el.com,
akpm@...ux-foundation.org
Subject: Re: [RFC PATCH] xfs: check shared state of when CoW, update reflink
flag when io ends
On Fri, Mar 17, 2023 at 03:59:48AM +0000, Shiyang Ruan wrote:
> 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()).
IOWs, dax_iomap_copy_around (or something very near it) should be
calling dax_disassociate_entry on the source range after copying extA's
contents to extB to drop its page->shared count?
> 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);
This will disable COW on /all/ blocks in the entire file, including the
shared ones. At a bare minimum you'd have to scan the entire data fork
to ensure there are no shared extents. That's probably why doing this
causes so many new regressions.
--D
> + 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
>
Powered by blists - more mailing lists