[PATCH] mm/slab: fix folio_test_{anon,ksm}() false positive on slabs

[PATCH] mm/slab: fix folio_test_{anon,ksm}() false positive on slabs

[Harry Yoo]

When running tools/mm/page-types (with flags, page-counts, MB omitted),
it reports that some slabs have KPF_ANON and KPF_KSM set:

  $ sudo ./page-types | grep slab
  _______S___________________________________	slab
  _______S____a________x_____________________	slab,anonymous,ksm

This is unexpected, as slab memory should never be treated as anonymous
memory. This is because slab->slabs shares the same offset as
page->mapping and setting the lower two bits of ->slabs field means
folio_test_anon() and folio_test_ksm() will return true on the slab:

  [ field ]		[ offset ] [ size ]
  page->mapping		24         8
  page->lru		8	   16

  slab->next		16	   8
  slab->slabs		24	   4

Reorder ->slabs and ->next, so that the layout will be:

  slab->slabs		16	   4
  slab->next		24	   8

After reordering, slab->slabs shares its offset with page->lru.prev,
which is not a problem. slab->next now shares the offset with
page->mapping, but that's fine, as a slab is double-word aligned.

With the change, the nonsense slab pages disappear:

  $ sudo ./page-types | grep slab
  _______S___________________________________	slab

Fixes: 130d4df57390 ("mm/sl[au]b: rearrange struct slab fields to allow larger rcu_head")
Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
---

No Cc: stable because we don't usually check folio_test_anon() on slabs.

 mm/slab.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/slab.h b/mm/slab.h
index 05a21dc796e0..2e7064f7709e 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -59,8 +59,8 @@ struct slab {
 				struct list_head slab_list;
 #ifdef CONFIG_SLUB_CPU_PARTIAL
 				struct {
-					struct slab *next;
 					int slabs;	/* Nr of slabs left */
+					struct slab *next;
 				};
 #endif
 			};
-- 
2.43.0

Re: [PATCH] mm/slab: fix folio_test_{anon,ksm}() false positive on slabs

[Harry Yoo]

On Mon, Jun 09, 2025 at 10:27:23PM +0900, Harry Yoo wrote:
> When running tools/mm/page-types (with flags, page-counts, MB omitted),
> it reports that some slabs have KPF_ANON and KPF_KSM set:
> 
>   $ sudo ./page-types | grep slab
>   _______S___________________________________	slab
>   _______S____a________x_____________________	slab,anonymous,ksm
> 
> This is unexpected, as slab memory should never be treated as anonymous
> memory. This is because slab->slabs shares the same offset as
> page->mapping and setting the lower two bits of ->slabs field means
> folio_test_anon() and folio_test_ksm() will return true on the slab:
> 
>   [ field ]		[ offset ] [ size ]
>   page->mapping		24         8
>   page->lru		8	   16
> 
>   slab->next		16	   8
>   slab->slabs		24	   4
> 
> Reorder ->slabs and ->next, so that the layout will be:
> 
>   slab->slabs		16	   4
>   slab->next		24	   8
> 
> After reordering, slab->slabs shares its offset with page->lru.prev,
> which is not a problem. slab->next now shares the offset with
> page->mapping, but that's fine, as a slab is double-word aligned.
> 
> With the change, the nonsense slab pages disappear:
> 
>   $ sudo ./page-types | grep slab
>   _______S___________________________________	slab
> 
> Fixes: 130d4df57390 ("mm/sl[au]b: rearrange struct slab fields to allow larger rcu_head")
> Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
> ---

Oops, I should have mentioned that this may not _completely_ fix
the false positive.

