lkml.org 
[lkml]   [2020]   [Sep]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH RFC 01/10] mm: add Kernel Electric-Fence infrastructure
Date
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

\
 
 \ /
  Last update: 2020-09-16 02:06    [W:0.067 / U:10.744 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site