From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3CFFFC433B4 for ; Fri, 30 Apr 2021 08:50:57 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 7EC0F6142A for ; Fri, 30 Apr 2021 08:50:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7EC0F6142A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B9F316B006C; Fri, 30 Apr 2021 04:50:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B27956B0071; Fri, 30 Apr 2021 04:50:55 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 951A46B0072; Fri, 30 Apr 2021 04:50:55 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0184.hostedemail.com [216.40.44.184]) by kanga.kvack.org (Postfix) with ESMTP id 73B226B006C for ; Fri, 30 Apr 2021 04:50:55 -0400 (EDT) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 26E5D3632 for ; Fri, 30 Apr 2021 08:50:55 +0000 (UTC) X-FDA: 78088413270.04.12E4133 Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by imf13.hostedemail.com (Postfix) with ESMTP id 62681E000105 for ; Fri, 30 Apr 2021 08:50:45 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 43FCFAF88; Fri, 30 Apr 2021 08:50:53 +0000 (UTC) To: Andrew Morton , cl@linux.com, glittao@gmail.com, iamjoonsoo.kim@lge.com, linux-mm@kvack.org, mm-commits@vger.kernel.org, penberg@kernel.org, rientjes@google.com, torvalds@linux-foundation.org References: <20210430055445.hpofzhr9G%akpm@linux-foundation.org> From: Vlastimil Babka Subject: Re: [patch 030/178] kunit: add a KUnit test for SLUB debugging functionality Message-ID: Date: Fri, 30 Apr 2021 10:50:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <20210430055445.hpofzhr9G%akpm@linux-foundation.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 62681E000105 Authentication-Results: imf13.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf13.hostedemail.com: domain of vbabka@suse.cz designates 195.135.220.15 as permitted sender) smtp.mailfrom=vbabka@suse.cz X-Stat-Signature: 8bumzzj63u4h1jrxcpwbbwj4hw1pj38i Received-SPF: none (suse.cz>: No applicable sender policy available) receiver=imf13; identity=mailfrom; envelope-from=""; helo=mx2.suse.de; client-ip=195.135.220.15 X-HE-DKIM-Result: none/none X-HE-Tag: 1619772645-729629 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 4/30/21 7:54 AM, Andrew Morton wrote: > From: Oliver Glitta > Subject: kunit: add a KUnit test for SLUB debugging functionality >=20 > SLUB has resiliency_test() function which is hidden behind #ifdef > SLUB_RESILIENCY_TEST that is not part of Kconfig, so nobody runs it.=20 > KUnit should be a proper replacement for it. >=20 > Try changing byte in redzone after allocation and changing pointer to n= ext > free node, first byte, 50th byte and redzone byte. Check if validation > finds errors. >=20 > There are several differences from the original resiliency test: Tests > create own caches with known state instead of corrupting shared kmalloc > caches. >=20 > The corruption of freepointer uses correct offset, the original resilie= ncy > test got broken with freepointer changes. >=20 > Scratch changing random byte test, because it does not have meaning in > this form where we need deterministic results. >=20 > Add new option CONFIG_SLUB_KUNIT_TEST in Kconfig. >=20 > Add a counter field "errors" to struct kmem_cache to count number of > errors detected in cache. >=20 > Silence bug report in SLUB test. Add SLAB_SILENT_ERRORS debug flag. A= dd > SLAB_SILENT_ERRORS flag to SLAB_NEVER_MERGE, SLAB_DEBUG_FLAGS, > SLAB_FLAGS_PERMITTED macros. >=20 > Link: https://lkml.kernel.org/r/20210331085156.5028-1-glittao@gmail.com > Signed-off-by: Oliver Glitta > Acked-by: Vlastimil Babka > Cc: Christoph Lameter > Cc: David Rientjes > Cc: Joonsoo Kim > Cc: Pekka Enberg > Signed-off-by: Andrew Morton Hi, but this is v3 [1]. Then there was v4 [2] (with a prerequisity [3]) w= ith many changes after feedback with kunit folks, which would be better. We c= ould apply v4 as improvement later but there would be extra churn (even the te= st .c file is named differently). Merging v4 directly would make more sense. Anyway "[patch 031/178] slub: remove resiliency_test() function" (which t= he kunit test replaces) can go either way as that code is dead and broken. Thanks. [1] https://lkml.kernel.org/r/20210331085156.5028-1-glittao@gmail.com [2] https://lore.kernel.org/linux-kselftest/20210413100747.4921-2-glittao= @gmail.com/ [3] https://lore.kernel.org/linux-kselftest/20210413100747.4921-1-glittao= @gmail.com/ > --- >=20 > include/linux/slab.h | 2=20 > include/linux/slub_def.h | 2=20 > lib/Kconfig.debug | 5 + > lib/Makefile | 1=20 > lib/test_slub.c | 124 +++++++++++++++++++++++++++++++++++++ > mm/slab.h | 7 +- > mm/slab_common.c | 2=20 > mm/slub.c | 64 +++++++++++++------ > 8 files changed, 184 insertions(+), 23 deletions(-) >=20 > --- a/include/linux/slab.h~kunit-add-a-kunit-test-for-slub-debugging-fu= nctionality > +++ a/include/linux/slab.h > @@ -25,6 +25,8 @@ > */ > /* DEBUG: Perform (expensive) checks on alloc/free */ > #define SLAB_CONSISTENCY_CHECKS ((slab_flags_t __force)0x00000100U) > +/* DEBUG: Silent bug reports */ > +#define SLAB_SILENT_ERRORS ((slab_flags_t __force)0x00000200U) > /* DEBUG: Red zone objs in a cache */ > #define SLAB_RED_ZONE ((slab_flags_t __force)0x00000400U) > /* DEBUG: Poison objects */ > --- a/include/linux/slub_def.h~kunit-add-a-kunit-test-for-slub-debuggin= g-functionality > +++ a/include/linux/slub_def.h > @@ -133,6 +133,8 @@ struct kmem_cache { > unsigned int usersize; /* Usercopy region size */ > =20 > struct kmem_cache_node *node[MAX_NUMNODES]; > + > + int errors; /* Number of errors in cache */ > }; > =20 > #ifdef CONFIG_SLUB_CPU_PARTIAL > --- a/lib/Kconfig.debug~kunit-add-a-kunit-test-for-slub-debugging-funct= ionality > +++ a/lib/Kconfig.debug > @@ -2429,6 +2429,11 @@ config BITS_TEST > =20 > If unsure, say N. > =20 > +config SLUB_KUNIT_TEST > + tristate "KUnit Test for SLUB cache error detection" if !KUNIT_ALL_TE= STS > + depends on SLUB_DEBUG && KUNIT > + default KUNIT_ALL_TESTS > + > config TEST_UDELAY > tristate "udelay test driver" > help > --- a/lib/Makefile~kunit-add-a-kunit-test-for-slub-debugging-functional= ity > +++ a/lib/Makefile > @@ -353,5 +353,6 @@ obj-$(CONFIG_LIST_KUNIT_TEST) +=3D list-te > obj-$(CONFIG_LINEAR_RANGES_TEST) +=3D test_linear_ranges.o > obj-$(CONFIG_BITS_TEST) +=3D test_bits.o > obj-$(CONFIG_CMDLINE_KUNIT_TEST) +=3D cmdline_kunit.o > +obj-$(CONFIG_SLUB_KUNIT_TEST) +=3D test_slub.o > =20 > obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) +=3D devmem_is_allowed.o > --- /dev/null > +++ a/lib/test_slub.c > @@ -0,0 +1,124 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include > +#include > +#include > +#include > +#include > +#include "../mm/slab.h" > + > + > +static void test_clobber_zone(struct kunit *test) > +{ > + struct kmem_cache *s =3D kmem_cache_create("TestSlub_RZ_alloc", 64, 0= , > + SLAB_RED_ZONE | SLAB_SILENT_ERRORS, NULL); > + u8 *p =3D kmem_cache_alloc(s, GFP_KERNEL); > + > + p[64] =3D 0x12; > + > + validate_slab_cache(s); > + KUNIT_EXPECT_EQ(test, 1, s->errors); > + > + kmem_cache_free(s, p); > + kmem_cache_destroy(s); > +} > + > +static void test_next_pointer(struct kunit *test) > +{ > + struct kmem_cache *s =3D kmem_cache_create("TestSlub_next_ptr_free", = 64, 0, > + SLAB_POISON | SLAB_SILENT_ERRORS, NULL); > + u8 *p =3D kmem_cache_alloc(s, GFP_KERNEL); > + unsigned long tmp; > + unsigned long *ptr_addr; > + > + kmem_cache_free(s, p); > + > + ptr_addr =3D (unsigned long *)(p + s->offset); > + tmp =3D *ptr_addr; > + p[s->offset] =3D 0x12; > + > + /* > + * Expecting two errors. > + * One for the corrupted freechain and the other one for the wrong > + * count of objects in use. > + */ > + validate_slab_cache(s); > + KUNIT_EXPECT_EQ(test, 2, s->errors); > + > + /* > + * Try to repair corrupted freepointer. > + * Still expecting one error for the wrong count of objects in use. > + */ > + *ptr_addr =3D tmp; > + > + validate_slab_cache(s); > + KUNIT_EXPECT_EQ(test, 1, s->errors); > + > + /* > + * Previous validation repaired the count of objects in use. > + * Now expecting no error. > + */ > + validate_slab_cache(s); > + KUNIT_EXPECT_EQ(test, 0, s->errors); > + > + kmem_cache_destroy(s); > +} > + > +static void test_first_word(struct kunit *test) > +{ > + struct kmem_cache *s =3D kmem_cache_create("TestSlub_1th_word_free", = 64, 0, > + SLAB_POISON | SLAB_SILENT_ERRORS, NULL); > + u8 *p =3D kmem_cache_alloc(s, GFP_KERNEL); > + > + kmem_cache_free(s, p); > + *p =3D 0x78; > + > + validate_slab_cache(s); > + KUNIT_EXPECT_EQ(test, 1, s->errors); > + > + kmem_cache_destroy(s); > +} > + > +static void test_clobber_50th_byte(struct kunit *test) > +{ > + struct kmem_cache *s =3D kmem_cache_create("TestSlub_50th_word_free",= 64, 0, > + SLAB_POISON | SLAB_SILENT_ERRORS, NULL); > + u8 *p =3D kmem_cache_alloc(s, GFP_KERNEL); > + > + kmem_cache_free(s, p); > + p[50] =3D 0x9a; > + > + validate_slab_cache(s); > + KUNIT_EXPECT_EQ(test, 1, s->errors); > + kmem_cache_destroy(s); > +} > + > +static void test_clobber_redzone_free(struct kunit *test) > +{ > + struct kmem_cache *s =3D kmem_cache_create("TestSlub_RZ_free", 64, 0, > + SLAB_RED_ZONE | SLAB_SILENT_ERRORS, NULL); > + u8 *p =3D kmem_cache_alloc(s, GFP_KERNEL); > + > + kmem_cache_free(s, p); > + p[64] =3D 0xab; > + > + validate_slab_cache(s); > + KUNIT_EXPECT_EQ(test, 1, s->errors); > + kmem_cache_destroy(s); > +} > + > +static struct kunit_case test_cases[] =3D { > + KUNIT_CASE(test_clobber_zone), > + KUNIT_CASE(test_next_pointer), > + KUNIT_CASE(test_first_word), > + KUNIT_CASE(test_clobber_50th_byte), > + KUNIT_CASE(test_clobber_redzone_free), > + {} > +}; > + > +static struct kunit_suite test_suite =3D { > + .name =3D "slub_test", > + .test_cases =3D test_cases, > +}; > +kunit_test_suite(test_suite); > + > +MODULE_LICENSE("GPL"); > --- a/mm/slab_common.c~kunit-add-a-kunit-test-for-slub-debugging-functi= onality > +++ a/mm/slab_common.c > @@ -55,7 +55,7 @@ static DECLARE_WORK(slab_caches_to_rcu_d > */ > #define SLAB_NEVER_MERGE (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USE= R | \ > SLAB_TRACE | SLAB_TYPESAFE_BY_RCU | SLAB_NOLEAKTRACE | \ > - SLAB_FAILSLAB | kasan_never_merge()) > + SLAB_FAILSLAB | SLAB_SILENT_ERRORS | kasan_never_merge()) > =20 > #define SLAB_MERGE_SAME (SLAB_RECLAIM_ACCOUNT | SLAB_CACHE_DMA | \ > SLAB_CACHE_DMA32 | SLAB_ACCOUNT) > --- a/mm/slab.h~kunit-add-a-kunit-test-for-slub-debugging-functionality > +++ a/mm/slab.h > @@ -134,7 +134,8 @@ static inline slab_flags_t kmem_cache_fl > #define SLAB_DEBUG_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USE= R) > #elif defined(CONFIG_SLUB_DEBUG) > #define SLAB_DEBUG_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USE= R | \ > - SLAB_TRACE | SLAB_CONSISTENCY_CHECKS) > + SLAB_TRACE | SLAB_CONSISTENCY_CHECKS | \ > + SLAB_SILENT_ERRORS) > #else > #define SLAB_DEBUG_FLAGS (0) > #endif > @@ -164,7 +165,8 @@ static inline slab_flags_t kmem_cache_fl > SLAB_NOLEAKTRACE | \ > SLAB_RECLAIM_ACCOUNT | \ > SLAB_TEMPORARY | \ > - SLAB_ACCOUNT) > + SLAB_ACCOUNT | \ > + SLAB_SILENT_ERRORS) > =20 > bool __kmem_cache_empty(struct kmem_cache *); > int __kmem_cache_shutdown(struct kmem_cache *); > @@ -215,6 +217,7 @@ DECLARE_STATIC_KEY_TRUE(slub_debug_enabl > DECLARE_STATIC_KEY_FALSE(slub_debug_enabled); > #endif > extern void print_tracking(struct kmem_cache *s, void *object); > +long validate_slab_cache(struct kmem_cache *s); > #else > static inline void print_tracking(struct kmem_cache *s, void *object) > { > --- a/mm/slub.c~kunit-add-a-kunit-test-for-slub-debugging-functionality > +++ a/mm/slub.c > @@ -674,14 +674,16 @@ static void slab_bug(struct kmem_cache * > =20 > static void slab_fix(struct kmem_cache *s, char *fmt, ...) > { > - struct va_format vaf; > - va_list args; > - > - va_start(args, fmt); > - vaf.fmt =3D fmt; > - vaf.va =3D &args; > - pr_err("FIX %s: %pV\n", s->name, &vaf); > - va_end(args); > + if (!(s->flags & SLAB_SILENT_ERRORS)) { > + struct va_format vaf; > + va_list args; > + > + va_start(args, fmt); > + vaf.fmt =3D fmt; > + vaf.va =3D &args; > + pr_err("FIX %s: %pV\n", s->name, &vaf); > + va_end(args); > + } > } > =20 > static bool freelist_corrupted(struct kmem_cache *s, struct page *page= , > @@ -740,8 +742,10 @@ static void print_trailer(struct kmem_ca > void object_err(struct kmem_cache *s, struct page *page, > u8 *object, char *reason) > { > - slab_bug(s, "%s", reason); > - print_trailer(s, page, object); > + if (!(s->flags & SLAB_SILENT_ERRORS)) { > + slab_bug(s, "%s", reason); > + print_trailer(s, page, object); > + } > } > =20 > static __printf(3, 4) void slab_err(struct kmem_cache *s, struct page = *page, > @@ -753,9 +757,11 @@ static __printf(3, 4) void slab_err(stru > va_start(args, fmt); > vsnprintf(buf, sizeof(buf), fmt, args); > va_end(args); > - slab_bug(s, "%s", buf); > - print_page_info(page); > - dump_stack(); > + if (!(s->flags & SLAB_SILENT_ERRORS)) { > + slab_bug(s, "%s", buf); > + print_page_info(page); > + dump_stack(); > + } > } > =20 > static void init_object(struct kmem_cache *s, void *object, u8 val) > @@ -799,11 +805,13 @@ static int check_bytes_and_report(struct > while (end > fault && end[-1] =3D=3D value) > end--; > =20 > - slab_bug(s, "%s overwritten", what); > - pr_err("0x%p-0x%p @offset=3D%tu. First byte 0x%x instead of 0x%x\n", > + if (!(s->flags & SLAB_SILENT_ERRORS)) { > + slab_bug(s, "%s overwritten", what); > + pr_err("0x%p-0x%p @offset=3D%tu. First byte 0x%x instead of 0x%x\n", > fault, end - 1, fault - addr, > fault[0], value); > - print_trailer(s, page, object); > + print_trailer(s, page, object); > + } > =20 > restore_bytes(s, what, value, fault, end); > return 0; > @@ -965,6 +973,7 @@ static int check_slab(struct kmem_cache > =20 > if (!PageSlab(page)) { > slab_err(s, page, "Not a valid slab page"); > + s->errors +=3D 1; > return 0; > } > =20 > @@ -972,11 +981,13 @@ static int check_slab(struct kmem_cache > if (page->objects > maxobj) { > slab_err(s, page, "objects %u > max %u", > page->objects, maxobj); > + s->errors +=3D 1; > return 0; > } > if (page->inuse > page->objects) { > slab_err(s, page, "inuse %u > max %u", > page->inuse, page->objects); > + s->errors +=3D 1; > return 0; > } > /* Slab_pad_check fixes things up after itself */ > @@ -1009,8 +1020,10 @@ static int on_freelist(struct kmem_cache > page->freelist =3D NULL; > page->inuse =3D page->objects; > slab_fix(s, "Freelist cleared"); > + s->errors +=3D 1; > return 0; > } > + s->errors +=3D 1; > break; > } > object =3D fp; > @@ -1027,12 +1040,14 @@ static int on_freelist(struct kmem_cache > page->objects, max_objects); > page->objects =3D max_objects; > slab_fix(s, "Number of objects adjusted."); > + s->errors +=3D 1; > } > if (page->inuse !=3D page->objects - nr) { > slab_err(s, page, "Wrong object count. Counter is %d but counted wer= e %d", > page->inuse, page->objects - nr); > page->inuse =3D page->objects - nr; > slab_fix(s, "Object count adjusted."); > + s->errors +=3D 1; > } > return search =3D=3D NULL; > } > @@ -4641,8 +4656,10 @@ static void validate_slab(struct kmem_ca > u8 val =3D test_bit(__obj_to_index(s, addr, p), map) ? > SLUB_RED_INACTIVE : SLUB_RED_ACTIVE; > =20 > - if (!check_object(s, page, p, val)) > + if (!check_object(s, page, p, val)) { > + s->errors +=3D 1; > break; > + } > } > put_map(map); > unlock: > @@ -4662,9 +4679,11 @@ static int validate_slab_node(struct kme > validate_slab(s, page); > count++; > } > - if (count !=3D n->nr_partial) > + if (count !=3D n->nr_partial) { > pr_err("SLUB %s: %ld partial slabs counted but counter=3D%ld\n", > s->name, count, n->nr_partial); > + s->errors +=3D 1; > + } > =20 > if (!(s->flags & SLAB_STORE_USER)) > goto out; > @@ -4673,20 +4692,23 @@ static int validate_slab_node(struct kme > validate_slab(s, page); > count++; > } > - if (count !=3D atomic_long_read(&n->nr_slabs)) > + if (count !=3D atomic_long_read(&n->nr_slabs)) { > pr_err("SLUB: %s %ld slabs counted but counter=3D%ld\n", > s->name, count, atomic_long_read(&n->nr_slabs)); > + s->errors +=3D 1; > + } > =20 > out: > spin_unlock_irqrestore(&n->list_lock, flags); > return count; > } > =20 > -static long validate_slab_cache(struct kmem_cache *s) > +long validate_slab_cache(struct kmem_cache *s) > { > int node; > unsigned long count =3D 0; > struct kmem_cache_node *n; > + s->errors =3D 0; > =20 > flush_all(s); > for_each_kmem_cache_node(s, node, n) > @@ -4694,6 +4716,8 @@ static long validate_slab_cache(struct k > =20 > return count; > } > +EXPORT_SYMBOL(validate_slab_cache); > + > /* > * Generate lists of code addresses where slabcache objects are alloca= ted > * and freed. > _ >=20