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