Three years ago, there was a report [1] saying that CC_OPTIMIZE_FOR_SIZE
causes the function rcu_free_slab()'s alignment to be less than 4 bytes,
which can also lead to false positives :(

I don't know how the minimum function alignement has changed since then,
need to check.

[1] https://lore.kernel.org/all/ea96c78c-e1dc-1364-e91-51909f82388b@google.com/

> No Cc: stable because we don't usually check folio_test_anon() on slabs.
> 
>  mm/slab.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/slab.h b/mm/slab.h
> index 05a21dc796e0..2e7064f7709e 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -59,8 +59,8 @@ struct slab {
>  				struct list_head slab_list;
>  #ifdef CONFIG_SLUB_CPU_PARTIAL
>  				struct {
> -					struct slab *next;
>  					int slabs;	/* Nr of slabs left */
> +					struct slab *next;
>  				};
>  #endif
>  			};
> -- 
> 2.43.0
> 

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH] mm/slab: fix folio_test_{anon,ksm}() false positive on slabs

[Vlastimil Babka]

On 6/9/25 15:27, Harry Yoo wrote:
> When running tools/mm/page-types (with flags, page-counts, MB omitted),
> it reports that some slabs have KPF_ANON and KPF_KSM set:
> 
>   $ sudo ./page-types | grep slab
>   _______S___________________________________	slab
>   _______S____a________x_____________________	slab,anonymous,ksm
> 
> This is unexpected, as slab memory should never be treated as anonymous
> memory. This is because slab->slabs shares the same offset as
> page->mapping and setting the lower two bits of ->slabs field means
> folio_test_anon() and folio_test_ksm() will return true on the slab:
> 
>   [ field ]		[ offset ] [ size ]
>   page->mapping		24         8
>   page->lru		8	   16
> 
>   slab->next		16	   8
>   slab->slabs		24	   4
> 
> Reorder ->slabs and ->next, so that the layout will be:
> 
>   slab->slabs		16	   4
>   slab->next		24	   8
> 
> After reordering, slab->slabs shares its offset with page->lru.prev,
> which is not a problem. slab->next now shares the offset with
> page->mapping, but that's fine, as a slab is double-word aligned.

I think the double-word alignment doesn't even need to be mentioned, these
mapping flags are fine with just natural pointer's alignment that means the
lowest two bits are 0?

> 
> With the change, the nonsense slab pages disappear:
> 
>   $ sudo ./page-types | grep slab
>   _______S___________________________________	slab
> 
> Fixes: 130d4df57390 ("mm/sl[au]b: rearrange struct slab fields to allow larger rcu_head")
> Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
> ---
> 
> No Cc: stable because we don't usually check folio_test_anon() on slabs.

Hmm maybe we could just make the code generating /proc/kpageflags stop
testing/reporting those "flags" for slab pages? It could be even more future
proof code for when struct slab is separated. I mean there's even a comment
above PAGE_MAPPING_ANON:

 * For slab pages, since slab reuses the bits in struct page to store its
 * internal states, the folio->mapping does not exist as such, nor do
 * these flags below.  So in order to avoid testing non-existent bits,
 * please make sure that folio_test_slab(folio) actually evaluates to
 * false before calling the following functions (e.g., folio_test_anon).
 * See mm/slab.h.

Otherwise the change seems safe but maybe we're missing some other corner
case :) Willy what do you think?

>  mm/slab.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/slab.h b/mm/slab.h
> index 05a21dc796e0..2e7064f7709e 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -59,8 +59,8 @@ struct slab {
>  				struct list_head slab_list;
>  #ifdef CONFIG_SLUB_CPU_PARTIAL
>  				struct {
> -					struct slab *next;
>  					int slabs;	/* Nr of slabs left */
> +					struct slab *next;
>  				};
>  #endif
>  			};

Re: [PATCH] mm/slab: fix folio_test_{anon,ksm}() false positive on slabs

[Harry Yoo]

