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

[Vlastimil Babka <vbabka@suse.cz>]

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
>  			};