Re: [PATCH] mm/slub, kunit: Make slub_kunit pass even when SLAB_RED_ZONE flag is set

[Hyeonggon Yoo <42.hyeyoo@gmail.com>]

On Wed, Mar 16, 2022 at 11:11:08PM +0100, Vlastimil Babka wrote:
> On 3/16/22 15:38, Hyeonggon Yoo wrote:
> > Testcase test_next_pointer in slub_kunit fails when SLAB_RED_ZONE flag
> > is globally set. This is because on_freelist() cuts corrupted freelist
> > chain and does not update cut objects' redzone to SLUB_RED_ACTIVE.
> > 
> > When the test validates a slab that whose freelist is cut, it expects
> > redzone of objects unreachable by freelist is set to SLUB_RED_ACTIVE.
> > And it reports "Left Redzone overritten" error because the expectation
> > failed.
> > 
> > This patch makes slub_kunit expect two more errors for reporting and
> > fixing red overwritten error when SLAB_RED_ZONE flag is set.
> > 
> > The test passes on slub_debug and slub_debug=Z after this patch.
> 
> Hmm I think it's not optimal strategy for unit tests to adapt like this to
> external influence. It seems rather fragile. The test cases should be
> designed to test a specific condition and that's it. So maybe we could e.g.
> introduce a new SLAB_ flag passed to kmem_cache_create that tells it to
> ignore any globally specified slub debug flags?

Agree. It's so easy to be broken. I think your suggestion is
good for unit tests. I'll send a patch.

BTW, The situation is that SLUB makes (by cutting freelist chain)
objects invalid (invalid redzone) and reporting what caused by SLUB
itself is quite ugly.

No simple solution comes into mind but yeah... it's not a big problem
as we rarely call validate_slab_cache().

> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> > ---
> >  lib/slub_kunit.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
> > index 8662dc6cb509..7cf1fb5a7fde 100644
> > --- a/lib/slub_kunit.c
> > +++ b/lib/slub_kunit.c
> > @@ -45,21 +45,36 @@ static void test_next_pointer(struct kunit *test)
> >  	 * Expecting three errors.
> >  	 * One for the corrupted freechain and the other one for the wrong
> >  	 * count of objects in use. The third error is fixing broken cache.
> > +	 *
> > +	 * When flag SLUB_RED_ZONE is set, we expect two more errors for reporting
> > +	 * and fixing overwritten redzone error. This two errors are detected
> > +	 * because SLUB cuts corrupted freelist in on_freelist(), but does not
> > +	 * update its redzone to SLUB_RED_ACTIVE.
> >  	 */
> >  	validate_slab_cache(s);
> > -	KUNIT_EXPECT_EQ(test, 3, slab_errors);
> > +
> > +	if (s->flags & SLAB_RED_ZONE)
> > +		KUNIT_EXPECT_EQ(test, 5, slab_errors);
> > +	else
> > +		KUNIT_EXPECT_EQ(test, 3, slab_errors);
> >  
> >  	/*
> >  	 * Try to repair corrupted freepointer.
> >  	 * Still expecting two errors. The first for the wrong count
> >  	 * of objects in use.
> >  	 * The second error is for fixing broken cache.
> > +	 *
> > +	 * When SLUB_RED_ZONE flag is set, we expect two more errors
> > +	 * for same reason as above.
> >  	 */
> >  	*ptr_addr = tmp;
> >  	slab_errors = 0;
> >  
> >  	validate_slab_cache(s);
> > -	KUNIT_EXPECT_EQ(test, 2, slab_errors);
> > +	if (s->flags & SLAB_RED_ZONE)
> > +		KUNIT_EXPECT_EQ(test, 4, slab_errors);
> > +	else
> > +		KUNIT_EXPECT_EQ(test, 2, slab_errors);
> >  
> >  	/*
> >  	 * Previous validation repaired the count of objects in use.
> 

-- 
Thank you, You are awesome!
Hyeonggon :-)