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=-17.6 required=3.0 tests=BAYES_00,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, 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 5BA07C433E1 for ; Wed, 12 Aug 2020 14:13:30 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 0BA6620885 for ; Wed, 12 Aug 2020 14:13:29 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="ZyrDNv/E" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0BA6620885 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 69F0A8D001D; Wed, 12 Aug 2020 10:13:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 628A28D0001; Wed, 12 Aug 2020 10:13:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5183B8D001D; Wed, 12 Aug 2020 10:13:29 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0071.hostedemail.com [216.40.44.71]) by kanga.kvack.org (Postfix) with ESMTP id 35BE28D0001 for ; Wed, 12 Aug 2020 10:13:29 -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 A1E432DFD for ; Wed, 12 Aug 2020 14:13:28 +0000 (UTC) X-FDA: 77142109296.04.self33_3c0f5fe26fec Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin04.hostedemail.com (Postfix) with ESMTP id D6DDD80081E7 for ; Wed, 12 Aug 2020 14:13:23 +0000 (UTC) X-HE-Tag: self33_3c0f5fe26fec X-Filterd-Recvd-Size: 9172 Received: from mail-oi1-f195.google.com (mail-oi1-f195.google.com [209.85.167.195]) by imf27.hostedemail.com (Postfix) with ESMTP for ; Wed, 12 Aug 2020 14:13:23 +0000 (UTC) Received: by mail-oi1-f195.google.com with SMTP id z22so1947800oid.1 for ; Wed, 12 Aug 2020 07:13:23 -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; bh=Eny7GtW3fheHTjAaLnI+U+IUiSf5L2wZMby5TEt/uUU=; b=ZyrDNv/EFXrsfT2cLSeHIYizOdkcvsp1j7OBQ1aowLsdKMYwu3Erh3AfIT0w3ioDCQ KfZPUlQPzSoP+nUFYYYO75TVc/gwd5Pb29VR2AvJEi/zoQ05+n/MwJ7Ce5uYS3rN6mC5 cXGuavRO/u9qGk+OlTcMvNTTf0BH+S3ny3O8O6wGekHkSvY7XaEXxoreGvasTdQuVLLN 9k5KaqttiFGfMl0dKXpl/MS6KmuVwDc0i/TlFUM9bFFdvPTgN2wKkSauEFJWPUhuv+Sh JgKs2SJ/+byaCu7G78QKTwAlQ2T+UHsNjxsoHrSWI9ihc6q1Ecwt9WthXZ8Q5RUtMJ98 Ybrg== 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; bh=Eny7GtW3fheHTjAaLnI+U+IUiSf5L2wZMby5TEt/uUU=; b=clz0jl6F9Dj/yMypiA8MlxZP6U8uxpDxTNTCvOuGShWEKlWpQrtMDzCXMPMJZnHB4W 8ug/LYblCphkUN/H/HERlF/KZIXyhangSsdXObI0zWmofv7V8inbL7aoLA5o/+o3+xQz N28y+CNlwalo/vyKKTDEsftYegwDiTTLhE2O6OxlxqCyXHfYUZq2nK8ukZsrarFUlc8+ 8ESgRgFon8fW6q8Iyd2qzgGf163QLoYY2dz1YKUTmooOJWr47Zi5OUFIpPq+LpcNbjT9 LEuZ1JAkdQCfMtemOG2t0JUI8hEtykD2plSv77M4D/nWAVMGhuaCyMoNT1P4nbHaP0Su pDHQ== X-Gm-Message-State: AOAM532jkTdrvDyjNDXtoReKH7CrtGTJb1CkAkJlSGZ0vI1eCezPBneW wc3+f6Gq2OtT1IIDPm24eLEYN5uKrKIpMR/hgvETjA== X-Google-Smtp-Source: ABdhPJw85SLSUV6JhAygRCX/l0A0lj6KIvRKkfbUSL+xaRgnaDhfrkRyLSy4hvJ32Tjv/Nn70me/rXN2+2i6JwfXQLM= X-Received: by 2002:aca:d4d5:: with SMTP id l204mr7728838oig.70.1597241602391; Wed, 12 Aug 2020 07:13:22 -0700 (PDT) MIME-Version: 1.0 References: <20200810072313.529-1-walter-zh.wu@mediatek.com> In-Reply-To: <20200810072313.529-1-walter-zh.wu@mediatek.com> From: Marco Elver Date: Wed, 12 Aug 2020 16:13:09 +0200 Message-ID: Subject: Re: [PATCH 1/5] timer: kasan: record and print timer stack To: Walter Wu Cc: Andrey Ryabinin , Alexander Potapenko , Dmitry Vyukov , Matthias Brugger , John Stultz , Stephen Boyd , Andrew Morton , kasan-dev , Linux Memory Management List , LKML , Linux ARM , wsd_upstream , linux-mediatek@lists.infradead.org, Thomas Gleixner Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: D6DDD80081E7 X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam05 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 Mon, 10 Aug 2020 at 09:23, Walter Wu wrote: > This patch records the last two timer queueing stacks and prints > up to 2 timer stacks in KASAN report. It is useful for programmers > to solve use-after-free or double-free memory timer issues. > > When timer_setup() or timer_setup_on_stack() is called, then it > prepares to use this timer and sets timer callback, we store > this call stack in order to print it in KASAN report. > > Signed-off-by: Walter Wu > Cc: Andrey Ryabinin > Cc: Dmitry Vyukov > Cc: Alexander Potapenko > Cc: Thomas Gleixner > Cc: John Stultz > Cc: Stephen Boyd > Cc: Andrew Morton > --- > include/linux/kasan.h | 2 ++ > kernel/time/timer.c | 2 ++ > mm/kasan/generic.c | 21 +++++++++++++++++++++ > mm/kasan/kasan.h | 4 +++- > mm/kasan/report.c | 11 +++++++++++ > 5 files changed, 39 insertions(+), 1 deletion(-) I'm commenting on the code here, but it also applies to patch 2/5, as it's almost a copy-paste. In general, I'd say the solution to get this feature is poorly designed, resulting in excessive LOC added. The logic added already exists for the aux stacks. > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > index 23b7ee00572d..763664b36dc6 100644 > --- a/include/linux/kasan.h > +++ b/include/linux/kasan.h > @@ -175,12 +175,14 @@ static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; } > void kasan_cache_shrink(struct kmem_cache *cache); > void kasan_cache_shutdown(struct kmem_cache *cache); > void kasan_record_aux_stack(void *ptr); > +void kasan_record_tmr_stack(void *ptr); > > #else /* CONFIG_KASAN_GENERIC */ > > static inline void kasan_cache_shrink(struct kmem_cache *cache) {} > static inline void kasan_cache_shutdown(struct kmem_cache *cache) {} > static inline void kasan_record_aux_stack(void *ptr) {} > +static inline void kasan_record_tmr_stack(void *ptr) {} It appears that the 'aux' stack is currently only used for call_rcu stacks, but this interface does not inherently tie it to call_rcu. The only thing tying it to call_rcu is the fact that the report calls out call_rcu. > /** > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c > index 4b3cbad7431b..f35dcec990ab 100644 > --- a/mm/kasan/generic.c > +++ b/mm/kasan/generic.c > @@ -347,6 +347,27 @@ void kasan_record_aux_stack(void *addr) > alloc_info->aux_stack[0] = kasan_save_stack(GFP_NOWAIT); > } > > +void kasan_record_tmr_stack(void *addr) > +{ > + struct page *page = kasan_addr_to_page(addr); > + struct kmem_cache *cache; > + struct kasan_alloc_meta *alloc_info; > + void *object; > + > + if (!(page && PageSlab(page))) > + return; > + > + cache = page->slab_cache; > + object = nearest_obj(cache, page, addr); > + alloc_info = get_alloc_info(cache, object); > + > + /* > + * record the last two timer stacks. > + */ > + alloc_info->tmr_stack[1] = alloc_info->tmr_stack[0]; > + alloc_info->tmr_stack[0] = kasan_save_stack(GFP_NOWAIT); > +} The solution here is, unfortunately, poorly designed. This is a copy-paste of the kasan_record_aux_stack() function. > void kasan_set_free_info(struct kmem_cache *cache, > void *object, u8 tag) > { > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h > index ef655a1c6e15..c50827f388a3 100644 > --- a/mm/kasan/kasan.h > +++ b/mm/kasan/kasan.h > @@ -108,10 +108,12 @@ struct kasan_alloc_meta { > struct kasan_track alloc_track; > #ifdef CONFIG_KASAN_GENERIC > /* > - * call_rcu() call stack is stored into struct kasan_alloc_meta. > + * call_rcu() call stack and timer queueing stack are stored > + * into struct kasan_alloc_meta. > * The free stack is stored into struct kasan_free_meta. > */ > depot_stack_handle_t aux_stack[2]; > + depot_stack_handle_t tmr_stack[2]; > #else > struct kasan_track free_track[KASAN_NR_FREE_STACKS]; > #endif > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > index fed3c8fdfd25..6fa3bfee381f 100644 > --- a/mm/kasan/report.c > +++ b/mm/kasan/report.c > @@ -191,6 +191,17 @@ static void describe_object(struct kmem_cache *cache, void *object, > print_stack(alloc_info->aux_stack[1]); > pr_err("\n"); > } > + > + if (alloc_info->tmr_stack[0]) { > + pr_err("Last timer stack:\n"); > + print_stack(alloc_info->tmr_stack[0]); > + pr_err("\n"); > + } > + if (alloc_info->tmr_stack[1]) { > + pr_err("Second to last timer stack:\n"); > + print_stack(alloc_info->tmr_stack[1]); > + pr_err("\n"); > + } Why can't we just use the aux stack for everything, and simply change the message printed in the report. After all, the stack trace will include all the information to tell if it's call_rcu, timer, or workqueue. The reporting code would simply have to be changed to say something like "Last potentially related work creation:" -- because what the "aux" thing is trying to abstract are stack traces to work creations that took an address that is closely related. Whether or not you want to call it "work" is up to you, but that's the most generic term I could think of right now (any better terms?). Another argument for this consolidation is that it's highly unlikely that aux_stack[a] && tmr_stack[b] && wq_stack[c], and you need to print all the stacks. If you are worried we need more aux stacks, just make the array size 3+ (but I think it's not necessary). Thanks, -- Marco