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=-23.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,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 AE486C433ED for ; Thu, 22 Apr 2021 09:58:36 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2A0476144D for ; Thu, 22 Apr 2021 09:58:36 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2A0476144D 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 9047C6B006E; Thu, 22 Apr 2021 05:58:35 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8DB586B0070; Thu, 22 Apr 2021 05:58:35 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7A39D6B0071; Thu, 22 Apr 2021 05:58:35 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0176.hostedemail.com [216.40.44.176]) by kanga.kvack.org (Postfix) with ESMTP id 5C2AA6B006E for ; Thu, 22 Apr 2021 05:58:35 -0400 (EDT) Received: from smtpin26.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 0DCA71802E60A for ; Thu, 22 Apr 2021 09:58:35 +0000 (UTC) X-FDA: 78059553390.26.835EBCF Received: from mail-qt1-f173.google.com (mail-qt1-f173.google.com [209.85.160.173]) by imf03.hostedemail.com (Postfix) with ESMTP id 0BA73C0007D4 for ; Thu, 22 Apr 2021 09:58:29 +0000 (UTC) Received: by mail-qt1-f173.google.com with SMTP id d6so15063418qtx.13 for ; Thu, 22 Apr 2021 02:58:34 -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=sVnJbmEkfxXnJEPDJlf6F38sWKPScLpz9CDnrIopbyM=; b=TYedG1fJacOD+Bb2QwT7pOSedLhckqpyaM0TQ7zNMtmFymonWhNGx6n+SCybHM7wQl vzx52vqKLK2h2i8TmhMnXI++DcjCn1EAiUxqh9sg1tH/d3AlIfExRhooaIjGLQBAnjnC rvoWcAkXVIyMev4qGuGtMx7TPcNrWFp+eM9oSuZxLs8AiXK3a44jiCgz6BoZ4WaxJ3T0 TTFULShEdpyq7JmNnZHkA+XUvZ4KpularJ70h7K9j+jFEXywTLkrQ+EzalCs5xrXM7Cf NX1wuP05h8WWOFsFbC0ezeu8OfwoGi/IQnL5GGHJWZQsA4I7qjQmR+XSD12pyKINfFnd aGnw== 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=sVnJbmEkfxXnJEPDJlf6F38sWKPScLpz9CDnrIopbyM=; b=bjjClaQ41rX8c/Q5BxApwNJrYfkTmHktm1lTNUmOfNykr4ZJGIB/ByA0yYz9qMXKj9 ZeZlzVjkUlpqIWpErVltgPH5XXuR1zpg5nJhlZzC1B5aUDKYV/mNn6X4K6Ci+8a7l6N3 SEy1e4Lbj7po2KWBfmNpBIb6SoMa8uWi/n8g5cLwJyEhxLyG1ZY2+nkavjasAQ410qOL 4Pgp8Mow8tVIJtvk+sTb+DWSisx8kHGNacYnOoKoJxR//PcqoZYh2tGF7loVSjbMRk8/ AV5gxew7ngQDpSsw9fzFWjFqQWHkz1M9gaXcgnDzEDzSE2ZVZ1u+z2swd71UTs43g/Il CnOQ== X-Gm-Message-State: AOAM531ZRhFEsDh5Ly2luRybzPqX3xJ9J8uT/V95cnyYrw6jbn+/EurV /DJ4WF5xhWEy+XqUeYU+UKWRdO+fKeDnycdOs6uiXw== X-Google-Smtp-Source: ABdhPJyJ8oT1MqeUdOl5WYqQqJBPncgbwFNVWPDlaBH7Gtho0a7YHGMvC3uCjojGUnpUotMf6LpoilEeGPbiM39X5EM= X-Received: by 2002:ac8:5c92:: with SMTP id r18mr2284877qta.66.1619085513632; Thu, 22 Apr 2021 02:58:33 -0700 (PDT) MIME-Version: 1.0 References: <1619079317-1131-1-git-send-email-maninder1.s@samsung.com> In-Reply-To: <1619079317-1131-1-git-send-email-maninder1.s@samsung.com> From: Dmitry Vyukov Date: Thu, 22 Apr 2021 11:58:22 +0200 Message-ID: Subject: Re: [PATCH 1/2] mm/kasan: avoid duplicate KASAN issues from reporting To: Maninder Singh Cc: Andrey Ryabinin , Alexander Potapenko , Andrey Konovalov , Andrew Morton , kasan-dev , Linux-MM , LKML , AMIT SAHRAWAT , Vaneet Narang Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 0BA73C0007D4 X-Stat-Signature: q356oybzgzwwkkz8da49jccho6acq5ay Received-SPF: none (google.com>: No applicable sender policy available) receiver=imf03; identity=mailfrom; envelope-from=""; helo=mail-qt1-f173.google.com; client-ip=209.85.160.173 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1619085509-475696 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 Thu, Apr 22, 2021 at 11:17 AM Maninder Singh wrote: > > when KASAN multishot is ON and some buggy code hits same code path > of KASAN issue repetetively, it can flood logs on console. > > Check for allocaton, free and backtrace path at time of KASAN error, > if these are same then it is duplicate error and avoid these prints > from KASAN. > > Co-developed-by: Vaneet Narang > Signed-off-by: Vaneet Narang > Signed-off-by: Maninder Singh > --- > mm/kasan/kasan.h | 6 +++++ > mm/kasan/report.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 73 insertions(+) > > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h > index 78cf99247139..d14ccce246ba 100644 > --- a/mm/kasan/kasan.h > +++ b/mm/kasan/kasan.h > @@ -102,6 +102,12 @@ struct kasan_access_info { > unsigned long ip; > }; > > +struct kasan_record { > + depot_stack_handle_t bt_handle; > + depot_stack_handle_t alloc_handle; > + depot_stack_handle_t free_handle; > +}; Hi Maninder, There is no need to declare this in the header, it can be declared more locally in report.h. > + > /* The layout of struct dictated by compiler */ > struct kasan_source_location { > const char *filename; > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > index 87b271206163..4576de76991b 100644 > --- a/mm/kasan/report.c > +++ b/mm/kasan/report.c > @@ -39,6 +39,10 @@ static unsigned long kasan_flags; > #define KASAN_BIT_REPORTED 0 > #define KASAN_BIT_MULTI_SHOT 1 > > +#define MAX_RECORDS (200) s/MAX_RECORDS/KASAN_MAX_RECORDS/ > +static struct kasan_record kasan_records[MAX_RECORDS]; Since all fields in kasan_record are stack handles, the code will be simpler and more uniform, if we store just an array of handles w/o distinguishing between alloc/free/access. > +static int stored_kasan_records; > + > bool kasan_save_enable_multi_shot(void) > { > return test_and_set_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags); > @@ -360,6 +364,65 @@ void kasan_report_invalid_free(void *object, unsigned long ip) > end_report(&flags, (unsigned long)object); > } > > +/* > + * @save_report() > + * > + * returns false if same record is already saved. s/same/the same/ > + * returns true if its new record and saved in database of KASAN. s/its/it's/ s/database/the database/ > + */ > +static bool save_report(void *addr, struct kasan_access_info *info, u8 tag, unsigned long *flags) > +{ > + struct kasan_record record = {0}; > + depot_stack_handle_t bt_handle; > + int i = 0; > + const char *bug_type; > + struct kasan_alloc_meta *alloc_meta; > + struct kasan_track *free_track; > + struct page *page; > + bool ret = true; > + > + kasan_disable_current(); > + spin_lock_irqsave(&report_lock, *flags); Reusing the caller flags looks strange, do we need it? But also the very next function start_report() also does the same dance: kasan_disable_current/spin_lock_irqsave. It feels reasonable to lock once, check for dups and return early if it's a dup. > + bug_type = kasan_get_bug_type(info); > + page = kasan_addr_to_page(addr); > + bt_handle = kasan_save_stack(GFP_KERNEL); ASsign directly to record.bt_handle. > + if (page && PageSlab(page)) { > + struct kmem_cache *cache = page->slab_cache; > + void *object = nearest_obj(cache, page, addr); Since you already declare new var in this block, move alloc_meta/free_track here as well. > + > + alloc_meta = kasan_get_alloc_meta(cache, object); > + free_track = kasan_get_free_track(cache, object, tag); > + record.alloc_handle = alloc_meta->alloc_track.stack; > + if (free_track) > + record.free_handle = free_track->stack; > + } > + > + record.bt_handle = bt_handle; > + > + for (i = 0; i < stored_kasan_records; i++) { > + if (record.bt_handle != kasan_records[i].bt_handle) > + continue; > + if (record.alloc_handle != kasan_records[i].alloc_handle) > + continue; > + if (!strncmp("use-after-free", bug_type, 15) && Comparing strings is unreliable and will break in future. Compare handle with 0 instead, you already assume that 0 handle is "no handle". > + (record.free_handle != kasan_records[i].free_handle)) > + continue; > + > + ret = false; > + goto done; > + } > + > + memcpy(&kasan_records[stored_kasan_records], &record, sizeof(struct kasan_record)); > + stored_kasan_records++; I think you just introduced an out-of-bounds write into KASAN, check for MAX_RECORDS ;) > + > +done: > + spin_unlock_irqrestore(&report_lock, *flags); > + kasan_enable_current(); > + return ret; > +} > + > static void __kasan_report(unsigned long addr, size_t size, bool is_write, > unsigned long ip) > { > @@ -388,6 +451,10 @@ static void __kasan_report(unsigned long addr, size_t size, bool is_write, > info.is_write = is_write; > info.ip = ip; > > + if (addr_has_metadata(untagged_addr) && Why addr_has_metadata check? The kernel will probably crash later anyway, but from point of view of this code, I don't see reasons to not dedup wild accesses. > + !save_report(untagged_addr, &info, get_tag(tagged_addr), &flags)) > + return; > + > start_report(&flags); > > print_error_description(&info); > -- > 2.17.1 >