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

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

[Hui Zhu]

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>
---
 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.
+	 */
+	return memdesc_nid(data_race(PF_POISONED_CHECK(page)->flags));
 }
 
 static inline int folio_nid(const struct folio *folio)
-- 
2.43.0

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

[David Hildenbrand (Arm)]

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>
> ---
>  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?

> +	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?

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

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

[Lorenzo Stoakes]

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