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=-14.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_IN_DEF_DKIM_WL 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 1C9EBCA9EA9 for ; Fri, 18 Oct 2019 14:55:07 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B1975222C2 for ; Fri, 18 Oct 2019 14:55:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="RajIa5GN" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B1975222C2 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 5E4D68E0007; Fri, 18 Oct 2019 10:55:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 595498E0003; Fri, 18 Oct 2019 10:55:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 485198E0007; Fri, 18 Oct 2019 10:55:06 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0050.hostedemail.com [216.40.44.50]) by kanga.kvack.org (Postfix) with ESMTP id 249658E0003 for ; Fri, 18 Oct 2019 10:55:06 -0400 (EDT) Received: from smtpin21.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with SMTP id ACE8282F3711 for ; Fri, 18 Oct 2019 14:55:05 +0000 (UTC) X-FDA: 76057202970.21.frame83_497a7951feb48 X-HE-Tag: frame83_497a7951feb48 X-Filterd-Recvd-Size: 14700 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by imf35.hostedemail.com (Postfix) with ESMTP for ; Fri, 18 Oct 2019 14:55:04 +0000 (UTC) Received: by mail-wr1-f67.google.com with SMTP id c2so1357537wrr.10 for ; Fri, 18 Oct 2019 07:55:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=hi+BZI7iype3ttZ+NIgbZBZGb+tOPPjRFYuN+lufWUg=; b=RajIa5GNmp3mkBXNmSG5X9az2D8SqMg7nGtqyQzpAwirHry3s3JPSbAE81G50lSVlt 3VuWbDDrPxK4WmxtPqJMilAGdPexS/o9R5yPOwURIKYK4JMs+YOBTItUChkJN9PyEy26 +ygLRTAJ62FFQbPR8VPYl7VNJ25bRlfVA3y8BnmYEw/jz1brGa/fSeydueCtAudGQbS4 ImUBVZKQzi0xxVQOu3TszaKoqSS4vVKUiwowX/Q1U1U9i0T9FuCANhHByseek6ic1qub OsJgDLUgg2FAfkPtpXl+xwGbPTiN19u36w+C/sgQPaJnupZnOWIbUzvWBfjWiUoPfPkb VdJQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=hi+BZI7iype3ttZ+NIgbZBZGb+tOPPjRFYuN+lufWUg=; b=Pd+pyGGQs5w/HTotxkk/7xG1zdc8Ztv6cJX3SXdYoU5pusMYq8LhBq2iWlhcAaah9w 0m30QOU3te4O39TCN7rZ94Q+uHC+ugbCc0GJRfedJ1C3ybWwIJIeRknDGFSvXrNaZ0WC DLyYTzM80OsEXPicQTw+GekANZJou8sfzZ+2BsH/uQISZd2B8o/6Eh8zfea5ykNILxBO 9cPH8yr9LuQIahokaFF7dzj6VCTYv4NcIEOEkEzfBQ2JYZP3zyx9u3XlmIbsVRIg0kHQ L5MrShTaoDTNlpkqyYGNUw/ie71Vt7V75fSQeYFNhyO+E8hLeIaqNMo3CWTwjLbcsXt5 V1kw== X-Gm-Message-State: APjAAAUEPR/LnQKTVknHvtSPjauGHBysPB4urbIt7ZW8Jf/L9Jfrilf2 C1J+7vFM1gOdUltyfqe4hr0y2nQQIEXfDqx+lVh66LzTDtI= X-Google-Smtp-Source: APXvYqzwBLj6qzCQV5V4zVx9Q1i6YBe2dEw3ePoE2nF5BbLchMEIBoScBIkmGiUPsoSaKI4zYva9m+TpWufEOLbBqK4= X-Received: by 2002:a5d:4b42:: with SMTP id w2mr8069962wrs.360.1571410503284; Fri, 18 Oct 2019 07:55:03 -0700 (PDT) MIME-Version: 1.0 References: <20191018094304.37056-1-glider@google.com> <20191018094304.37056-19-glider@google.com> <1571404943.5937.71.camel@lca.pw> <1571406117.5937.73.camel@lca.pw> <1571409732.5937.76.camel@lca.pw> In-Reply-To: <1571409732.5937.76.camel@lca.pw> From: Alexander Potapenko Date: Fri, 18 Oct 2019 16:54:51 +0200 Message-ID: Subject: Re: [PATCH RFC v1 18/26] kmsan: mm: call KMSAN hooks from SLUB code To: Qian Cai Cc: Andrew Morton , Vegard Nossum , Dmitry Vyukov , Linux Memory Management List Content-Type: text/plain; charset="UTF-8" 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 Fri, Oct 18, 2019 at 4:42 PM Qian Cai wrote: > > On Fri, 2019-10-18 at 15:55 +0200, Alexander Potapenko wrote: > > On Fri, Oct 18, 2019 at 3:42 PM Qian Cai wrote: > > > > > > On Fri, 2019-10-18 at 15:33 +0200, Alexander Potapenko wrote: > > > > On Fri, Oct 18, 2019 at 3:22 PM Qian Cai wrote: > > > > > > > > > > On Fri, 2019-10-18 at 11:42 +0200, glider@google.com wrote: > > > > > > In order to report uninitialized memory coming from heap alloca= tions > > > > > > KMSAN has to poison them unless they're created with __GFP_ZERO= . > > > > > > > > > > > > It's handy that we need KMSAN hooks in the places where > > > > > > init_on_alloc/init_on_free initialization is performed. > > > > > > > > > > Well, there is SLUB debug which has red zoning and poisoning chec= ks. What's > > > > > value of this patch? > > > > > > > > Sorry, are you talking about the whole patchset or just this patch? > > > > > > Just this patch. > > > > > > > Note that SLUB debug is unable to detect uninitialized values with > > > > bit-to-bit precision, neither have I heard of anyone using it for > > > > detecting uses of uninitialized memory in the kernel at all. > > > > The purpose of SLUB debug is to detect corruptions of freed memory. > > > > > > The point is if developers have SLUB debug enabled, all the free obje= cts will be > > > poisoned, so what's the point of checking uninitialized memory there? > > > > You are right, SLUB debug has to be handled separately. If I'm > > understanding correctly, right now KMSAN poisons freed memory before > > SLUB debug wipes it, therefore the memory will count as initialized. > > On the other hand, newly allocated memory is still marked as > > uninitialized, so a lot of bugs still remain detectable. > > Yes, but newly allocated slub objects will be poisoned as well. As far as I can tell, KMSAN hook marking newly allocated objects as uninitialized is called after slub poisoning. Therefore these allocations will be treated by KMSAN as uninitialized. > > TBH, I haven't tested KMSAN with SLUB debug good enough. Note that > > it's anyway a separate build that requires Clang, so right now it > > doesn't make much sense to combine KMSAN and SLUB debug together. > > Can't you just build a debug kernel with SLUB debug enabled but dropping = this > patch? If there is an uninitialized memory here leading to data corruptio= n, SLUB > debug should be detected as well as this patch. If not, it needs to under= stand > why. Sorry, there might be some misunderstanding here. KMSAN keeps the state of heap objects separately by keeping exactly the same amount of initialized/uninitialized bits for every allocation. A call to kmsan_slab_alloc()/kmsan_slab_free() will mark an allocation as uninitialized for KMSAN. Not doing so will result in false reports. A call to memset(object, POISON_FREE, object_size) performed by SLAB debug will actually mark this allocation as initialized from KMSAN point of view, because we're memsetting a range with initialized data. Note that use of uninitialized data doesn't necessarily lead to immediate data corruption, so there might be nothing to detect for SLUB debug. > > > > > > > > > > > > > Signed-off-by: Alexander Potapenko > > > > > > To: Alexander Potapenko > > > > > > Cc: Andrew Morton > > > > > > Cc: Vegard Nossum > > > > > > Cc: Dmitry Vyukov > > > > > > Cc: linux-mm@kvack.org > > > > > > --- > > > > > > > > > > > > Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871 > > > > > > --- > > > > > > mm/slub.c | 37 +++++++++++++++++++++++++++++++------ > > > > > > 1 file changed, 31 insertions(+), 6 deletions(-) > > > > > > > > > > > > diff --git a/mm/slub.c b/mm/slub.c > > > > > > index 3d63ae320d31..3d6d4c63446e 100644 > > > > > > --- a/mm/slub.c > > > > > > +++ b/mm/slub.c > > > > > > @@ -21,6 +21,8 @@ > > > > > > #include > > > > > > #include > > > > > > #include > > > > > > +#include > > > > > > +#include /* KMSAN_INIT_VALUE */ > > > > > > #include > > > > > > #include > > > > > > #include > > > > > > @@ -285,17 +287,27 @@ static void prefetch_freepointer(const st= ruct kmem_cache *s, void *object) > > > > > > prefetch(object + s->offset); > > > > > > } > > > > > > > > > > > > +/* > > > > > > + * When running under KMSAN, get_freepointer_safe() may return= an uninitialized > > > > > > + * pointer value in the case the current thread loses the race= for the next > > > > > > + * memory chunk in the freelist. In that case this_cpu_cmpxchg= _double() in > > > > > > + * slab_alloc_node() will fail, so the uninitialized value won= 't be used, but > > > > > > + * KMSAN will still check all arguments of cmpxchg because of = imperfect > > > > > > + * handling of inline assembly. > > > > > > + * To work around this problem, use KMSAN_INIT_VALUE() to forc= e initialize the > > > > > > + * return value of get_freepointer_safe(). > > > > > > + */ > > > > > > static inline void *get_freepointer_safe(struct kmem_cache *s,= void *object) > > > > > > { > > > > > > unsigned long freepointer_addr; > > > > > > void *p; > > > > > > > > > > > > if (!debug_pagealloc_enabled()) > > > > > > - return get_freepointer(s, object); > > > > > > + return KMSAN_INIT_VALUE(get_freepointer(s, object= )); > > > > > > > > > > > > freepointer_addr =3D (unsigned long)object + s->offset; > > > > > > probe_kernel_read(&p, (void **)freepointer_addr, sizeof(p= )); > > > > > > - return freelist_ptr(s, p, freepointer_addr); > > > > > > + return KMSAN_INIT_VALUE(freelist_ptr(s, p, freepointer_ad= dr)); > > > > > > } > > > > > > > > > > > > static inline void set_freepointer(struct kmem_cache *s, void = *object, void *fp) > > > > > > @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_node_ho= ok(void *ptr, size_t size, gfp_t flags) > > > > > > ptr =3D kasan_kmalloc_large(ptr, size, flags); > > > > > > /* As ptr might get tagged, call kmemleak hook after KASA= N. */ > > > > > > kmemleak_alloc(ptr, size, 1, flags); > > > > > > + kmsan_kmalloc_large(ptr, size, flags); > > > > > > return ptr; > > > > > > } > > > > > > > > > > > > @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hook(vo= id *x) > > > > > > { > > > > > > kmemleak_free(x); > > > > > > kasan_kfree_large(x, _RET_IP_); > > > > > > + kmsan_kfree_large(x); > > > > > > } > > > > > > > > > > > > static __always_inline bool slab_free_hook(struct kmem_cache *= s, void *x) > > > > > > @@ -1453,6 +1467,12 @@ static inline bool slab_free_freelist_ho= ok(struct kmem_cache *s, > > > > > > } while (object !=3D old_tail); > > > > > > } > > > > > > > > > > > > + do { > > > > > > + object =3D next; > > > > > > + next =3D get_freepointer(s, object); > > > > > > + kmsan_slab_free(s, object); > > > > > > + } while (object !=3D old_tail); > > > > > > + > > > > > > /* > > > > > > * Compiler cannot detect this function can be removed if slab= _free_hook() > > > > > > * evaluates to nothing. Thus, catch all relevant config debu= g options here. > > > > > > @@ -2769,6 +2789,7 @@ static __always_inline void *slab_alloc_n= ode(struct kmem_cache *s, > > > > > > if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && obj= ect) > > > > > > memset(object, 0, s->object_size); > > > > > > > > > > > > + kmsan_slab_alloc(s, object, gfpflags); > > > > > > slab_post_alloc_hook(s, gfpflags, 1, &object); > > > > > > > > > > > > return object; > > > > > > @@ -2797,6 +2818,7 @@ void *kmem_cache_alloc_trace(struct kmem_= cache *s, gfp_t gfpflags, size_t size) > > > > > > void *ret =3D slab_alloc(s, gfpflags, _RET_IP_); > > > > > > trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags); > > > > > > ret =3D kasan_kmalloc(s, ret, size, gfpflags); > > > > > > + > > > > > > return ret; > > > > > > } > > > > > > EXPORT_SYMBOL(kmem_cache_alloc_trace); > > > > > > @@ -2809,7 +2831,6 @@ void *kmem_cache_alloc_node(struct kmem_c= ache *s, gfp_t gfpflags, int node) > > > > > > > > > > > > trace_kmem_cache_alloc_node(_RET_IP_, ret, > > > > > > s->object_size, s->size, gfpf= lags, node); > > > > > > - > > > > > > return ret; > > > > > > } > > > > > > EXPORT_SYMBOL(kmem_cache_alloc_node); > > > > > > @@ -2825,6 +2846,7 @@ void *kmem_cache_alloc_node_trace(struct = kmem_cache *s, > > > > > > size, s->size, gfpflags, node); > > > > > > > > > > > > ret =3D kasan_kmalloc(s, ret, size, gfpflags); > > > > > > + > > > > > > return ret; > > > > > > } > > > > > > EXPORT_SYMBOL(kmem_cache_alloc_node_trace); > > > > > > @@ -3150,7 +3172,7 @@ int kmem_cache_alloc_bulk(struct kmem_cac= he *s, gfp_t flags, size_t size, > > > > > > void **p) > > > > > > { > > > > > > struct kmem_cache_cpu *c; > > > > > > - int i; > > > > > > + int i, j; > > > > > > > > > > > > /* memcg and kmem_cache debug support */ > > > > > > s =3D slab_pre_alloc_hook(s, flags); > > > > > > @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct kmem_c= ache *s, gfp_t flags, size_t size, > > > > > > > > > > > > /* Clear memory outside IRQ disabled fastpath loop */ > > > > > > if (unlikely(slab_want_init_on_alloc(flags, s))) { > > > > > > - int j; > > > > > > - > > > > > > for (j =3D 0; j < i; j++) > > > > > > memset(p[j], 0, s->object_size); > > > > > > } > > > > > > + for (j =3D 0; j < i; j++) > > > > > > + kmsan_slab_alloc(s, p[j], flags); > > > > > > > > > > > > /* memcg and kmem_cache debug support */ > > > > > > slab_post_alloc_hook(s, flags, size, p); > > > > > > @@ -3793,6 +3815,7 @@ static int __init setup_slub_min_objects(= char *str) > > > > > > > > > > > > __setup("slub_min_objects=3D", setup_slub_min_objects); > > > > > > > > > > > > +__no_sanitize_memory > > > > > > void *__kmalloc(size_t size, gfp_t flags) > > > > > > { > > > > > > struct kmem_cache *s; > > > > > > @@ -5698,6 +5721,7 @@ static char *create_unique_id(struct kmem= _cache *s) > > > > > > p +=3D sprintf(p, "%07u", s->size); > > > > > > > > > > > > BUG_ON(p > name + ID_STR_LENGTH - 1); > > > > > > + kmsan_unpoison_shadow(name, p - name); > > > > > > return name; > > > > > > } > > > > > > > > > > > > @@ -5847,6 +5871,7 @@ static int sysfs_slab_alias(struct kmem_c= ache *s, const char *name) > > > > > > al->name =3D name; > > > > > > al->next =3D alias_list; > > > > > > alias_list =3D al; > > > > > > + kmsan_unpoison_shadow(al, sizeof(struct saved_alias)); > > > > > > return 0; > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > --=20 Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Stra=C3=9Fe, 33 80636 M=C3=BCnchen Gesch=C3=A4ftsf=C3=BChrer: Paul Manicle, Halimah DeLaine Prado Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg