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=-15.9 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,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1, USER_IN_DEF_DKIM_WL autolearn=unavailable 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 E5154C43461 for ; Tue, 15 Sep 2020 14:29:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 880B021D1B for ; Tue, 15 Sep 2020 14:29:00 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="H8nL+xfr" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726864AbgIOO2f (ORCPT ); Tue, 15 Sep 2020 10:28:35 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60300 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726844AbgIOOPx (ORCPT ); Tue, 15 Sep 2020 10:15:53 -0400 Received: from mail-wr1-x443.google.com (mail-wr1-x443.google.com [IPv6:2a00:1450:4864:20::443]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B4A76C061353 for ; Tue, 15 Sep 2020 07:14:57 -0700 (PDT) Received: by mail-wr1-x443.google.com with SMTP id m6so3554662wrn.0 for ; Tue, 15 Sep 2020 07:14:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=iCaVZIoES6nQdU1krWtYyw9RTOkMnShVAEcHdPRqonk=; b=H8nL+xfrR2mxbyn6DykwhrinOuCc43Bq4GG55o7PhvzL/oEb4JvN2r+q0D4w1iWRtl lTJAjeZmPRtqYiYVtDygNalJ/7YqIw8vqyGIevdY0rw1S8KfrDsIpCtIIrSm5Xqlws91 Mp0LGWsmmJdSf6Ep6I9VCMqe6bbK6bW5/LBQnS9KzDlAXh31DCfatqZhF3U8z0EmToMC y1WZ9qFx9VFHG0c5pQmxkBSaOltW+XnpfLWx+2Zf3i4S5ZLlS230y5ngbyQ9p1qAAK8G vckmdCk2DKTmA/KB82xol2azsXFCizK+AYwBKx3WB+OEn4fX3paGBgKtFWda+aF8c5xc eouQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=iCaVZIoES6nQdU1krWtYyw9RTOkMnShVAEcHdPRqonk=; b=CxnJbiJFK4jd9rU4/xG5SQE5ZbSFL9ufOzvB6itVtn8wbIFvP0sY3Udb27Gt3irKyU yOky7AxOX+AJ0rBxbYXbCHBjmyzhfHMOn6V92aOup58Hb8NO8/8XwWL1qWX5qq9pi7ts JMcHOgTyuSlPZAWHjTPjAkXIZkvcDx+2R1IvsyIU2S8/vgupmYJRE60F4HJn/eeAq7vE oSnfmbiujt9xGglF9/xkMElw38PBonNjwwpUQEN9IFVyLHXla89DvsFnYhmHO0+z8yyT gBPlFGPRn4NCpGwj5GRgDi4hYYdLHh/WsqIXeUWCcDxgUo71sxjWi2WaUg+scznkKjqR sWog== X-Gm-Message-State: AOAM532HPnKPm7pk+fQCt8UjLJrkBmC4V5p/iXxzeL27Qa30e0g/NjKV bFGRDv13wOzYG7fF0RORo5IMOw== X-Google-Smtp-Source: ABdhPJzTlxzBO4PMSx2ou+2vNV31AQcDM7exolub2p9hQpxlJ95llypdH0s2jTtZ0UyJtFiT/tEhZg== X-Received: by 2002:adf:df05:: with SMTP id y5mr23640721wrl.39.1600179295984; Tue, 15 Sep 2020 07:14:55 -0700 (PDT) Received: from elver.google.com ([100.105.32.75]) by smtp.gmail.com with ESMTPSA id k6sm23222807wmi.1.2020.09.15.07.14.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 15 Sep 2020 07:14:55 -0700 (PDT) Date: Tue, 15 Sep 2020 16:14:49 +0200 From: Marco Elver To: SeongJae Park Cc: glider@google.com, akpm@linux-foundation.org, catalin.marinas@arm.com, cl@linux.com, rientjes@google.com, iamjoonsoo.kim@lge.com, mark.rutland@arm.com, penberg@kernel.org, linux-doc@vger.kernel.org, peterz@infradead.org, dave.hansen@linux.intel.com, linux-mm@kvack.org, edumazet@google.com, hpa@zytor.com, will@kernel.org, corbet@lwn.net, x86@kernel.org, kasan-dev@googlegroups.com, mingo@redhat.com, linux-arm-kernel@lists.infradead.org, aryabinin@virtuozzo.com, keescook@chromium.org, paulmck@kernel.org, jannh@google.com, andreyknvl@google.com, cai@lca.pw, luto@kernel.org, tglx@linutronix.de, dvyukov@google.com, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, bp@alien8.de Subject: Re: [PATCH RFC 01/10] mm: add Kernel Electric-Fence infrastructure Message-ID: <20200915141449.GA3367763@elver.google.com> References: <20200907134055.2878499-2-elver@google.com> <20200915135754.24329-1-sjpark@amazon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200915135754.24329-1-sjpark@amazon.com> User-Agent: Mutt/1.14.4 (2020-06-18) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 15, 2020 at 03:57PM +0200, SeongJae Park wrote: [...] > > So interesting feature! I left some tirvial comments below. Thank you! > [...] > > diff --git a/lib/Kconfig.kfence b/lib/Kconfig.kfence > > new file mode 100644 > > index 000000000000..7ac91162edb0 > > --- /dev/null > > +++ b/lib/Kconfig.kfence > > @@ -0,0 +1,58 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > + > > +config HAVE_ARCH_KFENCE > > + bool > > + > > +config HAVE_ARCH_KFENCE_STATIC_POOL > > + bool > > + help > > + If the architecture supports using the static pool. > > + > > +menuconfig KFENCE > > + bool "KFENCE: low-overhead sampling-based memory safety error detector" > > + depends on HAVE_ARCH_KFENCE && !KASAN && (SLAB || SLUB) > > + depends on JUMP_LABEL # To ensure performance, require jump labels > > + select STACKTRACE > > + help > > + KFENCE is low-overhead sampling-based detector for heap out-of-bounds > > + access, use-after-free, and invalid-free errors. KFENCE is designed > > + to have negligible cost to permit enabling it in production > > + environments. > > + > > + See for more details. > > This patch doesn't provide the file yet. Why don't you add the reference with > the patch introducing the file? Sure, will fix for v3. > > + > > + Note that, KFENCE is not a substitute for explicit testing with tools > > + such as KASAN. KFENCE can detect a subset of bugs that KASAN can > > + detect (therefore enabling KFENCE together with KASAN does not make > > + sense), albeit at very different performance profiles. > [...] > > diff --git a/mm/kfence/core.c b/mm/kfence/core.c > > new file mode 100644 > > index 000000000000..e638d1f64a32 > > --- /dev/null > > +++ b/mm/kfence/core.c > > @@ -0,0 +1,730 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +#define pr_fmt(fmt) "kfence: " fmt > [...] > > + > > +static inline struct kfence_metadata *addr_to_metadata(unsigned long addr) > > +{ > > + long index; > > + > > + /* The checks do not affect performance; only called from slow-paths. */ > > + > > + if (!is_kfence_address((void *)addr)) > > + return NULL; > > + > > + /* > > + * May be an invalid index if called with an address at the edge of > > + * __kfence_pool, in which case we would report an "invalid access" > > + * error. > > + */ > > + index = ((addr - (unsigned long)__kfence_pool) / (PAGE_SIZE * 2)) - 1; > > Seems the outermost parentheses unnecessary. Will fix. > > + if (index < 0 || index >= CONFIG_KFENCE_NUM_OBJECTS) > > + return NULL; > > + > > + return &kfence_metadata[index]; > > +} > > + > > +static inline unsigned long metadata_to_pageaddr(const struct kfence_metadata *meta) > > +{ > > + unsigned long offset = ((meta - kfence_metadata) + 1) * PAGE_SIZE * 2; > > Seems the innermost parentheses unnecessary. Will fix. > > + unsigned long pageaddr = (unsigned long)&__kfence_pool[offset]; > > + > > + /* The checks do not affect performance; only called from slow-paths. */ > > + > > + /* Only call with a pointer into kfence_metadata. */ > > + if (KFENCE_WARN_ON(meta < kfence_metadata || > > + meta >= kfence_metadata + ARRAY_SIZE(kfence_metadata))) > > Is there a reason to use ARRAY_SIZE(kfence_metadata) instead of > CONFIG_KFENCE_NUM_OBJECTS? They're equivalent. We can switch it. (Although I don't see one being superior to the other.. maybe we save on compile-time?) > > + return 0; > > + > > + /* > > + * This metadata object only ever maps to 1 page; verify the calculation > > + * happens and that the stored address was not corrupted. > > + */ > > + if (KFENCE_WARN_ON(ALIGN_DOWN(meta->addr, PAGE_SIZE) != pageaddr)) > > + return 0; > > + > > + return pageaddr; > > +} > [...] > > +void __init kfence_init(void) > > +{ > > + /* Setting kfence_sample_interval to 0 on boot disables KFENCE. */ > > + if (!kfence_sample_interval) > > + return; > > + > > + if (!kfence_initialize_pool()) { > > + pr_err("%s failed\n", __func__); > > + return; > > + } > > + > > + schedule_delayed_work(&kfence_timer, 0); > > + WRITE_ONCE(kfence_enabled, true); > > + pr_info("initialized - using %zu bytes for %d objects", KFENCE_POOL_SIZE, > > + CONFIG_KFENCE_NUM_OBJECTS); > > + if (IS_ENABLED(CONFIG_DEBUG_KERNEL)) > > + pr_cont(" at 0x%px-0x%px\n", (void *)__kfence_pool, > > + (void *)(__kfence_pool + KFENCE_POOL_SIZE)); > > Why don't you use PTR_FMT that defined in 'kfence.h'? It's unnecessary, since all this is conditional on IS_ENABLED(CONFIG_DEBUG_KERNEL)) and we can just avoid the indirection through PTR_FMT. > > + else > > + pr_cont("\n"); > > +} > [...] > > diff --git a/mm/kfence/kfence.h b/mm/kfence/kfence.h > > new file mode 100644 > > index 000000000000..25ce2c0dc092 > > --- /dev/null > > +++ b/mm/kfence/kfence.h > > @@ -0,0 +1,104 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > + > > +#ifndef MM_KFENCE_KFENCE_H > > +#define MM_KFENCE_KFENCE_H > > + > > +#include > > +#include > > +#include > > +#include > > + > > +#include "../slab.h" /* for struct kmem_cache */ > > + > > +/* For non-debug builds, avoid leaking kernel pointers into dmesg. */ > > +#ifdef CONFIG_DEBUG_KERNEL > > +#define PTR_FMT "%px" > > +#else > > +#define PTR_FMT "%p" > > +#endif > > + > > +/* > > + * Get the canary byte pattern for @addr. Use a pattern that varies based on the > > + * lower 3 bits of the address, to detect memory corruptions with higher > > + * probability, where similar constants are used. > > + */ > > +#define KFENCE_CANARY_PATTERN(addr) ((u8)0xaa ^ (u8)((unsigned long)addr & 0x7)) > > + > > +/* Maximum stack depth for reports. */ > > +#define KFENCE_STACK_DEPTH 64 > > + > > +/* KFENCE object states. */ > > +enum kfence_object_state { > > + KFENCE_OBJECT_UNUSED, /* Object is unused. */ > > + KFENCE_OBJECT_ALLOCATED, /* Object is currently allocated. */ > > + KFENCE_OBJECT_FREED, /* Object was allocated, and then freed. */ > > Aligning the comments would look better (same to below comments). Will fix. > > +}; > [...] > > diff --git a/mm/kfence/report.c b/mm/kfence/report.c > > new file mode 100644 > > index 000000000000..8c28200e7433 > > --- /dev/null > > +++ b/mm/kfence/report.c > > @@ -0,0 +1,201 @@ > > +// SPDX-License-Identifier: GPL-2.0 > [...] > > +/* Get the number of stack entries to skip get out of MM internals. */ > > +static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries, > > + enum kfence_error_type type) > > +{ > > + char buf[64]; > > + int skipnr, fallback = 0; > > + > > + for (skipnr = 0; skipnr < num_entries; skipnr++) { > > + int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]); > > + > > + /* Depending on error type, find different stack entries. */ > > + switch (type) { > > + case KFENCE_ERROR_UAF: > > + case KFENCE_ERROR_OOB: > > + case KFENCE_ERROR_INVALID: > > + if (!strncmp(buf, KFENCE_SKIP_ARCH_FAULT_HANDLER, len)) > > Seems KFENCE_SKIP_ARCH_FAULT_HANDLER not defined yet? Correct, it'll be defined in in the x86 and arm64 patches. Leaving this is fine, since no architecture has selected HAVE_ARCH_KFENCE in this patch yet; as a result, we also can't break the build even if this is undefined. Thanks, -- Marco 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=-11.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable 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 E6F1AC433E2 for ; Tue, 15 Sep 2020 14:16:48 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 867AC22244 for ; Tue, 15 Sep 2020 14:16:48 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="zhSxUQoU"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="H8nL+xfr" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 867AC22244 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=W9AnjP0rqwt/GBImuYfEDr87XRcdrGiLx0H4o4vlmNs=; b=zhSxUQoUJUid5ivRYLnykdJNt Ai0Pfe1pOCyngFOLhqQ14RAsidDRaGOYLJ148kV5SkMDVti5IqGN73zYQ1ne4UdczVPcgMn//dlFL vF+Ofryo0mRPGki4COkvQl31l/Tp2f/gsRhVEU7BIFAu9JQv26mdRoYDaZa6nAvaXYR2r5Z92v5mE t7Q/iHIDTrLFGgAnWpe83AhxErdAT5/V4ZIlmFDb635aPvMIzR/ptKsgt+FgIj1yV941QEhJqT0kR RaSP+TqCxI+CAZkptw4+7IoFbBbzOa+lJTM1d8JSQUZ2D33HoQDLt+6u1AYl2NOfBnnWrNWUPr2yH hc3FohwRA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kIBjY-0005Gp-82; Tue, 15 Sep 2020 14:15:00 +0000 Received: from mail-wr1-x444.google.com ([2a00:1450:4864:20::444]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kIBjV-0005Fq-Dz for linux-arm-kernel@lists.infradead.org; Tue, 15 Sep 2020 14:14:58 +0000 Received: by mail-wr1-x444.google.com with SMTP id z1so3531709wrt.3 for ; Tue, 15 Sep 2020 07:14:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=iCaVZIoES6nQdU1krWtYyw9RTOkMnShVAEcHdPRqonk=; b=H8nL+xfrR2mxbyn6DykwhrinOuCc43Bq4GG55o7PhvzL/oEb4JvN2r+q0D4w1iWRtl lTJAjeZmPRtqYiYVtDygNalJ/7YqIw8vqyGIevdY0rw1S8KfrDsIpCtIIrSm5Xqlws91 Mp0LGWsmmJdSf6Ep6I9VCMqe6bbK6bW5/LBQnS9KzDlAXh31DCfatqZhF3U8z0EmToMC y1WZ9qFx9VFHG0c5pQmxkBSaOltW+XnpfLWx+2Zf3i4S5ZLlS230y5ngbyQ9p1qAAK8G vckmdCk2DKTmA/KB82xol2azsXFCizK+AYwBKx3WB+OEn4fX3paGBgKtFWda+aF8c5xc eouQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=iCaVZIoES6nQdU1krWtYyw9RTOkMnShVAEcHdPRqonk=; b=SEG34shb4pozIkABZanPqOEpwOYvpgQam/tZoC23GxpmSk0G2DWP0E38tvaaGk1O/V R5068CCHCbk3KVcGRF6lAyUIYe5E9dC/m52JazwS5aQ9iYDw83yH8CxU/Q2XWjw2oipr Ug3WbB7epighb56yPIHmlkhaER+KVl4zVmnNhFnM9rALic0jo+2qLqBUxdPLKBxiKLsR /lQoG/Job9M2a4sgnk4oRecv5yv5JocXhecnE4AgeBGDT7oOKSzEY/8rQqnHj4jptG+h lCKqYfaTCqF2e1nNM2n/OtdWLAipeO0+pH8Tqrc5DQADIUozIY40N9rjh2xzdZD3lZ+9 0bag== X-Gm-Message-State: AOAM530Epss8q7Xd4PkJxo3x85MufUc9c2FkeNOsstUX+n/Ozhq2ujK7 BQi/FmUhZzLrQV4aOsueBLWhzQ== X-Google-Smtp-Source: ABdhPJzTlxzBO4PMSx2ou+2vNV31AQcDM7exolub2p9hQpxlJ95llypdH0s2jTtZ0UyJtFiT/tEhZg== X-Received: by 2002:adf:df05:: with SMTP id y5mr23640721wrl.39.1600179295984; Tue, 15 Sep 2020 07:14:55 -0700 (PDT) Received: from elver.google.com ([100.105.32.75]) by smtp.gmail.com with ESMTPSA id k6sm23222807wmi.1.2020.09.15.07.14.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 15 Sep 2020 07:14:55 -0700 (PDT) Date: Tue, 15 Sep 2020 16:14:49 +0200 From: Marco Elver To: SeongJae Park Subject: Re: [PATCH RFC 01/10] mm: add Kernel Electric-Fence infrastructure Message-ID: <20200915141449.GA3367763@elver.google.com> References: <20200907134055.2878499-2-elver@google.com> <20200915135754.24329-1-sjpark@amazon.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200915135754.24329-1-sjpark@amazon.com> User-Agent: Mutt/1.14.4 (2020-06-18) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200915_101457_498375_E98486D4 X-CRM114-Status: GOOD ( 41.73 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, linux-doc@vger.kernel.org, peterz@infradead.org, catalin.marinas@arm.com, dave.hansen@linux.intel.com, linux-mm@kvack.org, edumazet@google.com, glider@google.com, hpa@zytor.com, cl@linux.com, will@kernel.org, corbet@lwn.net, x86@kernel.org, kasan-dev@googlegroups.com, mingo@redhat.com, dvyukov@google.com, rientjes@google.com, aryabinin@virtuozzo.com, keescook@chromium.org, paulmck@kernel.org, jannh@google.com, andreyknvl@google.com, cai@lca.pw, luto@kernel.org, tglx@linutronix.de, akpm@linux-foundation.org, linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, penberg@kernel.org, bp@alien8.de, iamjoonsoo.kim@lge.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Sep 15, 2020 at 03:57PM +0200, SeongJae Park wrote: [...] > > So interesting feature! I left some tirvial comments below. Thank you! > [...] > > diff --git a/lib/Kconfig.kfence b/lib/Kconfig.kfence > > new file mode 100644 > > index 000000000000..7ac91162edb0 > > --- /dev/null > > +++ b/lib/Kconfig.kfence > > @@ -0,0 +1,58 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > + > > +config HAVE_ARCH_KFENCE > > + bool > > + > > +config HAVE_ARCH_KFENCE_STATIC_POOL > > + bool > > + help > > + If the architecture supports using the static pool. > > + > > +menuconfig KFENCE > > + bool "KFENCE: low-overhead sampling-based memory safety error detector" > > + depends on HAVE_ARCH_KFENCE && !KASAN && (SLAB || SLUB) > > + depends on JUMP_LABEL # To ensure performance, require jump labels > > + select STACKTRACE > > + help > > + KFENCE is low-overhead sampling-based detector for heap out-of-bounds > > + access, use-after-free, and invalid-free errors. KFENCE is designed > > + to have negligible cost to permit enabling it in production > > + environments. > > + > > + See for more details. > > This patch doesn't provide the file yet. Why don't you add the reference with > the patch introducing the file? Sure, will fix for v3. > > + > > + Note that, KFENCE is not a substitute for explicit testing with tools > > + such as KASAN. KFENCE can detect a subset of bugs that KASAN can > > + detect (therefore enabling KFENCE together with KASAN does not make > > + sense), albeit at very different performance profiles. > [...] > > diff --git a/mm/kfence/core.c b/mm/kfence/core.c > > new file mode 100644 > > index 000000000000..e638d1f64a32 > > --- /dev/null > > +++ b/mm/kfence/core.c > > @@ -0,0 +1,730 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +#define pr_fmt(fmt) "kfence: " fmt > [...] > > + > > +static inline struct kfence_metadata *addr_to_metadata(unsigned long addr) > > +{ > > + long index; > > + > > + /* The checks do not affect performance; only called from slow-paths. */ > > + > > + if (!is_kfence_address((void *)addr)) > > + return NULL; > > + > > + /* > > + * May be an invalid index if called with an address at the edge of > > + * __kfence_pool, in which case we would report an "invalid access" > > + * error. > > + */ > > + index = ((addr - (unsigned long)__kfence_pool) / (PAGE_SIZE * 2)) - 1; > > Seems the outermost parentheses unnecessary. Will fix. > > + if (index < 0 || index >= CONFIG_KFENCE_NUM_OBJECTS) > > + return NULL; > > + > > + return &kfence_metadata[index]; > > +} > > + > > +static inline unsigned long metadata_to_pageaddr(const struct kfence_metadata *meta) > > +{ > > + unsigned long offset = ((meta - kfence_metadata) + 1) * PAGE_SIZE * 2; > > Seems the innermost parentheses unnecessary. Will fix. > > + unsigned long pageaddr = (unsigned long)&__kfence_pool[offset]; > > + > > + /* The checks do not affect performance; only called from slow-paths. */ > > + > > + /* Only call with a pointer into kfence_metadata. */ > > + if (KFENCE_WARN_ON(meta < kfence_metadata || > > + meta >= kfence_metadata + ARRAY_SIZE(kfence_metadata))) > > Is there a reason to use ARRAY_SIZE(kfence_metadata) instead of > CONFIG_KFENCE_NUM_OBJECTS? They're equivalent. We can switch it. (Although I don't see one being superior to the other.. maybe we save on compile-time?) > > + return 0; > > + > > + /* > > + * This metadata object only ever maps to 1 page; verify the calculation > > + * happens and that the stored address was not corrupted. > > + */ > > + if (KFENCE_WARN_ON(ALIGN_DOWN(meta->addr, PAGE_SIZE) != pageaddr)) > > + return 0; > > + > > + return pageaddr; > > +} > [...] > > +void __init kfence_init(void) > > +{ > > + /* Setting kfence_sample_interval to 0 on boot disables KFENCE. */ > > + if (!kfence_sample_interval) > > + return; > > + > > + if (!kfence_initialize_pool()) { > > + pr_err("%s failed\n", __func__); > > + return; > > + } > > + > > + schedule_delayed_work(&kfence_timer, 0); > > + WRITE_ONCE(kfence_enabled, true); > > + pr_info("initialized - using %zu bytes for %d objects", KFENCE_POOL_SIZE, > > + CONFIG_KFENCE_NUM_OBJECTS); > > + if (IS_ENABLED(CONFIG_DEBUG_KERNEL)) > > + pr_cont(" at 0x%px-0x%px\n", (void *)__kfence_pool, > > + (void *)(__kfence_pool + KFENCE_POOL_SIZE)); > > Why don't you use PTR_FMT that defined in 'kfence.h'? It's unnecessary, since all this is conditional on IS_ENABLED(CONFIG_DEBUG_KERNEL)) and we can just avoid the indirection through PTR_FMT. > > + else > > + pr_cont("\n"); > > +} > [...] > > diff --git a/mm/kfence/kfence.h b/mm/kfence/kfence.h > > new file mode 100644 > > index 000000000000..25ce2c0dc092 > > --- /dev/null > > +++ b/mm/kfence/kfence.h > > @@ -0,0 +1,104 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > + > > +#ifndef MM_KFENCE_KFENCE_H > > +#define MM_KFENCE_KFENCE_H > > + > > +#include > > +#include > > +#include > > +#include > > + > > +#include "../slab.h" /* for struct kmem_cache */ > > + > > +/* For non-debug builds, avoid leaking kernel pointers into dmesg. */ > > +#ifdef CONFIG_DEBUG_KERNEL > > +#define PTR_FMT "%px" > > +#else > > +#define PTR_FMT "%p" > > +#endif > > + > > +/* > > + * Get the canary byte pattern for @addr. Use a pattern that varies based on the > > + * lower 3 bits of the address, to detect memory corruptions with higher > > + * probability, where similar constants are used. > > + */ > > +#define KFENCE_CANARY_PATTERN(addr) ((u8)0xaa ^ (u8)((unsigned long)addr & 0x7)) > > + > > +/* Maximum stack depth for reports. */ > > +#define KFENCE_STACK_DEPTH 64 > > + > > +/* KFENCE object states. */ > > +enum kfence_object_state { > > + KFENCE_OBJECT_UNUSED, /* Object is unused. */ > > + KFENCE_OBJECT_ALLOCATED, /* Object is currently allocated. */ > > + KFENCE_OBJECT_FREED, /* Object was allocated, and then freed. */ > > Aligning the comments would look better (same to below comments). Will fix. > > +}; > [...] > > diff --git a/mm/kfence/report.c b/mm/kfence/report.c > > new file mode 100644 > > index 000000000000..8c28200e7433 > > --- /dev/null > > +++ b/mm/kfence/report.c > > @@ -0,0 +1,201 @@ > > +// SPDX-License-Identifier: GPL-2.0 > [...] > > +/* Get the number of stack entries to skip get out of MM internals. */ > > +static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries, > > + enum kfence_error_type type) > > +{ > > + char buf[64]; > > + int skipnr, fallback = 0; > > + > > + for (skipnr = 0; skipnr < num_entries; skipnr++) { > > + int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]); > > + > > + /* Depending on error type, find different stack entries. */ > > + switch (type) { > > + case KFENCE_ERROR_UAF: > > + case KFENCE_ERROR_OOB: > > + case KFENCE_ERROR_INVALID: > > + if (!strncmp(buf, KFENCE_SKIP_ARCH_FAULT_HANDLER, len)) > > Seems KFENCE_SKIP_ARCH_FAULT_HANDLER not defined yet? Correct, it'll be defined in in the x86 and arm64 patches. Leaving this is fine, since no architecture has selected HAVE_ARCH_KFENCE in this patch yet; as a result, we also can't break the build even if this is undefined. Thanks, -- Marco _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel