Re: [PATCH] mm: avoid KCSAN false positive in page_to_nid()

[Lorenzo Stoakes <ljs@kernel.org>]

On Tue, Jun 23, 2026 at 09:59:45AM +0200, David Hildenbrand (Arm) wrote:
> On 6/23/26 09:41, Hui Zhu wrote:
> > From: Hui Zhu <zhuhui@kylinos.cn>
> >
> > KCSAN reports a data race between page_to_nid() reading page->flags
> > and folio_trylock()/folio_lock() doing test_and_set_bit_lock(PG_locked,
> > ...) on the same word from another CPU, e.g.:
> >
> >   BUG: KCSAN: data-race in __lruvec_stat_mod_folio / shmem_get_folio_gfp
> >
> > The node id occupies a fixed, high bit-range of page->flags that is
> > set once when the page is initialized and never modified afterwards,
> > so it can never overlap with the low PG_locked/PG_waiters bits touched
> > by the folio lock path. The race is therefore harmless: page_to_nid()
> > always returns a consistent value regardless of how the read
> > interleaves with the lock bit ops.
> >
> > Wrap the flags read with data_race() to tell KCSAN this race is
> > intentional and benign, consistent with how page->page_type is
> > already annotated for similar packed-field accesses.
> >
> > Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>

How did you discover this?

A syzbot report? If so please include Reported-by, Closes tags.

> > ---
> >  include/linux/mm.h | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 485df9c2dbdd..122d3b39369f 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2296,7 +2296,14 @@ static inline int memdesc_nid(memdesc_flags_t mdf)
> >
> >  static inline int page_to_nid(const struct page *page)
> >  {
> > -	return memdesc_nid(PF_POISONED_CHECK(page)->flags);
> > +	/*
> > +	 * The node id occupies a fixed high bit-range of page->flags
> > +	 * that is set once at page init and never changed afterwards.
> > +	 * It cannot overlap with the low PG_locked/PG_waiters bits
> > +	 * that folio_lock()/folio_unlock() concurrently update, so
> > +	 * this data race is benign.
> > +	 */
>
> Do we really need this excessive comment?

Agreed, just delete it. For a trivial benign data race it's a bit much, and the
commit message can cover it off for those who are curious.

>
> > +	return memdesc_nid(data_race(PF_POISONED_CHECK(page)->flags));
>
> In memdesc_zonenum() we use ASSERT_EXCLUSIVE_BITS.
>
> Can we do the same here inside memdesc_nid?

Also agreed

>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 69daeeab7fe8f..76d3bb54be844 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2290,6 +2290,7 @@ int memdesc_nid(memdesc_flags_t mdf);
>  #else
>  static inline int memdesc_nid(memdesc_flags_t mdf)
>  {
> +       ASSERT_EXCLUSIVE_BITS(mdf.f, NODES_MASK << NODES_PGSHIFT);
>         return (mdf.f >> NODES_PGSHIFT) & NODES_MASK;
>  }
>  #endif
>
>
> --
> Cheers,
>
> David

Thanks, Lorenzo