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

[SeongJae Park]

On Tue, 15 Sep 2020 16:14:49 +0200 Marco Elver <elver@xxxxxxxxxx> 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