On Mon, Jun 09, 2025 at 04:22:01PM +0200, Vlastimil Babka wrote:
> On 6/9/25 15:27, Harry Yoo wrote:
> > When running tools/mm/page-types (with flags, page-counts, MB omitted),
> > it reports that some slabs have KPF_ANON and KPF_KSM set:
> > 
> >   $ sudo ./page-types | grep slab
> >   _______S___________________________________	slab
> >   _______S____a________x_____________________	slab,anonymous,ksm
> > 
> > This is unexpected, as slab memory should never be treated as anonymous
> > memory. This is because slab->slabs shares the same offset as
> > page->mapping and setting the lower two bits of ->slabs field means
> > folio_test_anon() and folio_test_ksm() will return true on the slab:
> > 
> >   [ field ]		[ offset ] [ size ]
> >   page->mapping		24         8
> >   page->lru		8	   16
> > 
> >   slab->next		16	   8
> >   slab->slabs		24	   4
> > 
> > Reorder ->slabs and ->next, so that the layout will be:
> > 
> >   slab->slabs		16	   4
> >   slab->next		24	   8
> > 
> > After reordering, slab->slabs shares its offset with page->lru.prev,
> > which is not a problem. slab->next now shares the offset with
> > page->mapping, but that's fine, as a slab is double-word aligned.
> 
> I think the double-word alignment doesn't even need to be mentioned, these
> mapping flags are fine with just natural pointer's alignment that means the
> lowest two bits are 0?

Ok. that's fine.

> > With the change, the nonsense slab pages disappear:
> > 
> >   $ sudo ./page-types | grep slab
> >   _______S___________________________________	slab
> > 
> > Fixes: 130d4df57390 ("mm/sl[au]b: rearrange struct slab fields to allow larger rcu_head")
> > Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
> > ---
> > 
> > No Cc: stable because we don't usually check folio_test_anon() on slabs.
> 
> Hmm maybe we could just make the code generating /proc/kpageflags stop
> testing/reporting those "flags" for slab pages?
>
> It could be even more future proof code for when struct slab is separated.

Not sure if I follow. When struct slab is separated, checking if a memdesc
refers to slab or not will not have false positives, and thus no need for
"must check folio_test_slab() before checking folio_test_anon()" rule?

Of course, to fix false positive completely, I think we need to add
something like:

__aligned(4) or __aligned(CONFIG_FUNCTION_ALIGNMENT) (whichever alignment is bigger)

to rcu_free_slab().

> I mean there's even a comment above PAGE_MAPPING_ANON:
>
>  * For slab pages, since slab reuses the bits in struct page to store its
>  * internal states, the folio->mapping does not exist as such, nor do
>  * these flags below.  So in order to avoid testing non-existent bits,
>  * please make sure that folio_test_slab(folio) actually evaluates to
>  * false before calling the following functions (e.g., folio_test_anon).
>  * See mm/slab.h.

That comment was added because it was a hard lesson learned from
false positive. 

-- 
Cheers,
Harry / Hyeonggon

> Otherwise the change seems safe but maybe we're missing some other corner
> case :) Willy what do you think?
> 
> >  mm/slab.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/slab.h b/mm/slab.h
> > index 05a21dc796e0..2e7064f7709e 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -59,8 +59,8 @@ struct slab {
> >  				struct list_head slab_list;
> >  #ifdef CONFIG_SLUB_CPU_PARTIAL
> >  				struct {
> > -					struct slab *next;
> >  					int slabs;	/* Nr of slabs left */
> > +					struct slab *next;
> >  				};
> >  #endif
> >  			};

Re: [PATCH] mm/slab: fix folio_test_{anon,ksm}() false positive on slabs

[Vlastimil Babka]

On 6/10/25 14:38, Harry Yoo wrote:
> On Mon, Jun 09, 2025 at 04:22:01PM +0200, Vlastimil Babka wrote:
>> > With the change, the nonsense slab pages disappear:
>> > 
>> >   $ sudo ./page-types | grep slab
>> >   _______S___________________________________	slab
>> > 
>> > Fixes: 130d4df57390 ("mm/sl[au]b: rearrange struct slab fields to allow larger rcu_head")
>> > Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
>> > ---
>> > 
>> > No Cc: stable because we don't usually check folio_test_anon() on slabs.
>> 
>> Hmm maybe we could just make the code generating /proc/kpageflags stop
>> testing/reporting those "flags" for slab pages?
>>
>> It could be even more future proof code for when struct slab is separated.
> 
> Not sure if I follow. When struct slab is separated, checking if a memdesc
> refers to slab or not will not have false positives, and thus no need for
> "must check folio_test_slab() before checking folio_test_anon()" rule?

