On Mon, Oct 16, 2023 at 05:28:52PM +0200, Christoph Hellwig wrote:
> xfs_reflink_end_cow_extent looks up the COW extent and the data fork
> extent at offset_fsb, and then proceeds to remap the common subset
> between the two.
>
> It does however not limit the remapped extent to the passed in
> [*offset_fsbm end_fsb] range and thus potentially remaps more blocks than
> the one handled by the current I/O completion. This means that with
> sufficiently large data and COW extents we could be remapping COW fork
> mappings that have not been written to, leading to a stale data exposure
> on a powerfail event.
>
> We use to have a xfs_trim_range to make the remap fit the I/O completion
> range, but that got (apparently accidentally) removed in commit
> df2fd88f8ac7 ("xfs: rewrite xfs_reflink_end_cow to use intents").
>
> Note that I've only found this by code inspection, and a test case would
> probably require very specific delay and error injection.
Hmm. xfs_prepare_ioend converts unwritten cowfork extents to written
prior to submit_bio. So I guess you'd have to trick writeback into
issuing totally separate bios for the single mapping. Then you'd have
to delay the bio for the higher offset part of the mapping while
allowing the bio for the lower part to complete, at which point it would
convey the entire mapping to the data fork. Then you'd have to convince
the kernel to reread the contents from disk. I think that would be hard
since the folios for the incomplete writeback are still uptodate and
marked for writeback. directio will block trying to flush and
invalidate the cache, and buffered io will read the pagecache.
So I don't think there's an actual exposure vector here, but others can
chime in about whatever I've missed. ;)
> Fixes: df2fd88f8ac7 ("xfs: rewrite xfs_reflink_end_cow to use intents")
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
Yep, that was a goof, thanks for catching that.
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_reflink.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index eb9102453affbf..0611af06771589 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -784,6 +784,7 @@ xfs_reflink_end_cow_extent(
> }
> }
> del = got;
> + xfs_trim_extent(&del, *offset_fsb, end_fsb - *offset_fsb);
>
> /* Grab the corresponding mapping in the data fork. */
> nmaps = 1;
> --
> 2.39.2
>
