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

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

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