Not sure I follow either. I mean today we can add code testing
folio_test_slab() and only evaluating the mapping flags (ksm, anon) if it's
not slab. Once we convert to memdescs, that code will stay logically the
same even the testing functions will be named differently, no?

> Of course, to fix false positive completely, I think we need to add
> something like:
> 
> __aligned(4) or __aligned(CONFIG_FUNCTION_ALIGNMENT) (whichever alignment is bigger)
> 
> to rcu_free_slab().

OK.

>> I mean there's even a comment above PAGE_MAPPING_ANON:
>>
>>  * For slab pages, since slab reuses the bits in struct page to store its
>>  * internal states, the folio->mapping does not exist as such, nor do
>>  * these flags below.  So in order to avoid testing non-existent bits,
>>  * please make sure that folio_test_slab(folio) actually evaluates to
>>  * false before calling the following functions (e.g., folio_test_anon).
>>  * See mm/slab.h.
> 
> That comment was added because it was a hard lesson learned from
> false positive. 
> 

Re: [PATCH] mm/slab: fix folio_test_{anon,ksm}() false positive on slabs

[Harry Yoo]

On Tue, Jun 10, 2025 at 03:03:12PM +0200, Vlastimil Babka wrote:
> On 6/10/25 14:38, Harry Yoo wrote:
> > On Mon, Jun 09, 2025 at 04:22:01PM +0200, Vlastimil Babka wrote:
> >> > With the change, the nonsense slab pages disappear:
> >> > 
> >> >   $ sudo ./page-types | grep slab
> >> >   _______S___________________________________	slab
> >> > 
> >> > Fixes: 130d4df57390 ("mm/sl[au]b: rearrange struct slab fields to allow larger rcu_head")
> >> > Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
> >> > ---
> >> > 
> >> > No Cc: stable because we don't usually check folio_test_anon() on slabs.
> >> 
> >> Hmm maybe we could just make the code generating /proc/kpageflags stop
> >> testing/reporting those "flags" for slab pages?
> >>
> >> It could be even more future proof code for when struct slab is separated.
> > 
> > Not sure if I follow. When struct slab is separated, checking if a memdesc
> > refers to slab or not will not have false positives, and thus no need for
> > "must check folio_test_slab() before checking folio_test_anon()" rule?
> 
> Not sure I follow either. I mean today we can add code testing
> folio_test_slab() and only evaluating the mapping flags (ksm, anon) if it's
> not slab. Once we convert to memdescs, that code will stay logically the
> same even the testing functions will be named differently, no?

Ah, I thought you were saying it is unnecessary to fix folio_test_slab()
false positives as long as we always check folio_test_slab() before
folio_test_anon().

Changing /proc/kpageflags in that direction will work and will be
future proof. Will do that.

-- 
Cheers,
Harry / Hyeonggon
 
> > Of course, to fix false positive completely, I think we need to add
> > something like:
> > 
> > __aligned(4) or __aligned(CONFIG_FUNCTION_ALIGNMENT) (whichever alignment is bigger)
> > 
> > to rcu_free_slab().
> 
> OK.
> 
> >> I mean there's even a comment above PAGE_MAPPING_ANON:
> >>
> >>  * For slab pages, since slab reuses the bits in struct page to store its
> >>  * internal states, the folio->mapping does not exist as such, nor do
> >>  * these flags below.  So in order to avoid testing non-existent bits,
> >>  * please make sure that folio_test_slab(folio) actually evaluates to
> >>  * false before calling the following functions (e.g., folio_test_anon).
> >>  * See mm/slab.h.
> > 
> > That comment was added because it was a hard lesson learned from
> > false positive. 
> > 
> 

