From: Marco Elver <elver@google.com>
To: SeongJae Park <sjpark@amazon.com>
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
Date: Tue, 15 Sep 2020 16:14:49 +0200 [thread overview]
Message-ID: <20200915141449.GA3367763@elver.google.com> (raw)
In-Reply-To: <20200915135754.24329-1-sjpark@amazon.com>
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 <file:Documentation/dev-tools/kfence.rst> 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 <linux/mm.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/types.h>
> > +
> > +#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 <asm/kfence.h> 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
next prev parent reply other threads:[~2020-09-15 14:29 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-07 13:40 [PATCH RFC 00/10] KFENCE: A low-overhead sampling-based memory safety error detector Marco Elver
2020-09-07 13:40 ` [PATCH RFC 01/10] mm: add Kernel Electric-Fence infrastructure Marco Elver
2020-09-07 15:41 ` Jonathan Cameron
2020-09-07 16:38 ` Marco Elver
2020-09-10 14:57 ` Dmitry Vyukov
2020-09-10 15:06 ` Marco Elver
2020-09-10 15:48 ` Dmitry Vyukov
2020-09-10 16:22 ` Marco Elver
2020-09-10 15:42 ` Dmitry Vyukov
2020-09-10 16:19 ` Alexander Potapenko
2020-09-10 17:11 ` Dmitry Vyukov
2020-09-10 17:41 ` Marco Elver
2020-09-10 20:25 ` Paul E. McKenney
2020-09-15 13:57 ` SeongJae Park
2020-09-15 14:14 ` Marco Elver [this message]
2020-09-15 14:26 ` SeongJae Park
2020-09-07 13:40 ` [PATCH RFC 02/10] x86, kfence: enable KFENCE for x86 Marco Elver
2020-09-07 13:40 ` [PATCH RFC 03/10] arm64, kfence: enable KFENCE for ARM64 Marco Elver
2020-09-09 15:13 ` Marco Elver
2020-09-07 13:40 ` [PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB Marco Elver
2020-09-11 7:17 ` Dmitry Vyukov
2020-09-11 12:24 ` Marco Elver
2020-09-11 13:03 ` Dmitry Vyukov
2020-09-07 13:40 ` [PATCH RFC 05/10] mm, kfence: insert KFENCE hooks for SLUB Marco Elver
2020-09-07 13:40 ` [PATCH RFC 06/10] kfence, kasan: make KFENCE compatible with KASAN Marco Elver
2020-09-11 7:04 ` Dmitry Vyukov
2020-09-11 13:00 ` Marco Elver
2020-09-07 13:40 ` [PATCH RFC 07/10] kfence, kmemleak: make KFENCE compatible with KMEMLEAK Marco Elver
2020-09-08 11:53 ` Catalin Marinas
2020-09-08 12:29 ` Alexander Potapenko
2020-09-07 13:40 ` [PATCH RFC 08/10] kfence, lockdep: make KFENCE compatible with lockdep Marco Elver
2020-09-07 13:40 ` [PATCH RFC 09/10] kfence, Documentation: add KFENCE documentation Marco Elver
2020-09-07 15:33 ` Andrey Konovalov
2020-09-07 16:33 ` Marco Elver
2020-09-07 17:55 ` Andrey Konovalov
2020-09-07 18:16 ` Marco Elver
2020-09-08 15:54 ` Dave Hansen
2020-09-08 16:14 ` Marco Elver
2020-09-11 7:14 ` Dmitry Vyukov
2020-09-11 7:46 ` Marco Elver
2020-09-07 13:40 ` [PATCH RFC 10/10] kfence: add test suite Marco Elver
2020-09-08 11:48 ` [PATCH RFC 00/10] KFENCE: A low-overhead sampling-based memory safety error detector Vlastimil Babka
2020-09-08 12:16 ` Alexander Potapenko
2020-09-08 14:40 ` Vlastimil Babka
2020-09-08 15:21 ` Marco Elver
[not found] ` <e399d8d5-03c2-3c13-2a43-3bb8e842c55a@intel.com>
2020-09-08 15:31 ` Marco Elver
2020-09-08 15:36 ` Vlastimil Babka
2020-09-08 15:56 ` Marco Elver
2020-09-11 7:35 ` Dmitry Vyukov
2020-09-11 12:03 ` Marco Elver
2020-09-11 13:09 ` Dmitry Vyukov
2020-09-11 13:33 ` Marco Elver
2020-09-11 16:33 ` Marco Elver
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200915141449.GA3367763@elver.google.com \
--to=elver@google.com \
--cc=akpm@linux-foundation.org \
--cc=andreyknvl@google.com \
--cc=aryabinin@virtuozzo.com \
--cc=bp@alien8.de \
--cc=cai@lca.pw \
--cc=catalin.marinas@arm.com \
--cc=cl@linux.com \
--cc=corbet@lwn.net \
--cc=dave.hansen@linux.intel.com \
--cc=dvyukov@google.com \
--cc=edumazet@google.com \
--cc=glider@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=hpa@zytor.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=jannh@google.com \
--cc=kasan-dev@googlegroups.com \
--cc=keescook@chromium.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=luto@kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=paulmck@kernel.org \
--cc=penberg@kernel.org \
--cc=peterz@infradead.org \
--cc=rientjes@google.com \
--cc=sjpark@amazon.com \
--cc=tglx@linutronix.de \
--cc=will@kernel.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).