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=-8.3 required=3.0 tests=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_AGENT_SANE_2 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 F1FD6CA9EA0 for ; Fri, 18 Oct 2019 16:08:26 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 9644A21D7C for ; Fri, 18 Oct 2019 16:08:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lca.pw header.i=@lca.pw header.b="gXpRhs/u" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9644A21D7C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=lca.pw Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 38C4C8E0007; Fri, 18 Oct 2019 12:08:26 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 362D98E0003; Fri, 18 Oct 2019 12:08:26 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 279308E0007; Fri, 18 Oct 2019 12:08:26 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0194.hostedemail.com [216.40.44.194]) by kanga.kvack.org (Postfix) with ESMTP id F179B8E0003 for ; Fri, 18 Oct 2019 12:08:25 -0400 (EDT) Received: from smtpin14.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with SMTP id 88A044DC9 for ; Fri, 18 Oct 2019 16:08:25 +0000 (UTC) X-FDA: 76057387770.14.news74_83afc8a30c017 X-HE-Tag: news74_83afc8a30c017 X-Filterd-Recvd-Size: 17799 Received: from mail-qk1-f194.google.com (mail-qk1-f194.google.com [209.85.222.194]) by imf10.hostedemail.com (Postfix) with ESMTP for ; Fri, 18 Oct 2019 16:08:24 +0000 (UTC) Received: by mail-qk1-f194.google.com with SMTP id e66so5743553qkf.13 for ; Fri, 18 Oct 2019 09:08:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=lca.pw; s=google; h=message-id:subject:from:to:cc:date:in-reply-to:references :mime-version:content-transfer-encoding; bh=IuRt9r4I68t5tWVfcW4JcDhBp8eRgyRRWFvmbLT+bwU=; b=gXpRhs/uU318065JAku5Fl3Zd+QYzY9NiQvmj7ZoSpCRFJk2OcmNe8AE0bhKCIKYP7 bGA8qdOluc65JnzJP7szG80zXDFvIgX8iHhCm9bs2wKwPg21bwwIe0pAg2iLSeIf4kKw fNpPue7Bk7CuqAh6MzCvs28duQaVWI9SrJUcqng7FAiQS+0n8w3OezPy5Zye+yWkD2CF 4h9aFmh+5AJrvqU4WSus2otdeqgsGeh7W26qcyxsRdbuEvLKjBSJ/7tlwK2MxszBB441 EdlF1UIfDbYcWKvp9TNyO4xV/61yiKiwdmZCSPqwSV98doaqRA/8n1tumgupaxmCR+mu WXvg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:mime-version:content-transfer-encoding; bh=IuRt9r4I68t5tWVfcW4JcDhBp8eRgyRRWFvmbLT+bwU=; b=H4Keqpig/KbkmsnsWls3iHC9P3imQvfNsjuVBDOkVjVy3F90Y75g/N7HE8MIP04qAb xgSTli8nBkjNi7mE2o56zLu5uBlURGW6Lsvu8eU2jpYItKEKpqDhK2MKdFcJQebzQ4DW Utz6+XcCp2A4PMTixsSMY6i6bNATC9bQDegHiNxFqTLgb2+HUkq8sbFF/uqurZEt8jq7 RVlwilxDNxQdTQqRESbSyacH+OFNEHTIYjLgKmVANMXaHKLa75A9RJJ4oEoksyPdq6Hk m7RD9b13MepZeDHoabCMJw6gnPCNn7l6yJIb3i9Y87Zo5PeqzHaeLNGEwrldcwVghl9O ohtA== X-Gm-Message-State: APjAAAXhacPvtswKohAuFqLPn9/2R5VmHGYv+ejJdmluBtwrX1JcUDCi nK9O8kBtAKEAWHCqXI5PGlr2s7lmxO0= X-Google-Smtp-Source: APXvYqwew8TBqCNa7L/Kod1XY6e41WxJU52ZbN++mwAHbcHE6b0kNcGdqgZW68Ip+hRWiUf9oJz6HA== X-Received: by 2002:a05:620a:12c7:: with SMTP id e7mr9384881qkl.162.1571414904094; Fri, 18 Oct 2019 09:08:24 -0700 (PDT) Received: from dhcp-41-57.bos.redhat.com (nat-pool-bos-t.redhat.com. [66.187.233.206]) by smtp.gmail.com with ESMTPSA id x203sm3360611qkb.11.2019.10.18.09.08.22 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 18 Oct 2019 09:08:23 -0700 (PDT) Message-ID: <1571414901.5937.82.camel@lca.pw> Subject: Re: [PATCH RFC v1 18/26] kmsan: mm: call KMSAN hooks from SLUB code From: Qian Cai To: Alexander Potapenko Cc: Andrew Morton , Vegard Nossum , Dmitry Vyukov , Linux Memory Management List Date: Fri, 18 Oct 2019 12:08:21 -0400 In-Reply-To: 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> <1571411587.5937.80.camel@lca.pw> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6 (3.22.6-10.el7) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit 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, 2019-10-18 at 17:30 +0200, Alexander Potapenko wrote: > On Fri, Oct 18, 2019 at 5:13 PM Qian Cai wrote: > > > > On Fri, 2019-10-18 at 16:54 +0200, Alexander Potapenko wrote: > > > 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 allocations > > > > > > > > > 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 checks. 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 objects 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 corruption, SLUB > > > > debug should be detected as well as this patch. If not, it needs to understand > > > > 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. > > > > Well, SLUB debug would mark any access of uninitialized memory as data > > corruption with the help of poisoning and red zoning. > > > > It is true that KMSAN might be doing a bit more than SLUB debug here, but the > > question is if it is worth the maintenance burden? Do you have any existing bugs > > to show that KMSAN would find by this patch but SLUB debug can't? > > Consider the following case: > > int *a = kmalloc(sizeof(int), GFP_KERNEL); > if (*a) { > // do something, e.g. become root > } > > IIUC SLUB poisoning will result in the branch being taken, but as long > as there are no invalid accesses the kernel won't crash and no errors > will be reported. > KMSAN however will report an error, because the contents of the buffer > pointed to by |a| are poisoned by kmsan_slab_alloc(). > The compiler instrumentation will retrieve the shadow value for *a, > compare it to zero and report a use of uninitialized memory. > > Another example would be copying the contents of newly allocated > buffer to the userspace. Make sense. BTW, it might worth testing this patchset with this MM debug config based on linux-next to edge out some corner cases if you could, https://raw.githubusercontent.com/cailca/linux-mm/master/x86.config > > > > > > > > > > > > > > > > > > > > > > > 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 struct 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 force 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 = (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_addr)); > > > > > > > > > } > > > > > > > > > > > > > > > > > > static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp) > > > > > > > > > @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags) > > > > > > > > > ptr = kasan_kmalloc_large(ptr, size, flags); > > > > > > > > > /* As ptr might get tagged, call kmemleak hook after KASAN. */ > > > > > > > > > kmemleak_alloc(ptr, size, 1, flags); > > > > > > > > > + kmsan_kmalloc_large(ptr, size, flags); > > > > > > > > > return ptr; > > > > > > > > > } > > > > > > > > > > > > > > > > > > @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hook(void *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_hook(struct kmem_cache *s, > > > > > > > > > } while (object != old_tail); > > > > > > > > > } > > > > > > > > > > > > > > > > > > + do { > > > > > > > > > + object = next; > > > > > > > > > + next = get_freepointer(s, object); > > > > > > > > > + kmsan_slab_free(s, object); > > > > > > > > > + } while (object != old_tail); > > > > > > > > > + > > > > > > > > > /* > > > > > > > > > * Compiler cannot detect this function can be removed if slab_free_hook() > > > > > > > > > * evaluates to nothing. Thus, catch all relevant config debug options here. > > > > > > > > > @@ -2769,6 +2789,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s, > > > > > > > > > if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object) > > > > > > > > > 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 = slab_alloc(s, gfpflags, _RET_IP_); > > > > > > > > > trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags); > > > > > > > > > ret = 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_cache *s, gfp_t gfpflags, int node) > > > > > > > > > > > > > > > > > > trace_kmem_cache_alloc_node(_RET_IP_, ret, > > > > > > > > > s->object_size, s->size, gfpflags, 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 = 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_cache *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 = slab_pre_alloc_hook(s, flags); > > > > > > > > > @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *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 = 0; j < i; j++) > > > > > > > > > memset(p[j], 0, s->object_size); > > > > > > > > > } > > > > > > > > > + for (j = 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=", 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 += 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_cache *s, const char *name) > > > > > > > > > al->name = name; > > > > > > > > > al->next = alias_list; > > > > > > > > > alias_list = al; > > > > > > > > > + kmsan_unpoison_shadow(al, sizeof(struct saved_alias)); > > > > > > > > > return 0; > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >