On Wed, Jun 15, 2016 at 6:50 PM, Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx> wrote: > > > On 06/15/2016 06:26 PM, Alexander Potapenko wrote: >> For KASAN builds: >> - switch SLUB allocator to using stackdepot instead of storing the >> allocation/deallocation stacks in the objects; >> - define SLAB_RED_ZONE, SLAB_POISON, SLAB_STORE_USER to zero, >> effectively disabling these debug features, as they're redundant in >> the presence of KASAN; > > So, why we forbid these? If user wants to set these, why not? If you don't want it, just don't turn them on, that's it. SLAB_RED_ZONE simply doesn't work with KASAN. With additional efforts it may work, but I don't think we really need that. Extra red zones will just bloat the heap, and won't give any interesting signal except "someone corrupted this object from non-instrumented code". SLAB_POISON doesn't crash on simple tests, but I am not sure there are no corner cases which I haven't checked, so I thought it's safer to disable it. As I said before, we can make SLAB_STORE_USER use stackdepot in a later CL, thus we disable it now. > And sometimes POISON/REDZONE might be actually useful. KASAN doesn't catch everything, > e.g. corruption may happen in assembly code, or DMA by some device. > > >> - change the freelist hook so that parts of the freelist can be put into >> the quarantine. >> >> Signed-off-by: Alexander Potapenko <glider@xxxxxxxxxx> >> --- > > ... > >> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h >> index fb87923..8c75953 100644 >> --- a/mm/kasan/kasan.h >> +++ b/mm/kasan/kasan.h >> @@ -110,7 +110,7 @@ static inline bool kasan_report_enabled(void) >> void kasan_report(unsigned long addr, size_t size, >> bool is_write, unsigned long ip); >> >> -#ifdef CONFIG_SLAB >> +#if defined(CONFIG_SLAB) || defined(CONFIG_SLUB) >> void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache); >> void quarantine_reduce(void); >> void quarantine_remove_cache(struct kmem_cache *cache); >> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c >> index 4973505..89259c2 100644 >> --- a/mm/kasan/quarantine.c >> +++ b/mm/kasan/quarantine.c >> @@ -149,7 +149,12 @@ static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache) >> >> local_irq_save(flags); >> alloc_info->state = KASAN_STATE_FREE; >> +#ifdef CONFIG_SLAB >> ___cache_free(cache, object, _THIS_IP_); >> +#elif defined(CONFIG_SLUB) >> + do_slab_free(cache, virt_to_head_page(object), object, NULL, 1, >> + _RET_IP_); >> +#endif > > Please, add some simple wrapper instead of this. > >> local_irq_restore(flags); >> } >> Done. I've reused ___cache_free(). > > ... > >> diff --git a/mm/slub.c b/mm/slub.c >> index 825ff45..f023dd4 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -191,7 +191,11 @@ static inline bool kmem_cache_has_cpu_partial(struct kmem_cache *s) >> #define MAX_OBJS_PER_PAGE 32767 /* since page.objects is u15 */ >> >> /* Internal SLUB flags */ >> +#ifndef CONFIG_KASAN >> #define __OBJECT_POISON 0x80000000UL /* Poison object */ >> +#else >> +#define __OBJECT_POISON 0x00000000UL /* Disable object poisoning */ >> +#endif >> #define __CMPXCHG_DOUBLE 0x40000000UL /* Use cmpxchg_double */ >> >> #ifdef CONFIG_SMP >> @@ -454,10 +458,8 @@ static inline void *restore_red_left(struct kmem_cache *s, void *p) >> */ >> #if defined(CONFIG_SLUB_DEBUG_ON) >> static int slub_debug = DEBUG_DEFAULT_FLAGS; >> -#elif defined(CONFIG_KASAN) >> -static int slub_debug = SLAB_STORE_USER; >> #else >> -static int slub_debug; >> +static int slub_debug = SLAB_STORE_USER; > > Huh! So now it is on!? By default, and for everyone! > Good catch, thanks! -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href