Re: [PATCH 1/2] mm: add infrastructure for per-dirty-page accounting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] mm: out-of-bounds read in folio_accounted_dirty_pages() on swapcache folios
- [High] mm: per-folio nr_dirtied in folio_account_dirtied() breaks dirty throttling
- [Medium] mm: writeback wb switch accounting drift for AS_DIRTY_PER_BLOCK mappings
- [Medium] mm: per-folio WB_DONTCACHE_DIRTY discharge in __filemap_get_folio_mpol()
- [Medium] mm: dirty counter leak and double charge after folio_clear_dirty_for_io()
--

commit 1721442268f1f31932ef0e4f11e5d239eea01bb6
Author: Salvatore Dipietro <dipiets@amazon.it>

mm: add infrastructure for per-dirty-page accounting

This commit adds memory management infrastructure to allow filesystems to
account dirty pages at sub-folio granularity via the AS_DIRTY_PER_BLOCK flag
and the accounted_dirty_pages address_space_operations callback. It updates
folio dirtying, cleaning, writeback preparation, and migration paths to
support per-page dirty accounting.

> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index eeab25d6ce364..6b4a8b6ad34c0 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2601,7 +2601,7 @@ EXPORT_SYMBOL(noop_dirty_folio);
>   * NOTE: This relies on being atomic wrt interrupts.
>   */
>  static void folio_account_dirtied(struct folio *folio,
> -		struct address_space *mapping)
> +		struct address_space *mapping, bool accounted)
>  {
>  	struct inode *inode = mapping->host;
>  
> @@ -2614,14 +2614,21 @@ static void folio_account_dirtied(struct folio *folio,
>  		inode_attach_wb(inode, folio);
>  		wb = inode_to_wb(inode);
>  
> -		lruvec_stat_mod_folio(folio, NR_FILE_DIRTY, nr);
> -		if (folio_test_dropbehind(folio))
> -			wb_stat_mod(wb, WB_DONTCACHE_DIRTY, nr);
> -		__zone_stat_mod_folio(folio, NR_ZONE_WRITE_PENDING, nr);
> -		__node_stat_mod_folio(folio, NR_DIRTIED, nr);
> -		wb_stat_mod(wb, WB_RECLAIMABLE, nr);
> -		wb_stat_mod(wb, WB_DIRTIED, nr);
> -		task_io_account_write(nr * PAGE_SIZE);
> +		/*
> +		 * @accounted means the filesystem charged these per dirty page
> +		 * already; the rate limiters below stay per-folio.  Keep in sync
> +		 * with folio_account_cleaned() and folio_clear_dirty_for_io().
> +		 */
> +		if (!accounted) {
> +			lruvec_stat_mod_folio(folio, NR_FILE_DIRTY, nr);
> +			if (folio_test_dropbehind(folio))
> +				wb_stat_mod(wb, WB_DONTCACHE_DIRTY, nr);
> +			__zone_stat_mod_folio(folio, NR_ZONE_WRITE_PENDING, nr);
> +			wb_stat_mod(wb, WB_RECLAIMABLE, nr);
> +			__node_stat_mod_folio(folio, NR_DIRTIED, nr);
> +			wb_stat_mod(wb, WB_DIRTIED, nr);
> +			task_io_account_write(nr * PAGE_SIZE);
> +		}
>  		current->nr_dirtied += nr;
>  		__this_cpu_add(bdp_ratelimits, nr);

[Severity: High]
When accounted is true, keeping current->nr_dirtied and bdp_ratelimits
per-folio in folio_account_dirtied() appears to break dirty throttling in two
ways.

First, __filemap_dirty_folio() only calls __folio_mark_dirty() ->
folio_account_dirtied() when PG_dirty transitions from 0 to 1:

mm/page-writeback.c:__filemap_dirty_folio() {
	if (folio_test_set_dirty(folio))
		return false;

	__folio_mark_dirty(folio, mapping, !folio_test_private(folio), accounted);
	...
}

If a writer dirties a single block of several large folios while below the
freerun ceiling, only 1 page per folio is charged to NR_FILE_DIRTY and
WB_RECLAIMABLE, and balance_dirty_pages() takes the freerun path and resets
current->nr_dirtied to 0. When subsequent buffered writes dirty the remaining
clean blocks of those already-dirty large folios, the filesystem charges the
newly dirtied pages to NR_FILE_DIRTY, NR_ZONE_WRITE_PENDING, and
WB_RECLAIMABLE, but __filemap_dirty_folio() returns early because PG_dirty is
already set. Since current->nr_dirtied and bdp_ratelimits are never
incremented for those subsequent writes, won't
balance_dirty_pages_ratelimited_flags() fail to call balance_dirty_pages() to
wake background writeback or throttle the task as NR_FILE_DIRTY and
WB_RECLAIMABLE grow past bg_thresh and dirty_thresh?

Second, when dirty throttling is active and a writer dirties 1 page of each
new large folio, the filesystem increments WB_DIRTIED by 1 page per folio
while folio_account_dirtied() increments current->nr_dirtied by
folio_nr_pages(folio). In wb_update_dirty_ratelimit(), dirty_rate is computed
from WB_DIRTIED and balanced_dirty_ratelimit is clamped to write_bw:

mm/page-writeback.c:wb_update_dirty_ratelimit() {
	...
	balanced_dirty_ratelimit = div_u64((u64)task_ratelimit * write_bw,
					   dirty_rate | 1);
	if (unlikely(balanced_dirty_ratelimit > write_bw))
		balanced_dirty_ratelimit = write_bw;
}

Because balance_dirty_pages() computes the pause duration from
current->nr_dirtied divided by task_ratelimit, doesn't mixing per-page
WB_DIRTIED with per-folio current->nr_dirtied cause severe over-throttling
when partially dirtying large folios?

>  
> @@ -2629,13 +2636,41 @@ static void folio_account_dirtied(struct folio *folio,
>  	}
>  }
>  
> +/**
> + * folio_accounted_dirty_pages - pages of @folio charged to the dirty counters
> + * @folio: folio to query
> + *
> + * Use this rather than folio_nr_pages() when moving or discharging a folio's dirty
> + * accounting, as a folio is not necessarily charged for all of its pages.  Walks
> + * per-block state that truncation frees, so the caller must hold the folio lock.
> + *
> + * Return: the whole folio for the usual per-folio accounting, otherwise only the
> + * pages holding a dirty block, and zero once those have been discharged.
> + */
> +long folio_accounted_dirty_pages(struct folio *folio)
> +{
> +	struct address_space *mapping = folio->mapping;
> +
> +	if (mapping && mapping_dirty_per_block(mapping) && folio->private &&
> +	    mapping->a_ops->accounted_dirty_pages)
> +		return mapping->a_ops->accounted_dirty_pages(folio);
> +	return folio_nr_pages(folio);
> +}

[Severity: Critical]
Can this cause an out-of-bounds read and invalid function pointer dereference
when migrating dirty anonymous or KSM folios that are in the swap cache?

During migration, move_to_new_folio() obtains mapping via
folio_mapping(src), which returns swap_address_space(folio->swap) for swap
cache folios:

mm/util.c:folio_mapping() {
	...
	if (unlikely(folio_test_swapcache(folio)))
		return swap_address_space(folio->swap);

	mapping = folio->mapping;
	if ((unsigned long)mapping & FOLIO_MAPPING_FLAGS)
		return NULL;
	...
}

Because mapping is non-NULL, swap_aops.migrate_folio (migrate_folio()) calls
__migrate_folio() -> __folio_migrate_mapping(), which invokes
folio_accounted_dirty_pages(folio) when folio_test_dirty(folio) is true:

mm/migrate.c:__folio_migrate_mapping() {
	...
	if (folio_test_dirty(folio))
		nr_dirty = folio_accounted_dirty_pages(folio);
}

However, folio_accounted_dirty_pages() reads raw folio->mapping instead of
folio_mapping(folio). For an anonymous or KSM folio in the swap cache,
folio->mapping is a tagged pointer to struct anon_vma (FOLIO_MAPPING_ANON) or
struct ksm_stable_node (FOLIO_MAPPING_KSM), and folio->private (union with
folio->swap) is non-zero.

Calling mapping_dirty_per_block(mapping) then performs test_bit() on
&mapping->flags at an unaligned offset past the end of struct anon_vma or
struct ksm_stable_node, and if the bit happens to be set in adjacent slab
memory, dereferences mapping->a_ops->accounted_dirty_pages(folio) out of
bounds. Should folio_accounted_dirty_pages() use folio_mapping(folio) instead
of folio->mapping?

[ ... ]

> @@ -2912,12 +2954,21 @@ bool folio_clear_dirty_for_io(struct folio *folio)
>  		 */
>  		wb = unlocked_inode_to_wb_begin(inode, &cookie);
>  		if (folio_test_clear_dirty(folio)) {
> -			long nr = folio_nr_pages(folio);
> -			lruvec_stat_mod_folio(folio, NR_FILE_DIRTY, -nr);
> -			if (folio_test_dropbehind(folio))
> -				wb_stat_mod(wb, WB_DONTCACHE_DIRTY, -nr);
> -			zone_stat_mod_folio(folio, NR_ZONE_WRITE_PENDING, -nr);
> -			wb_stat_mod(wb, WB_RECLAIMABLE, -nr);
> +			/*
> +			 * Per-block folios keep their dirty bits, and the charge,
> +			 * until the fs writes them back.  Folios of such a mapping
> +			 * that never got per-block state were charged whole, so
> +			 * they discharge here as usual.
> +			 */
> +			if (!mapping_dirty_per_block(mapping) || !folio->private) {
> +				long nr = folio_nr_pages(folio);
> +
> +				lruvec_stat_mod_folio(folio, NR_FILE_DIRTY, -nr);
> +				if (folio_test_dropbehind(folio))
> +					wb_stat_mod(wb, WB_DONTCACHE_DIRTY, -nr);

[Severity: Medium]
Does WB_DONTCACHE_DIRTY accounting stay balanced when PG_dropbehind is
cleared on a lookup in __filemap_get_folio_mpol()?

In mm/filemap.c, __filemap_get_folio_mpol() clears PG_dropbehind on normal
cached lookups and decrements WB_DONTCACHE_DIRTY by folio_nr_pages(folio):

mm/filemap.c:__filemap_get_folio_mpol() {
	...
	if (!(fgp_flags & FGP_DONTCACHE) && folio_test_clear_dropbehind(folio)) {
		if (folio_test_dirty(folio) &&
		    mapping_can_writeback(mapping)) {
			struct inode *inode = mapping->host;
			struct bdi_writeback *wb;
			struct wb_lock_cookie cookie = {};
			long nr = folio_nr_pages(folio);

			wb = unlocked_inode_to_wb_begin(inode, &cookie);
			wb_stat_mod(wb, WB_DONTCACHE_DIRTY, -nr);
			unlocked_inode_to_wb_end(inode, &cookie);
		}
	}
}

If a large folio on an AS_DIRTY_PER_BLOCK mapping has PG_dropbehind set and
only a subset of its pages are dirtied, subtracting folio_nr_pages(folio)
instead of the accounted dirty pages will underflow WB_DONTCACHE_DIRTY.

Additionally, if an unlocked lookup such as filemap_fault() ->
filemap_get_folio() calls __filemap_get_folio_mpol() after
folio_clear_dirty_for_io() has cleared PG_dirty, but before the filesystem
has discharged the per-block dirty pages during writeback,
__filemap_get_folio_mpol() clears PG_dropbehind without decrementing
WB_DONTCACHE_DIRTY because folio_test_dirty(folio) is already false, and the
filesystem will later skip decrementing WB_DONTCACHE_DIRTY because
folio_test_dropbehind(folio) is now false. Doesn't this leak the
WB_DONTCACHE_DIRTY charge?

> +				zone_stat_mod_folio(folio, NR_ZONE_WRITE_PENDING, -nr);
> +				wb_stat_mod(wb, WB_RECLAIMABLE, -nr);

[Severity: Medium]
How does cgroup writeback switching in inode_do_switch_wbs() interact with
AS_DIRTY_PER_BLOCK mappings?

In fs/fs-writeback.c, inode_do_switch_wbs() transfers WB_RECLAIMABLE and
WB_DONTCACHE_DIRTY between old_wb and new_wb using folio_nr_pages(folio):

fs/fs-writeback.c:inode_do_switch_wbs() {
	...
	xas_for_each_marked(&xas, folio, ULONG_MAX, PAGECACHE_TAG_DIRTY) {
		if (folio_test_dirty(folio)) {
			long nr = folio_nr_pages(folio);
			wb_stat_mod(old_wb, WB_RECLAIMABLE, -nr);
			wb_stat_mod(new_wb, WB_RECLAIMABLE, nr);
			if (folio_test_dropbehind(folio)) {
				wb_stat_mod(old_wb, WB_DONTCACHE_DIRTY, -nr);
				wb_stat_mod(new_wb, WB_DONTCACHE_DIRTY, nr);
			}
		}
	}
}

If a large folio only has a sub-folio range charged to old_wb, won't
transferring folio_nr_pages(folio) underflow old_wb and over-charge new_wb?

Furthermore, unlocked_inode_to_wb_begin() and unlocked_inode_to_wb_end()
serialize against inode_do_switch_wbs() under the assumption that PG_dirty
indicates whether the folio's charge is still held on inode->i_wb. When
folio_clear_dirty_for_io() clears PG_dirty without discharging WB_RECLAIMABLE
and WB_DONTCACHE_DIRTY, if inode_do_switch_wbs() runs before the filesystem
discharges the per-block dirty state during writeback, inode_do_switch_wbs()
will see !folio_test_dirty(folio) and skip the folio, causing the filesystem
to later discharge the pages from new_wb while leaving them stranded on
old_wb.

> +			}

[Severity: Medium]
When folio_clear_dirty_for_io() clears PG_dirty while leaving NR_FILE_DIRTY,
WB_DONTCACHE_DIRTY, NR_ZONE_WRITE_PENDING, and WB_RECLAIMABLE charged for
per-block folios, can those charges be leaked, double-charged, or left on the
wrong zone if writeback does not immediately clear the per-block dirty bits?

Several paths in mm and filesystems rely on folio_test_dirty(folio) to know
whether dirty accounting is active:

1. If ->writepages bails out after folio_clear_dirty_for_io() without writing
   or clearing the per-block dirty state (for example, in the following iomap
   patch when iomap_writeback_folio() returns early via
   iomap_writeback_handle_eof() because the folio is beyond i_size after a
   racing truncate, or in ntfs3 where ntfs_writepages() and
   ntfs_mft_writepages() use iomap_dirty_folio() without
   iomap_clear_range_dirty()), the folio is unlocked with PG_dirty cleared
   while per-block dirty pages remain charged.

2. Once PG_dirty and PG_writeback are both false, reclaim, eviction, or folio
   splitting can invoke ->release_folio() (such as iomap_release_folio(),
   which only checks folio_test_dirty(folio)) and free folio->private, or
   truncation can call folio_cancel_dirty(), which only calls
   folio_account_cleaned() when folio_test_dirty(folio) is true, permanently
   leaking NR_FILE_DIRTY, NR_ZONE_WRITE_PENDING, WB_RECLAIMABLE, and
   WB_DONTCACHE_DIRTY.

3. In __folio_migrate_mapping(), both folio_accounted_dirty_pages(folio) and
   the cross-zone NR_FILE_DIRTY / NR_ZONE_WRITE_PENDING transfer are gated on
   folio_test_dirty(folio), so migrating a folio in this state leaves the
   dirty counters on oldzone and old_lruvec while the subsequent discharge
   subtracts them from newzone and new_lruvec.

4. If folio_redirty_for_writepage() is called after
   folio_clear_dirty_for_io(), it calls filemap_dirty_folio() with
   accounted = false, charging the entire folio_nr_pages(folio) on top of the
   still-charged per-block dirty pages.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027112702.3747009-1-dipiets@amazon.it?part=1