Re: [PATCH v12 2/7] mm: take a folio off WB_DONTCACHE_DIRTY only if it was counted there

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mm: unsigned negation in filemap_clear_dropbehind() corrupts
  counter on 32-bit
--

commit 08b68ff1ed201d0ae5bce5df36b625f50a1bfa1f
Author: Mike Snitzer <snitzer@kernel.org>

mm: take a folio off WB_DONTCACHE_DIRTY only if it was counted there

This commit updates filemap_clear_dropbehind() to hold the folio lock and
i_pages lock before clearing the dropbehind flag and decrementing
WB_DONTCACHE_DIRTY when both PG_dirty and PAGECACHE_TAG_DIRTY are set.

> diff --git a/mm/filemap.c b/mm/filemap.c
> index 437113addbddf..98ce955404127 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1956,22 +1956,36 @@ void *filemap_get_entry(struct address_space *mapping, pgoff_t index)
>  	return folio;
>  }
>  
> -/* Clear the dropbehind flag of a folio that a cached lookup found. */
> +/*
> + * Clear the dropbehind flag of a folio that a cached lookup found, and take
> + * the folio off WB_DONTCACHE_DIRTY if it is counted there.  With the folio
> + * locked and the i_pages lock held, the folio is counted if both PG_dirty and
> + * the dirty tag are set.  A folio that another task has locked keeps the flag.
> + */
>  static void filemap_clear_dropbehind(struct address_space *mapping,
> -				     struct folio *folio)
> +				     struct folio *folio, bool locked)
>  {
> -	if (!folio_test_clear_dropbehind(folio))
> -		return;
> -	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);
> +	unsigned long flags;
>  
> -		wb = unlocked_inode_to_wb_begin(inode, &cookie);
> -		wb_stat_mod(wb, WB_DONTCACHE_DIRTY, -nr);
> -		unlocked_inode_to_wb_end(inode, &cookie);
> +	if (!locked) {
> +		if (!folio_trylock(folio))
> +			return;
> +		if (unlikely(folio->mapping != mapping))
> +			goto out;
> +	}
> +	if (!mapping_can_writeback(mapping)) {
> +		folio_clear_dropbehind(folio);
> +		goto out;
>  	}
> +	xa_lock_irqsave(&mapping->i_pages, flags);
> +	if (folio_test_clear_dropbehind(folio) && folio_test_dirty(folio) &&
> +	    xa_get_mark(&mapping->i_pages, folio->index, PAGECACHE_TAG_DIRTY))
> +		wb_stat_mod(inode_to_wb(mapping->host), WB_DONTCACHE_DIRTY,
> +			    -folio_nr_pages(folio));

[Severity: Medium]
Does passing -folio_nr_pages(folio) to wb_stat_mod() corrupt the
WB_DONTCACHE_DIRTY counter on 32-bit architectures?

folio_nr_pages() returns unsigned long, whereas wb_stat_mod() takes s64.
On 32-bit platforms, negating a 32-bit unsigned long produces 2^32 - nr,
which is then zero-extended to s64, adding a large positive value instead
of subtracting nr pages.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112190911.84702-1-snitzer@kernel.org?part=2