Re: [PATCH] mm: fix data race in __filemap_remove_folio / folio_mapping

[Matthew Wilcox <willy@infradead.org>]

On Mon, Mar 23, 2026 at 12:33:19AM +0530, Abhishek Kumar wrote:
> KCSAN reports a data race between page_cache_delete() and
> folio_mapping():
> 
>   page_cache_delete() performs a plain store to folio->mapping:
>     folio->mapping = NULL;
> 
>   folio_mapping() performs a plain load from folio->mapping:
>     mapping = folio->mapping;
> 
> page_cache_delete() is called from the truncation path under the i_pages
> xarray lock,

That's not relevant.  The important lock for maintaining folio->mapping
is the folio lock (see the VM_BUG_ON_FOLIO line in page_cache_delete()).
At a minimum, this changelog needs to be fixed because there's already
too much confusion around the locking rules.

> while folio_mapping() is called from the reclaim path
> (evict_folios -> folio_evictable -> folio_mapping) under only
> rcu_read_lock() without the xarray lock.

Umm.  First up, this is MGLRU-only code, right?  Adding the so-called
maintainers.

Second ... I'm really unsure how we want to handle this generally.
This could be quite the game of whack-a-mole; we have many, many places
in the kernel which dereference folio->mapping without holding a lock.

Perhaps they are all fine; but 12 of the 455 references to
folio->mapping currently have READ_ONCE attached.  That's a lot of code
to audit.

> The race is benign since the reclaim path tolerates stale values --
> reading a stale non-NULL mapping simply results in a suboptimal eviction
> decision.  However, the plain accesses risk store/load tearing and allow
> the compiler to perform harmful optimizations (merging, elision, or
> fission of the accesses).

I think the bigger problem is reloading.  As I understand it, this code:

	struct address_space *m = folio->mapping;

	if (m && m->flags)

could end up loading 'm' twice, once before the setting to NULL and once
after.  That's more plausible than deciding to load byte-by-byte, or
whatever else these "merging, elision, or fission" words mean.

> Fix this by using WRITE_ONCE() in page_cache_delete() and READ_ONCE()
> in folio_mapping() to prevent compiler misbehavior and silence the KCSAN
> report.

Just to be clear, I don't object to the patch itself, I'm just scared
of the consequences.  And the locking comment above needs to be fixed.
But please wait a few days for discussion to play out.