Re: [PATCH] mm/ksm: mark migration stores with WRITE_ONCE()

[David Hildenbrand (Arm)]

On 8/22/26 18:38, Chengfeng Ye wrote:
> ksm_get_folio() deliberately samples stable_node->kpfn and
> folio->mapping without taking the folio lock because the KSM folio may be
> migrated concurrently.  folio_migrate_ksm() updates the same state using
> plain assignments.
> 
> The reader can load the old kpfn, then the migrator can store the new kpfn,
> execute smp_wmb(), and clear the old folio's mapping before the reader
> checks that mapping.  Thus the initial kpfn load can overlap its update and
> the subsequent mapping load can overlap the clear, with no common lock.
> This leaves marked READ_ONCE() accesses racing with plain stores.
> 
> The kernel reported:
> 
>   BUG: KCSAN: data-race in folio_migrate_ksm / ksm_get_folio
> 
>   read (marked) to 0xffff8ce401421330 of 8 bytes by task 48 on cpu 3:
>    ksm_get_folio+0x7f/0x2a0
>    ksm_scan_thread+0x1635/0x3330
>    kthread+0x1af/0x1f0
> 
>   write to 0xffff8ce401421330 of 8 bytes by task 102 on cpu 1:
>    folio_migrate_ksm+0x6a/0xd0
>    folio_migrate_flags+0x193/0x420
>    __migrate_folio.isra.0+0x162/0x1a0
>    migrate_folio+0x4c/0x70
>    move_to_new_folio+0xd6/0x170
> 
> Use WRITE_ONCE() for both stores to pair them with the existing lockless
> reads.  This preserves the existing smp_wmb()/smp_rmb() migration protocol
> and control flow while preventing compiler transformations of the shared
> accesses.
> 

We want a Fixes: tag, can you dig?

I am not convinced CC stable is warranted ... but certainly wouldn't hurt here.

> Signed-off-by: Chengfeng Ye <nicoyip.dev@xxxxxxxxx>
> ---
>  mm/ksm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index b4142746777e..bec6fea0fdb4 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -1116,7 +1116,8 @@ static inline void folio_set_stable_node(struct folio *folio,
>  					 struct ksm_stable_node *stable_node)
>  {
>  	VM_WARN_ON_FOLIO(folio_test_anon(folio) && PageAnonExclusive(&folio->page), folio);
> -	folio->mapping = (void *)((unsigned long)stable_node | FOLIO_MAPPING_KSM);
> +	WRITE_ONCE(folio->mapping,
> +		   (void *)((unsigned long)stable_node | FOLIO_MAPPING_KSM));

That also matches what we do in folio_move_anon_rmap()/__folio_set_anon().

>  }
>  
>  #ifdef CONFIG_SYSFS
> @@ -3318,7 +3319,7 @@ void folio_migrate_ksm(struct folio *newfolio, struct folio *folio)
>  	stable_node = folio_stable_node(folio);
>  	if (stable_node) {
>  		VM_BUG_ON_FOLIO(stable_node->kpfn != folio_pfn(folio), folio);
> -		stable_node->kpfn = folio_pfn(newfolio);
> +		WRITE_ONCE(stable_node->kpfn, folio_pfn(newfolio));

That makes sense as well!

Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

-- 
Cheers,

David