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

[Harry Yoo <harry.yoo@oracle.com>]

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