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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id AFCB3C47258 for ; Thu, 25 Jan 2024 22:36:17 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 472106B009B; Thu, 25 Jan 2024 17:36:17 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 420EB6B009D; Thu, 25 Jan 2024 17:36:17 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2C1236B009E; Thu, 25 Jan 2024 17:36:17 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 183276B009B for ; Thu, 25 Jan 2024 17:36:17 -0500 (EST) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id DD16F1A0AA4 for ; Thu, 25 Jan 2024 22:36:16 +0000 (UTC) X-FDA: 81719293152.01.1100F20 Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by imf25.hostedemail.com (Postfix) with ESMTP id 14062A0004 for ; Thu, 25 Jan 2024 22:36:14 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=QQiQeV10; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf25.hostedemail.com: domain of andreyknvl@gmail.com designates 209.85.128.41 as permitted sender) smtp.mailfrom=andreyknvl@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1706222175; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=abiH/PhhRDGXkyL1IB+beHQOnSwz7Jg04l0sv5fMRUk=; b=4JPbAAVdB8n5DK5uxYEy5LaulHMVeINeeNRRRmBUFDKE2DKZ5GjYD24O53dK4Zooh6NWfn Rruw94/EfaQ84NtHK6gfJmXDOPbbWd9GldNq0iN1WfGoYohFYddUjdFniSm+JvqvAQP41K dQ4DJLHaIXtTX/CE/bxZjtbA99MRMpw= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=QQiQeV10; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf25.hostedemail.com: domain of andreyknvl@gmail.com designates 209.85.128.41 as permitted sender) smtp.mailfrom=andreyknvl@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1706222175; a=rsa-sha256; cv=none; b=RrFT52159Ox2AtQMvvSowdh492cPT4NA/ID/IVy4/1EP+sP7UGRSzGeaAVxpnBclBo8Uw7 GQBNC1O264S+BiFluh0Ok0p6gVJYDxvlWzTXF85YNDZU7ZMqZqdBE1KXU1vMtJ05gGyF3M 5ZWaXJtGcI63Cv6TL1OwJLBqAXCalYg= Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-40e5afc18f5so78176685e9.3 for ; Thu, 25 Jan 2024 14:36:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1706222173; x=1706826973; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=abiH/PhhRDGXkyL1IB+beHQOnSwz7Jg04l0sv5fMRUk=; b=QQiQeV10yo723rqtj1hkwXdEa5UTYliEZdg7IcnmZ9ttrl3hCjmHLYiaz1cE4x4UGJ J8tA2geif3FMEgCwH9HYQiaB67zLvi388krI7Whn2O0uyUg7VtBkovQH1VDNvGeZGjdW d8/dmqeIbZgPVuULqfAt/pMOpDV76/IAAyRSFdfxSdWhwVWH4wQh8m9b/XatzveYd0Fv E7CAEwYkDCUQ4LuMo8xeIt2uWbi+MgvFhz+3+cLHjGZ48BsISFgwH4R/tNwuV+SizMBS sg0LELzi225t/IO6R6bJBSJ4KFcqeE0NWKzzCuFG9GorFixmv7PuKEXHP9daU3sxq09N GrZg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706222173; x=1706826973; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=abiH/PhhRDGXkyL1IB+beHQOnSwz7Jg04l0sv5fMRUk=; b=e+RyMzzmMBTJYHhe1tRP8kc+5Bc9o8+EGcI83kpy/QrkUWsSFOSGX4AM0UxHGXwWZH tVqgZhIRSx9t+Pa6dDHICN+IiMtIERD6btCO6hjfXK19TCflBkfgvgowS50qfLfTJIOb 5qOBZdLBUM0BbnqweE966lgTYLm8wvI8bPeO669onunzMLvks5XDEwwkZ3HUxJZ9n/CC 1uftn3gGB9M7/s8G6NcEGd2avLPAmXAmtGDFVuJN4t5VomoOktVsR6eS8w4ve1COak5H nE1iktNFV3J2SWu37yniqzR/ewLcZXO0RBpGVKWGYXJ+26W3+RKedGiX5xnBQFNYtCxK TuAw== X-Gm-Message-State: AOJu0Yy9aEs2L8Lszq+Ie39UULKGGum/FbEOlCo5Wg0I+S5xfDsErCsA m7/KqtfXYD2ZlzjiBYcFktd7IJXCpZRzvOvmB+rrtDNoKojWI/+03AUmTsdAXtn8TnJsEIACbSu GX8/RDkfO5mRUgL1EfNzxFe4w3cTSLCWI X-Google-Smtp-Source: AGHT+IFa+Iin8gQ7TonNMFfz+ijkfFJZFx6vaYfznOHKeOAlCjnCkuFepmMP6ml3Fguv76JEt8aigeo6j0mFkqHIfZM= X-Received: by 2002:a5d:66c7:0:b0:33a:eb3:d8e9 with SMTP id k7-20020a5d66c7000000b0033a0eb3d8e9mr297880wrw.83.1706222173438; Thu, 25 Jan 2024 14:36:13 -0800 (PST) MIME-Version: 1.0 References: <20240125094815.2041933-1-elver@google.com> <20240125094815.2041933-2-elver@google.com> In-Reply-To: <20240125094815.2041933-2-elver@google.com> From: Andrey Konovalov Date: Thu, 25 Jan 2024 23:36:02 +0100 Message-ID: Subject: Re: [PATCH 2/2] kasan: revert eviction of stack traces in generic mode To: Marco Elver Cc: Andrew Morton , Alexander Potapenko , Dmitry Vyukov , Vlastimil Babka , Andrey Ryabinin , Vincenzo Frascino , linux-kernel@vger.kernel.org, kasan-dev@googlegroups.com, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 14062A0004 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: wxqznmtdb8sbr4r431gcd6z53osze3dj X-HE-Tag: 1706222174-507910 X-HE-Meta: U2FsdGVkX18Wc81p6h7PIvXZNZ+PPaoSu7YEFYpvO/9GEC0bIW81i+1vBh75yPHOyxAJKqpmfzsQ1x6LrArERfndEqNUHGiPbdCbvrINut6bDo0k6oZL1zTYzqKTbQ4y1Uf9KmFUnR/Tp6gUSHavsvEgwu7FEtpUMqWu52z2aJS9tHmV4Q/jPfS2VvjQPD0ewSkNVAU1n6xnXPP9ohYnDH8J95Emth2kQ9Y2OQI4/ndxsuBsQwcROv37sd5WPfKU2EgBfFZwwbSJBXWITIoLSyNwrCNkEGDWxGYlHRBdJOnuOxAwgff75lh0o9kxJcN2cNq/uB/H7a6RygM0kRLXZKHXQGWyGuYReRl1GClLlg2RYt+Rs7ugYgcXK6ftaTermOvW65tp5IYPZpbLL1qB+zGhTcSEx0kG/NJgvSuOdhno+aV09yUvVteaz/dIsbU70Eh0Ke3HAqVCY9AjZabua6N2RZGFqTcdph+lhjEbO/ODCn89Q8qvleAs1SU6rXCbzV1dE9HDW9LWPl4YG26cdHgAQe7JN82ekd4CCt0t1KSzH3jDp8/ttAHdpw9BCEziEMcaR79BZ2QnW/4b/9M1V9cJ4oEFqRijrBYz/RPgjFP1ElRORaaAvLCGlTyLnci8Aeefdjz34haBZtUnDZ6F7x6iX3TTSf7sNIWv/Mia9wSsL8hCQS6XvVZIQZ+8lWx8e1LxWwK1t8JIIOPQYAclzBLpAeDahRb8Mfp7hQmpHVv+OPJ9HPVnl9sWt8U4XFZHxHxcX6hvKbKcG/z1eX95SBmeZHWzw4ibR0b3OqilPe6PnqbTi+n6pc4lFpyu3SCI7YRacdWdkWeoChz1pHek7BvuRzX6LokZOR8Y1Y6FBHrjFZSovhSlT3OMO016ck0RezHHEnJhiBSYkPWGlJws+p3iGxSXPDx6BEEPEy5J2qFBkDiKd3SOYiiGgs8ZLBo3jZw/pqRGRY441lcLuWN jjD6HXR3 Sd9aEWCGPMM7n2TCRkHN4UohM/WL6q9QBhkksVEyPjBoCTIPA0yhH/nsaU4s6wQdqEclAd3tPkOlLr07BzqtVvCXPjQlnRbgiVEDqlTs4IlABVMkkLeT1naYsFPi3UjaBDxKcGml5wkcUigD79wnPm+dl01Z5Rd2SrBo3jr5faTbu8FDv9iLKlvU+Y7Lt+y4sQ/Z15mf0rE0/MtwX3lA/Jhy7UEkLiTjk0ppRZbzk0U5sSptnn+c61E3S9gESA6ycWPKRxPZkPCMEdvMHxmH/kZ4T1wvgIafDnw+QD/kmV77Bmr7xJnH5/LV28zpiFXkpNog8E1Ms9cPhFGEGdqmUyYcNnRcC4q8v+1vJpZNNqSWQ0wr2ZhZIRQQrgDukB+csYosi7/Qik6h2egxOCK5J/yG83FE8POX8zklr5p9hH6vn/LD/zkuHs3SyIYbtoCzqPqPfUKpwXIAyZNx0p9zgDXi06w== 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: List-Subscribe: List-Unsubscribe: On Thu, Jan 25, 2024 at 10:48=E2=80=AFAM Marco Elver wro= te: > > This partially reverts commits cc478e0b6bdf, 63b85ac56a64, 08d7c94d9635, > a414d4286f34, and 773688a6cb24 to make use of variable-sized stack depot > records, since eviction of stack entries from stack depot forces fixed- > sized stack records. Care was taken to retain the code cleanups by the > above commits. > > Eviction was added to generic KASAN as a response to alleviating the > additional memory usage from fixed-sized stack records, but this still > uses more memory than previously. > > With the re-introduction of variable-sized records for stack depot, we > can just switch back to non-evictable stack records again, and return > back to the previous performance and memory usage baseline. > > Before (observed after a KASAN kernel boot): > > pools: 597 > allocations: 29657 > frees: 6425 > in_use: 23232 > freelist_size: 3493 > > After: > > pools: 315 > allocations: 28964 > frees: 0 > in_use: 28964 > freelist_size: 0 > > As can be seen from the number of "frees", with a generic KASAN config, > evictions are no longer used but due to using variable-sized records, I > observe a reduction of 282 stack depot pools (saving 4512 KiB) with my > test setup. > > Fixes: cc478e0b6bdf ("kasan: avoid resetting aux_lock") > Fixes: 63b85ac56a64 ("kasan: stop leaking stack trace handles") > Fixes: 08d7c94d9635 ("kasan: memset free track in qlink_free") > Fixes: a414d4286f34 ("kasan: handle concurrent kasan_record_aux_stack cal= ls") > Fixes: 773688a6cb24 ("kasan: use stack_depot_put for Generic mode") > Signed-off-by: Marco Elver > Cc: Alexander Potapenko > Cc: Andrey Konovalov > Cc: Dmitry Vyukov > --- > mm/kasan/common.c | 3 +-- > mm/kasan/generic.c | 54 ++++++---------------------------------------- > mm/kasan/kasan.h | 8 ------- > 3 files changed, 8 insertions(+), 57 deletions(-) > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > index 610efae91220..ad32803e34e9 100644 > --- a/mm/kasan/common.c > +++ b/mm/kasan/common.c > @@ -65,8 +65,7 @@ void kasan_save_track(struct kasan_track *track, gfp_t = flags) > { > depot_stack_handle_t stack; > > - stack =3D kasan_save_stack(flags, > - STACK_DEPOT_FLAG_CAN_ALLOC | STACK_DEPOT_FLAG_GET= ); > + stack =3D kasan_save_stack(flags, STACK_DEPOT_FLAG_CAN_ALLOC); > kasan_set_track(track, stack); > } > > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c > index df6627f62402..8bfb52b28c22 100644 > --- a/mm/kasan/generic.c > +++ b/mm/kasan/generic.c > @@ -485,16 +485,6 @@ void kasan_init_object_meta(struct kmem_cache *cache= , const void *object) > if (alloc_meta) { > /* Zero out alloc meta to mark it as invalid. */ > __memset(alloc_meta, 0, sizeof(*alloc_meta)); > - > - /* > - * Prepare the lock for saving auxiliary stack traces. > - * Temporarily disable KASAN bug reporting to allow instr= umented > - * raw_spin_lock_init to access aux_lock, which resides i= nside > - * of a redzone. > - */ > - kasan_disable_current(); > - raw_spin_lock_init(&alloc_meta->aux_lock); > - kasan_enable_current(); > } > > /* > @@ -506,18 +496,8 @@ void kasan_init_object_meta(struct kmem_cache *cache= , const void *object) > > static void release_alloc_meta(struct kasan_alloc_meta *meta) > { > - /* Evict the stack traces from stack depot. */ > - stack_depot_put(meta->alloc_track.stack); > - stack_depot_put(meta->aux_stack[0]); > - stack_depot_put(meta->aux_stack[1]); > - > - /* > - * Zero out alloc meta to mark it as invalid but keep aux_lock > - * initialized to avoid having to reinitialize it when another ob= ject > - * is allocated in the same slot. > - */ > - __memset(&meta->alloc_track, 0, sizeof(meta->alloc_track)); > - __memset(meta->aux_stack, 0, sizeof(meta->aux_stack)); > + /* Zero out alloc meta to mark it as invalid. */ > + __memset(meta, 0, sizeof(*meta)); > } > > static void release_free_meta(const void *object, struct kasan_free_meta= *meta) > @@ -526,9 +506,6 @@ static void release_free_meta(const void *object, str= uct kasan_free_meta *meta) > if (*(u8 *)kasan_mem_to_shadow(object) !=3D KASAN_SLAB_FREE_META) > return; > > - /* Evict the stack trace from the stack depot. */ > - stack_depot_put(meta->free_track.stack); > - > /* Mark free meta as invalid. */ > *(u8 *)kasan_mem_to_shadow(object) =3D KASAN_SLAB_FREE; > } > @@ -571,8 +548,6 @@ static void __kasan_record_aux_stack(void *addr, depo= t_flags_t depot_flags) > struct kmem_cache *cache; > struct kasan_alloc_meta *alloc_meta; > void *object; > - depot_stack_handle_t new_handle, old_handle; > - unsigned long flags; > > if (is_kfence_address(addr) || !slab) > return; > @@ -583,33 +558,18 @@ static void __kasan_record_aux_stack(void *addr, de= pot_flags_t depot_flags) > if (!alloc_meta) > return; > > - new_handle =3D kasan_save_stack(0, depot_flags); > - > - /* > - * Temporarily disable KASAN bug reporting to allow instrumented > - * spinlock functions to access aux_lock, which resides inside of= a > - * redzone. > - */ > - kasan_disable_current(); > - raw_spin_lock_irqsave(&alloc_meta->aux_lock, flags); > - old_handle =3D alloc_meta->aux_stack[1]; > alloc_meta->aux_stack[1] =3D alloc_meta->aux_stack[0]; > - alloc_meta->aux_stack[0] =3D new_handle; > - raw_spin_unlock_irqrestore(&alloc_meta->aux_lock, flags); > - kasan_enable_current(); > - > - stack_depot_put(old_handle); > + alloc_meta->aux_stack[0] =3D kasan_save_stack(0, depot_flags); > } > > void kasan_record_aux_stack(void *addr) > { > - return __kasan_record_aux_stack(addr, > - STACK_DEPOT_FLAG_CAN_ALLOC | STACK_DEPOT_FLAG_GET= ); > + return __kasan_record_aux_stack(addr, STACK_DEPOT_FLAG_CAN_ALLOC)= ; > } > > void kasan_record_aux_stack_noalloc(void *addr) > { > - return __kasan_record_aux_stack(addr, STACK_DEPOT_FLAG_GET); > + return __kasan_record_aux_stack(addr, 0); > } > > void kasan_save_alloc_info(struct kmem_cache *cache, void *object, gfp_t= flags) > @@ -620,7 +580,7 @@ void kasan_save_alloc_info(struct kmem_cache *cache, = void *object, gfp_t flags) > if (!alloc_meta) > return; > > - /* Evict previous stack traces (might exist for krealloc or mempo= ol). */ > + /* Invalidate previous stack traces (might exist for krealloc or = mempool). */ > release_alloc_meta(alloc_meta); > > kasan_save_track(&alloc_meta->alloc_track, flags); > @@ -634,7 +594,7 @@ void kasan_save_free_info(struct kmem_cache *cache, v= oid *object) > if (!free_meta) > return; > > - /* Evict previous stack trace (might exist for mempool). */ > + /* Invalidate previous stack trace (might exist for mempool). */ > release_free_meta(object, free_meta); > > kasan_save_track(&free_meta->free_track, 0); > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h > index d0f172f2b978..216ae0ef1e4b 100644 > --- a/mm/kasan/kasan.h > +++ b/mm/kasan/kasan.h > @@ -6,7 +6,6 @@ > #include > #include > #include > -#include > #include > > #if defined(CONFIG_KASAN_SW_TAGS) || defined(CONFIG_KASAN_HW_TAGS) > @@ -265,13 +264,6 @@ struct kasan_global { > struct kasan_alloc_meta { > struct kasan_track alloc_track; > /* Free track is stored in kasan_free_meta. */ > - /* > - * aux_lock protects aux_stack from accesses from concurrent > - * kasan_record_aux_stack calls. It is a raw spinlock to avoid sl= eeping > - * on RT kernels, as kasan_record_aux_stack_noalloc can be called= from > - * non-sleepable contexts. > - */ > - raw_spinlock_t aux_lock; > depot_stack_handle_t aux_stack[2]; > }; > > -- > 2.43.0.429.g432eaa2c6b-goog > Reviewed-by: Andrey Konovalov But I'm wondering if we should also stop resetting metadata when the object is fully freed (from quarantine or bypassing quarantine). With stack_depot_put, I had to put the stack handles on free, as otherwise we would leak the stack depot references. And I also chose to memset meta at that point, as its gets invalid anyway. But without stack_depot_put, this is not required. Before the stack depot-related changes, the code was inconsistent in this regard AFAICS: for quarantine, free meta was marked as invalid via KASAN_SLAB_FREE but alloc meta was kept; for no quarantine, both alloc and free meta were kept. So perhaps we can just keep both metas on full free. I.e. drop both kasan_release_object_meta calls. This will go back to the old behavior + keeping free meta for the quarantine case (I think there's no harm in that). This will give better reporting for uaf-before-realloc bugs. WDYT?