Re: [PATCH v6 8/9] kfence: add test suite

[Marco Elver <elver@google.com>]

On Fri, 30 Oct 2020 at 03:50, Jann Horn <jannh@google.com> wrote:
>
> On Thu, Oct 29, 2020 at 2:17 PM Marco Elver <elver@google.com> wrote:
> > Add KFENCE test suite, testing various error detection scenarios. Makes
> > use of KUnit for test organization. Since KFENCE's interface to obtain
> > error reports is via the console, the test verifies that KFENCE outputs
> > expected reports to the console.
> [...]
> > diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
> [...]
> > +static void *test_alloc(struct kunit *test, size_t size, gfp_t gfp, enum allocation_policy policy)
> > +{
> > +       void *alloc;
> > +       unsigned long timeout, resched_after;
> [...]
> > +       /*
> > +        * 100x the sample interval should be more than enough to ensure we get
> > +        * a KFENCE allocation eventually.
> > +        */
> > +       timeout = jiffies + msecs_to_jiffies(100 * CONFIG_KFENCE_SAMPLE_INTERVAL);
> > +       /*
> > +        * Especially for non-preemption kernels, ensure the allocation-gate
> > +        * timer has time to catch up.
> > +        */
> > +       resched_after = jiffies + msecs_to_jiffies(CONFIG_KFENCE_SAMPLE_INTERVAL);
> > +       do {
> [...]
> > +               if (time_after(jiffies, resched_after))
> > +                       cond_resched();
>
> You probably meant to recalculate resched_after after the call to
> cond_resched()?

This is intentional. After @resched_after is reached, every failed
allocation attempt will result in a cond_resched(), because we know
the sample interval has elapsed and KFENCE should have kicked in. So
we just want to ensure the delayed work gets to run as soon as
possible, and just keep yielding.

Added a clarifying comment.

> > +       } while (time_before(jiffies, timeout));
> > +
> > +       KUNIT_ASSERT_TRUE_MSG(test, false, "failed to allocate from KFENCE");
> > +       return NULL; /* Unreachable. */
> > +}
> [...]
> > +/*
> > + * KFENCE is unable to detect an OOB if the allocation's alignment requirements
> > + * leave a gap between the object and the guard page. Specifically, an
> > + * allocation of e.g. 73 bytes is aligned on 8 and 128 bytes for SLUB or SLAB
> > + * respectively. Therefore it is impossible for the allocated object to adhere
> > + * to either of the page boundaries.
>
> Should this be "to the left page boundary" instead of "to either of
> the page boundaries"?

Thanks for spotting. I think it's "Therefore it is impossible for the
allocated object to contiguously line up with the right guard page."

> > + * However, we test that an access to memory beyond the gap result in KFENCE
>
> *results
>
>
>
> > + * detecting an OOB access.
> > + */
> > +static void test_kmalloc_aligned_oob_read(struct kunit *test)

Thanks, will address these for v7.