Re: [PATCH v2] btrfs: write-protect folios during data writeback

[Qu Wenruo <wqu@suse.com>]

在 2026/7/15 09:51, Boris Burkov 写道:
> commit 095be159f3eb ("btrfs: unify folio dirty flag clearing") replaced
> the folio_clear_dirty_for_io() call in extent_write_cache_pages() with a
> plain folio_test_dirty() check. Besides clearing the dirty flag,
> folio_clear_dirty_for_io() also calls folio_mkclean(), which write-protects
> the shared mmap PTEs mapping the folio. Note that we still do call
> folio_clear_dirty_for_io() later in submit_one_sector() when we clear
> dirty on the last sector of the folio (the only sector for non-subpage
> cases). But we lost this early call in extent_write_cache_pages().
> 
> Without the extra write-protection, a process with the file mmap-ed can
> modify a sector while it is being used by writeback in a way that
> expects a stable folio (checksumming, compressing, copying, etc...)
> without faulting, which manifests as a handful of concrete bugs.
> 
> 1. For large folios or subpage sectorsize, it is possible to submit a bio
> which does not cover the whole folio. When this happens, we will have a
> bio in flight for a folio that we have *not* called
> folio_clear_dirty_for_io() on. If a task with an existing mmap-ed PTE
> writes (without faulting..) in this window, it can result in
> corruptions. If the write arrives while the checksumming or writing itself
> is underway, this can result in an invalid checksum and later corruption
> reports on read. If the write arrives after checksumming/writing is done
> but before the last sector dirty is cleared, then the write is present
> in page cache but doesn't affect the dirty tracking and will be lost
> when the the folio is fully finished being submitted and the dirty bit
> is cleared. This results in losing the write even if fsync() is called.
> 
> 2. For zoned submissions which are done in batch separate from the main
> extent_writepage() loop, we also risk csum violations for those
> submissions. Zoned writes are clamped to max_zone_append_size and are
> not aligned with folios, so a submission can span two folios. The first
> folio being processed in extent_write_cache_pages() will call
> extent_write_locked_range() which will submit the partial range of the
> next folio, while the rest of that folio could still be dirty. So
> clearing dirty on the submitted sectors doesn't call
> folio_clear_dirty_for_io() and we have the same issue. Since
> extent_write_cache_pages() skips these batch submitted folios (they are
> already marked for writeback from submission by the preceding folio), we
> must add the extra write protection in lock_delalloc_folios().
> 
> 3. For inline extents this will subtly risk losing writes that happen
> after/while we copy the inline extent but before we clear dirty on
> the folio.
> 
> 4. For folios spanning EOF, mmap could tamper with the zeroed bytes past
> EOF and cause them to be persisted where future faults would improperly
> see them instead of zeros.
> 
> 5. Finally, for compressed extents, we risk modifying the folios while we
> work on compressing them which will result in corrupted compressed data.
> Specifically, in run_delalloc_compressed() we queue up work to do
> compress_file_range() in BTRFS_COMPRESSION_CHUNK_SIZE (512K) chunks which
> will call btrfs_folio_clamp_clear_dirty() on the range. For non-subpage,
> this will always clear the whole folio, safely. For subpage, we risk a
> partial clear here as well. In particular, imagine a 2M folio broken up
> into 512K chunks of work which might start compression work on one chunk
> before all the chunks compress_file_range() workers have gotten far
> enough to finish clearing all the dirty bitmaps of the folio and getting
> to folio_clear_dirty_for_io(). Large folios on the edges of submission
> ranges are similarly at risk to be only partly cleared.
> This particular gap was introduced by a second patch in the same series:
> commit a4ef54dbb576 ("btrfs: make extent_range_clear_dirty_for_io() to handle sector size < page size cases")
> 
> We cannot simply restore the call to folio_clear_dirty_for_io() because
> that also drops the dirty flag off the folio which violates invariants
> introduced for large folios by
> commit 334509ce9d07 ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")
> and results in failing to invalidate clean folios past i_size, resulting
> in deadlocks.
> 
> Therefore, to fix it, leave the existing semantics w.r.t. the folio's
> dirty flag (to preserve the correct invalidate behavior) but ensure that
> the other aspect of folio_clear_dirty_for_io(), folio_mkclean(), is run
> on the folio when we lock it for writeback.
> 
> Finally, to help prevent similar regressions in the future, add a debug
> warning that triggers at the known corruption sites if we have failed to
> write protect the folio.
> 
> Assisted-by: LLM (debug, reproduce, research fix, review patch)
> Fixes: 095be159f3eb ("btrfs: unify folio dirty flag clearing")
> Fixes: a4ef54dbb576 ("btrfs: make extent_range_clear_dirty_for_io() to handle sector size < page size cases")
> Signed-off-by: Boris Burkov <boris@bur.io>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
> Changelog:
> v2:
> - improved changelog to cover more details of the conditions needed to
>    cause the bugs.
> 
>   fs/btrfs/extent_io.c | 31 +++++++++++++++++++++++++++++++
>   fs/btrfs/extent_io.h |  5 +++++
>   fs/btrfs/inode.c     | 23 +++++++++++++++++------
>   3 files changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 8fbb798767ca..647b109ef61b 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -6,6 +6,7 @@
>   #include <linux/mm.h>
>   #include <linux/pagemap.h>
>   #include <linux/page-flags.h>
> +#include <linux/rmap.h>
>   #include <linux/sched/mm.h>
>   #include <linux/spinlock.h>
>   #include <linux/blkdev.h>
> @@ -299,6 +300,25 @@ static noinline void unlock_delalloc_folio(const struct inode *inode,
>   				PAGE_UNLOCK);
>   }
>   
> +#ifdef CONFIG_BTRFS_DEBUG
> +/*
> + * Writeback must write-protect a folio when locking it for IO, before
> + * anything consumes its data (zeroing, inline copy, compression,
> + * checksumming). If this fails, then an mmap writer would be able to
> + * modify the data concurrently while we need it to be stable.
> + */
> +void btrfs_check_folio_write_protected(struct folio *folio)
> +{
> +	if (folio_mkclean(folio)) {
> +		const struct btrfs_inode *inode = BTRFS_I(folio->mapping->host);
> +
> +		DEBUG_WARN("writable mmap PTEs, root %llu ino %llu pos %llu order %u",
> +			   btrfs_root_id(inode->root), btrfs_ino(inode), folio_pos(folio),
> +			   folio_order(folio));
> +	}
> +}
> +#endif
> +
>   static noinline int lock_delalloc_folios(struct inode *inode,
>   					 struct folio *locked_folio,
>   					 u64 start, u64 end)
> @@ -332,6 +352,8 @@ static noinline int lock_delalloc_folios(struct inode *inode,
>   				folio_unlock(folio);
>   				goto out;
>   			}
> +			/* Locked for writeback; revoke writable mmap PTEs before using the data. */
> +			folio_mkclean(folio);
>   			range_start = max_t(u64, folio_pos(folio), start);
>   			range_len = min_t(u64, folio_next_pos(folio), end + 1) - range_start;
>   			btrfs_folio_set_lock(fs_info, folio, range_start, range_len);
> @@ -1780,6 +1802,13 @@ static noinline_for_stack int extent_writepage_io(struct btrfs_inode *inode,
>   	ASSERT(end <= folio_end, "start=%llu len=%u folio_start=%llu folio_size=%zu",
>   	       start, len, folio_start, folio_size(folio));
>   
> +	/*
> +	 * We are about to checksum and write out the data, so it must not be
> +	 * mmap writeable, or we could corrupt the data and end up with invalid
> +	 * checksums.
> +	 */
> +	btrfs_check_folio_write_protected(folio);
> +
>   	/* Truncate the submit bitmap to the current range. */
>   	if (start > folio_start)
>   		bitmap_clear(bio_ctrl->submit_bitmap, 0,
> @@ -2590,6 +2619,8 @@ static int extent_write_cache_pages(struct address_space *mapping,
>   				continue;
>   			}
>   
> +			/* Locked for writeback; revoke writable mmap PTEs before using the data. */
> +			folio_mkclean(folio);
>   			ret = extent_writepage(folio, bio_ctrl);
>   			if (ret < 0) {
>   				done = true;
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 9896e15ddc40..869925337699 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -255,6 +255,11 @@ bool try_release_extent_mapping(struct folio *folio, gfp_t mask);
>   int try_release_extent_buffer(struct folio *folio);
>   
>   int btrfs_read_folio(struct file *file, struct folio *folio);
> +#ifdef CONFIG_BTRFS_DEBUG
> +void btrfs_check_folio_write_protected(struct folio *folio);
> +#else
> +static inline void btrfs_check_folio_write_protected(struct folio *folio) { }
> +#endif
>   void extent_write_locked_range(struct inode *inode, const struct folio *locked_folio,
>   			       u64 start, u64 end, struct writeback_control *wbc,
>   			       bool pages_dirty);
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index b47e2aa5071d..636196705fa3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -775,19 +775,28 @@ static inline void inode_should_defrag(struct btrfs_inode *inode,
>   
>   static int extent_range_clear_dirty_for_io(struct btrfs_inode *inode, u64 start, u64 end)
>   {
> +	pgoff_t index = start >> PAGE_SHIFT;
>   	const pgoff_t end_index = end >> PAGE_SHIFT;
>   	struct folio *folio;
>   	int ret = 0;
>   
> -	for (pgoff_t index = start >> PAGE_SHIFT; index <= end_index; index++) {
> +	while (index <= end_index) {
>   		folio = filemap_get_folio(inode->vfs_inode.i_mapping, index);
>   		if (IS_ERR(folio)) {
>   			if (!ret)
>   				ret = PTR_ERR(folio);
> +			index++;
>   			continue;
>   		}
> +		/*
> +		 * We are about to compress the folio, so it must not be mmap
> +		 * writeable or we could corrupt the data as we attempt to
> +		 * compress it.
> +		 */
> +		btrfs_check_folio_write_protected(folio);
>   		btrfs_folio_clamp_clear_dirty(inode->root->fs_info, folio, start,
>   					      end + 1 - start);
> +		index = folio_next_index(folio);
>   		folio_put(folio);
>   	}
>   	return ret;
> @@ -877,11 +886,6 @@ static void compress_file_range(struct btrfs_work *work)
>   
>   	inode_should_defrag(inode, start, end, end - start + 1, SZ_16K);
>   
> -	/*
> -	 * We need to call clear_page_dirty_for_io on each page in the range.
> -	 * Otherwise applications with the file mmap'd can wander in and change
> -	 * the page contents while we are compressing them.
> -	 */
>   	ret = extent_range_clear_dirty_for_io(inode, start, end);
>   
>   	/*
> @@ -2317,6 +2321,13 @@ static int run_delalloc_inline(struct btrfs_inode *inode, struct folio *locked_f
>   	int ret;
>   
>   	ASSERT(folio_pos(locked_folio) == 0);
> +	/*
> +	 * If an mmap writer could modify the folio while we copy it into an
> +	 * inline extent we might see only part of their modification then
> +	 * wrongly mark it clean again after copying, losing that write. So the
> +	 * folio must be write protected here.
> +	 */
> +	btrfs_check_folio_write_protected(locked_folio);
>   
>   	if (btrfs_inode_can_compress(inode) &&
>   	    inode_need_compress(inode, 0, blocksize, true)) {