Re: [PATCH v8 7/7] mm: kasan: Initial memory quarantine implementation

[Andrey Ryabinin <ryabinin.a.a@gmail.com>]

2016-03-15 13:10 GMT+03:00 Alexander Potapenko <glider@google.com>:

>
>  static inline int kasan_module_alloc(void *addr, size_t size) { return 0; }
>  static inline void kasan_free_shadow(const struct vm_struct *vm) {}
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 82169fb..799c98e 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -344,6 +344,32 @@ static noinline void __init kasan_stack_oob(void)
>         *(volatile char *)p;
>  }
>
> +#ifdef CONFIG_SLAB
> +static noinline void __init kasan_quarantine_cache(void)
> +{
> +       struct kmem_cache *cache = kmem_cache_create(
> +                       "test", 137, 8, GFP_KERNEL, NULL);
> +       int i;
> +
> +       for (i = 0; i <  100; i++) {
> +               void *p = kmem_cache_alloc(cache, GFP_KERNEL);
> +
> +               kmem_cache_free(cache, p);
> +               p = kmalloc(sizeof(u64), GFP_KERNEL);
> +               kfree(p);
> +       }
> +       kmem_cache_shrink(cache);
> +       for (i = 0; i <  100; i++) {
> +               u64 *p = kmem_cache_alloc(cache, GFP_KERNEL);
> +
> +               kmem_cache_free(cache, p);
> +               p = kmalloc(sizeof(u64), GFP_KERNEL);
> +               kfree(p);
> +       }
> +       kmem_cache_destroy(cache);
> +}
> +#endif
> +

Test looks quite useless. The kernel does allocations/frees all the
time, so I don't think that this test
adds something valuable.
And what's the result that we expect from this test? No crashes?
I'm thinking it would better to remove it.

[...]

> +
> +/* smp_load_acquire() here pairs with smp_store_release() in
> + * quarantine_reduce().
> + */
> +#define QUARANTINE_LOW_SIZE (smp_load_acquire(&quarantine_size) * 3 / 4)

I'd prefer open coding barrier with a proper comment int place,
instead of sneaking it into macros.

[...]

> +
> +void quarantine_reduce(void)
> +{
> +       size_t new_quarantine_size;
> +       unsigned long flags;
> +       struct qlist to_free = QLIST_INIT;
> +       size_t size_to_free = 0;
> +       void **last;
> +
> +       /* smp_load_acquire() here pairs with smp_store_release() below. */

Besides pairing rules, the comment should also explain *why* we need
this and for what
load/stores it provides memory ordering guarantees. For example take a
look at other
comments near barriers in the kernel tree.

> +       if (likely(ACCESS_ONCE(global_quarantine.bytes) <=
> +                  smp_load_acquire(&quarantine_size)))
> +               return;
> +
>