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

[Vlastimil Babka <vbabka@suse.cz>]

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?

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