Re: [PATCH] mm/slab: fix folio_test_{anon,ksm}() false positive on slabs

[David Hildenbrand]

On 10.06.25 15:17, Harry Yoo wrote:
> On Tue, Jun 10, 2025 at 03:03:12PM +0200, Vlastimil Babka wrote:
>> On 6/10/25 14:38, Harry Yoo wrote:
>>> On Mon, Jun 09, 2025 at 04:22:01PM +0200, Vlastimil Babka wrote:
>>>>> With the change, the nonsense slab pages disappear:
>>>>>
>>>>>    $ sudo ./page-types | grep slab
>>>>>    _______S___________________________________	slab
>>>>>
>>>>> Fixes: 130d4df57390 ("mm/sl[au]b: rearrange struct slab fields to allow larger rcu_head")
>>>>> Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
>>>>> ---
>>>>>
>>>>> No Cc: stable because we don't usually check folio_test_anon() on slabs.
>>>>
>>>> Hmm maybe we could just make the code generating /proc/kpageflags stop
>>>> testing/reporting those "flags" for slab pages?
>>>>
>>>> It could be even more future proof code for when struct slab is separated.
>>>
>>> Not sure if I follow. When struct slab is separated, checking if a memdesc
>>> refers to slab or not will not have false positives, and thus no need for
>>> "must check folio_test_slab() before checking folio_test_anon()" rule?
>>
>> Not sure I follow either. I mean today we can add code testing
>> folio_test_slab() and only evaluating the mapping flags (ksm, anon) if it's
>> not slab. Once we convert to memdescs, that code will stay logically the
>> same even the testing functions will be named differently, no?
> 
> Ah, I thought you were saying it is unnecessary to fix folio_test_slab()
> false positives as long as we always check folio_test_slab() before
> folio_test_anon().

Yes, relevant code should do that. And we already do it at a couple of 
places (e.g., folio_expected_ref_count()).

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/slab: fix folio_test_{anon,ksm}() false positive on slabs

[Christoph Lameter (Ampere)]

On Mon, 9 Jun 2025, Harry Yoo wrote:

>   [ field ]		[ offset ] [ size ]
>   page->mapping		24         8
>   page->lru		8	   16
>
>   slab->next		16	   8
>   slab->slabs		24	   4
>
> Reorder ->slabs and ->next, so that the layout will be:
>
>   slab->slabs		16	   4
>   slab->next		24	   8
>
> After reordering, slab->slabs shares its offset with page->lru.prev,
> which is not a problem. slab->next now shares the offset with
> page->mapping, but that's fine, as a slab is double-word aligned.

Add comments to document the trickery that is going on here? Otherwise we
may stumble across this issue again later.

Re: [PATCH] mm/slab: fix folio_test_{anon,ksm}() false positive on slabs

[Harry Yoo]

On Mon, Jun 09, 2025 at 09:06:04AM -0700, Christoph Lameter (Ampere) wrote:
> On Mon, 9 Jun 2025, Harry Yoo wrote:
> 
> >   [ field ]		[ offset ] [ size ]
> >   page->mapping		24         8
> >   page->lru		8	   16
> >
> >   slab->next		16	   8
> >   slab->slabs		24	   4
> >
> > Reorder ->slabs and ->next, so that the layout will be:
> >
> >   slab->slabs		16	   4
> >   slab->next		24	   8
> >
> > After reordering, slab->slabs shares its offset with page->lru.prev,
> > which is not a problem. slab->next now shares the offset with
> > page->mapping, but that's fine, as a slab is double-word aligned.
> 
> Add comments to document the trickery that is going on here? Otherwise we
> may stumble across this issue again later.

Will add, thanks!

-- 
Cheers,
Harry / Hyeonggon