Re: [PATCH RFC 01/10] mm: add Kernel Electric-Fence infrastructure

From: SeongJae Park <sjpark@amazon.com>
To: Marco Elver <elver@google.com>
Cc: SeongJae Park <sjpark@amazon.com>, <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>
Subject: Re: [PATCH RFC 01/10] mm: add Kernel Electric-Fence infrastructure
Date: Tue, 15 Sep 2020 16:26:31 +0200	[thread overview]
Message-ID: <20200915142631.31234-1-sjpark@amazon.com> (raw)
In-Reply-To: <20200915141449.GA3367763@elver.google.com>

On Tue, 15 Sep 2020 16:14:49 +0200 Marco Elver <elver@google.com> wrote:

> On Tue, Sep 15, 2020 at 03:57PM +0200, SeongJae Park wrote:
> [...]
> > 
> > So interesting feature!  I left some tirvial comments below.
> 
> Thank you!
[...]
> > > +
> > > +	/* 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?)

I prefer CONFIG_KFENCE_NUM_OBJECTS here just because it's more widely used in
the code.  Also, I personally think it's more easy to read.

[...]
> > > +	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.

Ok, agreed.

[...]
> > > +	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.

Ah, got it.  Thank you for the kind explanation.

Thanks,
SeongJae Park

> 
> Thanks,
> -- Marco