SLAB support for KASAN

Add KASAN hooks to SLAB allocator.

This patch is based on the "mm: kasan: unified support for SLUB and SLAB allocators" patch originally prepared by Dmitry Chernenkov.

Signed-off-by Alexander Potapenko <[email protected]>

---
v3: - minor description changes
- store deallocation info in kasan_slab_free()

v4: - fix kbuild compile-time warnings in print_track()
---

mm/kasan/kasan.c +53 -0

index bc0a8d8..d26ffb4 100644
@@ -314,6 +314,59 @@ void kasan_free_pages(struct page *page, unsigned int order)
 				KASAN_FREE_PAGE);
 }
 
+#ifdef CONFIG_SLAB
+/*
+ * Adaptive redzone policy taken from the userspace AddressSanitizer runtime.
+ * For larger allocations larger redzones are used.
+ */
+static size_t optimal_redzone(size_t object_size)
+{
+	int rz =
+		object_size <= 64        - 16   ? 16 :
+		object_size <= 128       - 32   ? 32 :
+		object_size <= 512       - 64   ? 64 :
+		object_size <= 4096      - 128  ? 128 :
+		object_size <= (1 << 14) - 256  ? 256 :
+		object_size <= (1 << 15) - 512  ? 512 :
+		object_size <= (1 << 16) - 1024 ? 1024 : 2048;
+	return rz;
+}
+
+void kasan_cache_create(struct kmem_cache *cache, size_t *size,
+			unsigned long *flags)
+{
+	int redzone_adjust;
+	/* Make sure the adjusted size is still less than
+	 * KMALLOC_MAX_CACHE_SIZE.
+	 * TODO: this check is only useful for SLAB, but not SLUB. We'll need
+	 * to skip it for SLUB when it starts using kasan_cache_create().
+	 */
+	if (*size > KMALLOC_MAX_CACHE_SIZE -
+	    sizeof(struct kasan_alloc_meta) -
+	    sizeof(struct kasan_free_meta))
+		return;
+	*flags |= SLAB_KASAN;
+	/* Add alloc meta. */
+	cache->kasan_info.alloc_meta_offset = *size;
+	*size += sizeof(struct kasan_alloc_meta);
+
+	/* Add free meta. */
+	if (cache->flags & SLAB_DESTROY_BY_RCU || cache->ctor ||
+	    cache->object_size < sizeof(struct kasan_free_meta)) {
+		cache->kasan_info.free_meta_offset = *size;
+		*size += sizeof(struct kasan_free_meta);
+	}
+	redzone_adjust = optimal_redzone(cache->object_size) -
+		(*size - cache->object_size);
+	if (redzone_adjust > 0)
+		*size += redzone_adjust;
+	*size = min(KMALLOC_MAX_CACHE_SIZE,
+		    max(*size,
+			cache->object_size +
+			optimal_redzone(cache->object_size)));
+}
+#endif
+
 
 
 

void kasan_poison_slab(struct page *page)
{

mm/kasan/kasan.c +49 -0

 	kasan_poison_shadow(page_address(page),

@@ -331,8 +384,36 @@ void kasan_poison_object_data(struct kmem_cache *cache, void *object)
 	kasan_poison_shadow(object,
 			round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE),
 			KASAN_KMALLOC_REDZONE);
+#ifdef CONFIG_SLAB
+	if (cache->flags & SLAB_KASAN) {
+		struct kasan_alloc_meta *alloc_info =
+			get_alloc_info(cache, object);
+		alloc_info->state = KASAN_STATE_INIT;
+	}
+#endif
+}
+
+static inline void set_track(struct kasan_track *track)
+{
+	track->cpu = raw_smp_processor_id();
+	track->pid = current->pid;
+	track->when = jiffies;
 }
 
+#ifdef CONFIG_SLAB
+struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
+					const void *object)
+{
+	return (void *)object + cache->kasan_info.alloc_meta_offset;
+}
+
+struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
+				      const void *object)
+{
+	return (void *)object + cache->kasan_info.free_meta_offset;
+}
+#endif
+
 void kasan_slab_alloc(struct kmem_cache *cache, void *object)
 {
 	kasan_kmalloc(cache, object, cache->object_size);

@@ -347,6 +428,17 @@ void kasan_slab_free(struct kmem_cache *cache, void *object)
 	if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
 		return;
 
+#ifdef CONFIG_SLAB
+	if (cache->flags & SLAB_KASAN) {
+		struct kasan_free_meta *free_info =
+			get_free_info(cache, object);
+		struct kasan_alloc_meta *alloc_info =
+			get_alloc_info(cache, object);
+		alloc_info->state = KASAN_STATE_FREE;
+		set_track(&free_info->track);
+	}
+#endif
+
 	kasan_poison_shadow(object, rounded_up_size, KASAN_KMALLOC_FREE);
 }
 

@@ -366,6 +458,16 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size)
 	kasan_unpoison_shadow(object, size);
 	kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start,
 		KASAN_KMALLOC_REDZONE);
+#ifdef CONFIG_SLAB
+	if (cache->flags & SLAB_KASAN) {
+		struct kasan_alloc_meta *alloc_info =
+			get_alloc_info(cache, object);
+
+		alloc_info->state = KASAN_STATE_ALLOC;
+		alloc_info->alloc_size = size;
+		set_track(&alloc_info->track);
+	}
+#endif
 }
 EXPORT_SYMBOL(kasan_kmalloc);
 

mm/kasan/kasan.h +24 -0

index 4f6c62e..7b9e4ab9 100644
@@ -54,6 +54,40 @@ struct kasan_global {
 #endif
 };
 
+/**
+ * Structures to keep alloc and free tracks *
+ */
+
+enum kasan_state {
+	KASAN_STATE_INIT,
+	KASAN_STATE_ALLOC,
+	KASAN_STATE_FREE
+};
+
+struct kasan_track {
+	u64 cpu : 6;			/* for NR_CPUS = 64 */
+	u64 pid : 16;			/* 65536 processes */
+	u64 when : 42;			/* ~140 years */
+};
+
+struct kasan_alloc_meta {
+	u32 state : 2;	/* enum kasan_state */
+	u32 alloc_size : 30;
+	struct kasan_track track;
+};
+
+struct kasan_free_meta {
+	/* Allocator freelist pointer, unused by KASAN. */

mm/kasan/kasan.h +0 -0

 static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
 {
 	return (void *)(((unsigned long)shadow_addr - KASAN_SHADOW_OFFSET)

mm/kasan/report.c +6 -0

index 12f222d..2c1407f 100644
@@ -115,6 +115,44 @@ static inline bool init_task_stack_addr(const void *addr)
 			sizeof(init_thread_union.stack));
 }
 
+#ifdef CONFIG_SLAB
+static void print_track(struct kasan_track *track)
+{
+	pr_err("PID = %u, CPU = %u, timestamp = %lu\n", track->pid,
+	       track->cpu, (unsigned long)track->when);
+}

mm/kasan/report.c +22 -4

+	case KASAN_STATE_INIT:
+		pr_err("Object not allocated yet\n");
+		break;
+	case KASAN_STATE_ALLOC:
+		pr_err("Object allocated with size %u bytes.\n",
+		       alloc_info->alloc_size);
+		pr_err("Allocation:\n");
+		print_track(&alloc_info->track);
+		break;
+	case KASAN_STATE_FREE:
+		pr_err("Object freed, allocated with size %u bytes\n",
+		       alloc_info->alloc_size);
+		free_info = get_free_info(cache, object);
+		pr_err("Allocation:\n");
+		print_track(&alloc_info->track);
+		pr_err("Deallocation:\n");
+		print_track(&free_info->track);
+		break;
+	}
+}
+#endif
+
 static void print_address_description(struct kasan_access_info *info)
 {
 	const void *addr = info->access_addr;

@@ -126,17 +164,14 @@ static void print_address_description(struct kasan_access_info *info)
 		if (PageSlab(page)) {
 			void *object;
 			struct kmem_cache *cache = page->slab_cache;
-			void *last_object;
-
-			object = virt_to_obj(cache, page_address(page), addr);
-			last_object = page_address(page) +

mm/kasan/report.c +2 -0

+		dump_stack();
+#endif
 		print_address_description(info);
 		print_shadow_for_address(info->first_bad_addr);
 	}

mm/slab.c +0 -0

index 621fbcb..805b39b 100644
 
 
 
 
 	if (gfpflags_allow_blocking(local_flags))

Quoted 191 lines · Alexander Potapenko

Add KASAN hooks to SLAB allocator.

This patch is based on the "mm: kasan: unified support for SLUB and SLAB allocators" patch originally prepared by Dmitry Chernenkov.

Signed-off-by Alexander Potapenko <[email protected]>

---
v3: - minor description changes
- store deallocation info in kasan_slab_free()

v4: - fix kbuild compile-time warnings in print_track()
---

mm/kasan/kasan.c +53 -0

index bc0a8d8..d26ffb4 100644
@@ -314,6 +314,59 @@ void kasan_free_pages(struct page *page, unsigned int order)
                              KASAN_FREE_PAGE);
 }
 
+#ifdef CONFIG_SLAB
+/*
+ * Adaptive redzone policy taken from the userspace AddressSanitizer runtime.
+ * For larger allocations larger redzones are used.
+ */
+static size_t optimal_redzone(size_t object_size)
+{
+     int rz =
+             object_size <= 64        - 16   ? 16 :
+             object_size <= 128       - 32   ? 32 :
+             object_size <= 512       - 64   ? 64 :
+             object_size <= 4096      - 128  ? 128 :
+             object_size <= (1 << 14) - 256  ? 256 :
+             object_size <= (1 << 15) - 512  ? 512 :
+             object_size <= (1 << 16) - 1024 ? 1024 : 2048;
+     return rz;
+}
+
+void kasan_cache_create(struct kmem_cache *cache, size_t *size,
+                     unsigned long *flags)
+{
+     int redzone_adjust;
+     /* Make sure the adjusted size is still less than
+      * KMALLOC_MAX_CACHE_SIZE.
+      * TODO: this check is only useful for SLAB, but not SLUB. We'll need
+      * to skip it for SLUB when it starts using kasan_cache_create().
+      */
+     if (*size > KMALLOC_MAX_CACHE_SIZE -
+         sizeof(struct kasan_alloc_meta) -
+         sizeof(struct kasan_free_meta))
+             return;
+     *flags |= SLAB_KASAN;
+     /* Add alloc meta. */
+     cache->kasan_info.alloc_meta_offset = *size;
+     *size += sizeof(struct kasan_alloc_meta);
+
+     /* Add free meta. */
+     if (cache->flags & SLAB_DESTROY_BY_RCU || cache->ctor ||
+         cache->object_size < sizeof(struct kasan_free_meta)) {
+             cache->kasan_info.free_meta_offset = *size;
+             *size += sizeof(struct kasan_free_meta);
+     }
+     redzone_adjust = optimal_redzone(cache->object_size) -
+             (*size - cache->object_size);
+     if (redzone_adjust > 0)
+             *size += redzone_adjust;
+     *size = min(KMALLOC_MAX_CACHE_SIZE,
+                 max(*size,
+                     cache->object_size +
+                     optimal_redzone(cache->object_size)));
+}
+#endif
+
 
 
 

void kasan_poison_slab(struct page *page)
{
kasan_poison_shadow(page_address(page),

mm/kasan/kasan.c +49 -0

@@ -331,8 +384,36 @@ void kasan_poison_object_data(struct kmem_cache *cache, void *object)
      kasan_poison_shadow(object,
                      round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE),
                      KASAN_KMALLOC_REDZONE);
+#ifdef CONFIG_SLAB
+     if (cache->flags & SLAB_KASAN) {
+             struct kasan_alloc_meta *alloc_info =
+                     get_alloc_info(cache, object);
+             alloc_info->state = KASAN_STATE_INIT;
+     }
+#endif
+}
+
+static inline void set_track(struct kasan_track *track)
+{
+     track->cpu = raw_smp_processor_id();
+     track->pid = current->pid;
+     track->when = jiffies;
 }
 
+#ifdef CONFIG_SLAB
+struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
+                                     const void *object)
+{
+     return (void *)object + cache->kasan_info.alloc_meta_offset;
+}
+
+struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
+                                   const void *object)
+{
+     return (void *)object + cache->kasan_info.free_meta_offset;
+}
+#endif
+
 void kasan_slab_alloc(struct kmem_cache *cache, void *object)
 {
      kasan_kmalloc(cache, object, cache->object_size);

@@ -347,6 +428,17 @@ void kasan_slab_free(struct kmem_cache *cache, void *object)
      if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
              return;
 
+#ifdef CONFIG_SLAB
+     if (cache->flags & SLAB_KASAN) {
+             struct kasan_free_meta *free_info =
+                     get_free_info(cache, object);
+             struct kasan_alloc_meta *alloc_info =
+                     get_alloc_info(cache, object);
+             alloc_info->state = KASAN_STATE_FREE;
+             set_track(&free_info->track);
+     }
+#endif
+
      kasan_poison_shadow(object, rounded_up_size, KASAN_KMALLOC_FREE);
 }
 

@@ -366,6 +458,16 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size)
      kasan_unpoison_shadow(object, size);
      kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start,
              KASAN_KMALLOC_REDZONE);
+#ifdef CONFIG_SLAB
+     if (cache->flags & SLAB_KASAN) {
+             struct kasan_alloc_meta *alloc_info =
+                     get_alloc_info(cache, object);
+
+             alloc_info->state = KASAN_STATE_ALLOC;
+             alloc_info->alloc_size = size;
+             set_track(&alloc_info->track);
+     }
+#endif
 }
 EXPORT_SYMBOL(kasan_kmalloc);
 

mm/kasan/kasan.h +29 -0

index 4f6c62e..7b9e4ab9 100644
@@ -54,6 +54,40 @@ struct kasan_global {
 #endif
 };
 
+/**
+ * Structures to keep alloc and free tracks *
+ */
+
+enum kasan_state {
+     KASAN_STATE_INIT,
+     KASAN_STATE_ALLOC,
+     KASAN_STATE_FREE
+};
+
+struct kasan_track {
+     u64 cpu : 6;                    /* for NR_CPUS = 64 */
+     u64 pid : 16;                   /* 65536 processes */
+     u64 when : 42;                  /* ~140 years */
+};
+
+struct kasan_alloc_meta {
+     u32 state : 2;  /* enum kasan_state */
+     u32 alloc_size : 30;
+     struct kasan_track track;
+};
+
+struct kasan_free_meta {
+     /* Allocator freelist pointer, unused by KASAN. */
+     void **freelist;
+     struct kasan_track track;
+};
+
+struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,

mm/kasan/kasan.h struct kasan_global { · line 90

Quoted 5 lines

mm/kasan/kasan.h +5 -0

+                                     const void *object);
+struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
+                                     const void *object);
+
+

Basically, all this big pile of code above is implementation of yet another SLAB_STORE_USER and SLAB_RED_ZONE exclusively for KASAN. It would be so much better to alter existing code to satisfy all you needs.

Quoted 17 lines

mm/kasan/kasan.h +0 -0

 static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
 {
      return (void *)(((unsigned long)shadow_addr - KASAN_SHADOW_OFFSET)

mm/kasan/report.c +6 -0

index 12f222d..2c1407f 100644
@@ -115,6 +115,44 @@ static inline bool init_task_stack_addr(const void *addr)
                      sizeof(init_thread_union.stack));
 }
 
+#ifdef CONFIG_SLAB
+static void print_track(struct kasan_track *track)
+{
+     pr_err("PID = %u, CPU = %u, timestamp = %lu\n", track->pid,
+            track->cpu, (unsigned long)track->when);
+}

mm/kasan/report.c static inline bool init_task_stack_addr(const void *addr) · line 133

Quoted 10 lines

mm/kasan/report.c +10 -0

+
+static void print_object(struct kmem_cache *cache, void *object)
+{
+     struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
+     struct kasan_free_meta *free_info;
+
+     pr_err("Object at %p, in cache %s\n", object, cache->name);
+     if (!(cache->flags & SLAB_KASAN))
+             return;
+     switch (alloc_info->state) {

'->state' seems useless. It's used only here, but object's state could be determined by shadow value.

Quoted 37 lines

mm/kasan/report.c +22 -8

+     case KASAN_STATE_INIT:
+             pr_err("Object not allocated yet\n");
+             break;
+     case KASAN_STATE_ALLOC:
+             pr_err("Object allocated with size %u bytes.\n",
+                    alloc_info->alloc_size);
+             pr_err("Allocation:\n");
+             print_track(&alloc_info->track);
+             break;
+     case KASAN_STATE_FREE:
+             pr_err("Object freed, allocated with size %u bytes\n",
+                    alloc_info->alloc_size);
+             free_info = get_free_info(cache, object);
+             pr_err("Allocation:\n");
+             print_track(&alloc_info->track);
+             pr_err("Deallocation:\n");
+             print_track(&free_info->track);
+             break;
+     }
+}
+#endif
+
 static void print_address_description(struct kasan_access_info *info)
 {
      const void *addr = info->access_addr;

@@ -126,17 +164,14 @@ static void print_address_description(struct kasan_access_info *info)
              if (PageSlab(page)) {
                      void *object;
                      struct kmem_cache *cache = page->slab_cache;
-                     void *last_object;
-
-                     object = virt_to_obj(cache, page_address(page), addr);
-                     last_object = page_address(page) +
-                             page->objects * cache->size;
-
-                     if (unlikely(object > last_object))
-                             object = last_object; /* we hit into padding */

mm/kasan/report.c static void print_address_description(struct kasan_access_info *info) · line 171

Quoted 6 lines

mm/kasan/report.c +5 -1

-
+                     object = nearest_obj(cache, page,
+                                             (void *)info->access_addr);
+#ifdef CONFIG_SLAB
+                     print_object(cache, object);
+#else

Instead of these ifdefs, please, make universal API for printing object's information.

Quoted 7 lines

mm/kasan/report.c +2 -1

                      object_err(cache, page, object,
-                             "kasan: bad access detected");
+                                     "kasan: bad access detected");
+#endif
                      return;
              }
              dump_page(page, "kasan: bad access detected");

mm/kasan/report.c static void print_address_description(struct kasan_access_info *info) · line 184

Quoted 6 lines

mm/kasan/report.c +1 -1

@@ -146,8 +181,9 @@ static void print_address_description(struct kasan_access_info *info)
              if (!init_task_stack_addr(addr))
                      pr_err("Address belongs to variable %pS\n", addr);
      }
-
+#ifdef CONFIG_SLUB

???

Quoted 5 lines

mm/kasan/report.c +1 -0

      dump_stack();
+#endif
 }
 

mm/kasan/report.c static void kasan_report_error(struct kasan_access_info *info) · line 272

Quoted 6 lines

mm/kasan/report.c +1 -0

 static bool row_is_guilty(const void *row, const void *guilty)

@@ -233,6 +269,9 @@ static void kasan_report_error(struct kasan_access_info *info)
              dump_stack();
      } else {
              print_error_description(info);
+#ifdef CONFIG_SLAB

I'm lost here. What's the point of reordering dump_stack() for CONFIG_SLAB=y?

Quoted 11 lines

mm/kasan/report.c +2 -0

+             dump_stack();
+#endif
              print_address_description(info);
              print_shadow_for_address(info->first_bad_addr);
      }

mm/slab.c +0 -0

index 621fbcb..805b39b 100644

mm/slab.c free_done: · line 3380

Quoted 10 lines

if (gfpflags_allow_blocking(local_flags))

mm/slab.c +4 -1

@@ -3364,7 +3374,10 @@ free_done:
 static inline void __cache_free(struct kmem_cache *cachep, void *objp,
                              unsigned long caller)
 {
-     struct array_cache *ac = cpu_cache_get(cachep);
+     struct array_cache *ac;
+
+     kasan_slab_free(cachep, objp);
+     ac = cpu_cache_get(cachep);

Why cpu_cache_get() was moved? Looks like unnecessary change.

mm/slab.c static inline void __cache_free(struct kmem_cache *cachep, void *objp, · line 3419

Quoted 8 lines

mm/slab.c +1 -0

      check_irq_off();
      kmemleak_free_recursive(objp, cachep->flags);

@@ -3403,6 +3416,8 @@ static inline void __cache_free(struct kmem_cache *cachep, void *objp,
 void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
 {
      void *ret = slab_alloc(cachep, flags, _RET_IP_);
+     if (ret)

kasan_slab_alloc() should deal fine with ret == NULL.

Quoted 5 lines

mm/slab.c +1 -0

+             kasan_slab_alloc(cachep, ret);
 
      trace_kmem_cache_alloc(_RET_IP_, ret,
                             cachep->object_size, cachep->size, flags);

Quoted 194 lines · Andrey Ryabinin

Quoted 191 lines · Alexander Potapenko

Add KASAN hooks to SLAB allocator.

This patch is based on the "mm: kasan: unified support for SLUB and SLAB allocators" patch originally prepared by Dmitry Chernenkov.

Signed-off-by Alexander Potapenko <[email protected]>

---
v3: - minor description changes
- store deallocation info in kasan_slab_free()

v4: - fix kbuild compile-time warnings in print_track()
---

mm/kasan/kasan.c +53 -0

index bc0a8d8..d26ffb4 100644
@@ -314,6 +314,59 @@ void kasan_free_pages(struct page *page, unsigned int order)
                              KASAN_FREE_PAGE);
 }
 
+#ifdef CONFIG_SLAB
+/*
+ * Adaptive redzone policy taken from the userspace AddressSanitizer runtime.
+ * For larger allocations larger redzones are used.
+ */
+static size_t optimal_redzone(size_t object_size)
+{
+     int rz =
+             object_size <= 64        - 16   ? 16 :
+             object_size <= 128       - 32   ? 32 :
+             object_size <= 512       - 64   ? 64 :
+             object_size <= 4096      - 128  ? 128 :
+             object_size <= (1 << 14) - 256  ? 256 :
+             object_size <= (1 << 15) - 512  ? 512 :
+             object_size <= (1 << 16) - 1024 ? 1024 : 2048;
+     return rz;
+}
+
+void kasan_cache_create(struct kmem_cache *cache, size_t *size,
+                     unsigned long *flags)
+{
+     int redzone_adjust;
+     /* Make sure the adjusted size is still less than
+      * KMALLOC_MAX_CACHE_SIZE.
+      * TODO: this check is only useful for SLAB, but not SLUB. We'll need
+      * to skip it for SLUB when it starts using kasan_cache_create().
+      */
+     if (*size > KMALLOC_MAX_CACHE_SIZE -
+         sizeof(struct kasan_alloc_meta) -
+         sizeof(struct kasan_free_meta))
+             return;
+     *flags |= SLAB_KASAN;
+     /* Add alloc meta. */
+     cache->kasan_info.alloc_meta_offset = *size;
+     *size += sizeof(struct kasan_alloc_meta);
+
+     /* Add free meta. */
+     if (cache->flags & SLAB_DESTROY_BY_RCU || cache->ctor ||
+         cache->object_size < sizeof(struct kasan_free_meta)) {
+             cache->kasan_info.free_meta_offset = *size;
+             *size += sizeof(struct kasan_free_meta);
+     }
+     redzone_adjust = optimal_redzone(cache->object_size) -
+             (*size - cache->object_size);
+     if (redzone_adjust > 0)
+             *size += redzone_adjust;
+     *size = min(KMALLOC_MAX_CACHE_SIZE,
+                 max(*size,
+                     cache->object_size +
+                     optimal_redzone(cache->object_size)));
+}
+#endif
+
 
 
 

void kasan_poison_slab(struct page *page)
{
kasan_poison_shadow(page_address(page),

mm/kasan/kasan.c +49 -0

@@ -331,8 +384,36 @@ void kasan_poison_object_data(struct kmem_cache *cache, void *object)
      kasan_poison_shadow(object,
                      round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE),
                      KASAN_KMALLOC_REDZONE);
+#ifdef CONFIG_SLAB
+     if (cache->flags & SLAB_KASAN) {
+             struct kasan_alloc_meta *alloc_info =
+                     get_alloc_info(cache, object);
+             alloc_info->state = KASAN_STATE_INIT;
+     }
+#endif
+}
+
+static inline void set_track(struct kasan_track *track)
+{
+     track->cpu = raw_smp_processor_id();
+     track->pid = current->pid;
+     track->when = jiffies;
 }
 
+#ifdef CONFIG_SLAB
+struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
+                                     const void *object)
+{
+     return (void *)object + cache->kasan_info.alloc_meta_offset;
+}
+
+struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
+                                   const void *object)
+{
+     return (void *)object + cache->kasan_info.free_meta_offset;
+}
+#endif
+
 void kasan_slab_alloc(struct kmem_cache *cache, void *object)
 {
      kasan_kmalloc(cache, object, cache->object_size);

@@ -347,6 +428,17 @@ void kasan_slab_free(struct kmem_cache *cache, void *object)
      if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
              return;
 
+#ifdef CONFIG_SLAB
+     if (cache->flags & SLAB_KASAN) {
+             struct kasan_free_meta *free_info =
+                     get_free_info(cache, object);
+             struct kasan_alloc_meta *alloc_info =
+                     get_alloc_info(cache, object);
+             alloc_info->state = KASAN_STATE_FREE;
+             set_track(&free_info->track);
+     }
+#endif
+
      kasan_poison_shadow(object, rounded_up_size, KASAN_KMALLOC_FREE);
 }
 

@@ -366,6 +458,16 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size)
      kasan_unpoison_shadow(object, size);
      kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start,
              KASAN_KMALLOC_REDZONE);
+#ifdef CONFIG_SLAB
+     if (cache->flags & SLAB_KASAN) {
+             struct kasan_alloc_meta *alloc_info =
+                     get_alloc_info(cache, object);
+
+             alloc_info->state = KASAN_STATE_ALLOC;
+             alloc_info->alloc_size = size;
+             set_track(&alloc_info->track);
+     }
+#endif
 }
 EXPORT_SYMBOL(kasan_kmalloc);
 

mm/kasan/kasan.h +29 -0

index 4f6c62e..7b9e4ab9 100644
@@ -54,6 +54,40 @@ struct kasan_global {
 #endif
 };
 
+/**
+ * Structures to keep alloc and free tracks *
+ */
+
+enum kasan_state {
+     KASAN_STATE_INIT,
+     KASAN_STATE_ALLOC,
+     KASAN_STATE_FREE
+};
+
+struct kasan_track {
+     u64 cpu : 6;                    /* for NR_CPUS = 64 */
+     u64 pid : 16;                   /* 65536 processes */
+     u64 when : 42;                  /* ~140 years */
+};
+
+struct kasan_alloc_meta {
+     u32 state : 2;  /* enum kasan_state */
+     u32 alloc_size : 30;
+     struct kasan_track track;
+};
+
+struct kasan_free_meta {
+     /* Allocator freelist pointer, unused by KASAN. */
+     void **freelist;
+     struct kasan_track track;
+};
+
+struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,

Quoted 8 lines

mm/kasan/kasan.h struct kasan_global { · line 90

Quoted 5 lines

mm/kasan/kasan.h +5 -0

+                                     const void *object);
+struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
+                                     const void *object);
+
+

Basically, all this big pile of code above is implementation of yet another SLAB_STORE_USER and SLAB_RED_ZONE exclusively for KASAN. It would be so much better to alter existing code to satisfy all you needs.

Thanks for the suggestion, I will take a look.

Quoted 29 lines

Quoted 17 lines

mm/kasan/kasan.h +0 -0

 static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
 {
      return (void *)(((unsigned long)shadow_addr - KASAN_SHADOW_OFFSET)

mm/kasan/report.c +6 -0

index 12f222d..2c1407f 100644
@@ -115,6 +115,44 @@ static inline bool init_task_stack_addr(const void *addr)
                      sizeof(init_thread_union.stack));
 }
 
+#ifdef CONFIG_SLAB
+static void print_track(struct kasan_track *track)
+{
+     pr_err("PID = %u, CPU = %u, timestamp = %lu\n", track->pid,
+            track->cpu, (unsigned long)track->when);
+}

mm/kasan/report.c static inline bool init_task_stack_addr(const void *addr) · line 133

Quoted 10 lines

mm/kasan/report.c +10 -0

+
+static void print_object(struct kmem_cache *cache, void *object)
+{
+     struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
+     struct kasan_free_meta *free_info;
+
+     pr_err("Object at %p, in cache %s\n", object, cache->name);
+     if (!(cache->flags & SLAB_KASAN))
+             return;
+     switch (alloc_info->state) {

'->state' seems useless. It's used only here, but object's state could be determined by shadow value.

Quoted 37 lines

Quoted 37 lines

mm/kasan/report.c +22 -8

+     case KASAN_STATE_INIT:
+             pr_err("Object not allocated yet\n");
+             break;
+     case KASAN_STATE_ALLOC:
+             pr_err("Object allocated with size %u bytes.\n",
+                    alloc_info->alloc_size);
+             pr_err("Allocation:\n");
+             print_track(&alloc_info->track);
+             break;
+     case KASAN_STATE_FREE:
+             pr_err("Object freed, allocated with size %u bytes\n",
+                    alloc_info->alloc_size);
+             free_info = get_free_info(cache, object);
+             pr_err("Allocation:\n");
+             print_track(&alloc_info->track);
+             pr_err("Deallocation:\n");
+             print_track(&free_info->track);
+             break;
+     }
+}
+#endif
+
 static void print_address_description(struct kasan_access_info *info)
 {
      const void *addr = info->access_addr;

@@ -126,17 +164,14 @@ static void print_address_description(struct kasan_access_info *info)
              if (PageSlab(page)) {
                      void *object;
                      struct kmem_cache *cache = page->slab_cache;
-                     void *last_object;
-
-                     object = virt_to_obj(cache, page_address(page), addr);
-                     last_object = page_address(page) +
-                             page->objects * cache->size;
-
-                     if (unlikely(object > last_object))
-                             object = last_object; /* we hit into padding */

Quoted 8 lines

mm/kasan/report.c static void print_address_description(struct kasan_access_info *info) · line 171

Quoted 6 lines

mm/kasan/report.c +5 -1

-
+                     object = nearest_obj(cache, page,
+                                             (void *)info->access_addr);
+#ifdef CONFIG_SLAB
+                     print_object(cache, object);
+#else

Instead of these ifdefs, please, make universal API for printing object's information.

My intention here was to touch the SLUB functionality as little as possible to avoid the mess and feature regressions.
I'll be happy to refactor the code in the upcoming patches once this one is landed.

Quoted 7 lines

Quoted 7 lines

mm/kasan/report.c +2 -1

                      object_err(cache, page, object,
-                             "kasan: bad access detected");
+                                     "kasan: bad access detected");
+#endif
                      return;
              }
              dump_page(page, "kasan: bad access detected");

Quoted 8 lines

mm/kasan/report.c static void print_address_description(struct kasan_access_info *info) · line 184

Quoted 6 lines

mm/kasan/report.c +1 -1

@@ -146,8 +181,9 @@ static void print_address_description(struct kasan_access_info *info)
              if (!init_task_stack_addr(addr))
                      pr_err("Address belongs to variable %pS\n", addr);
      }
-
+#ifdef CONFIG_SLUB

???

Not sure what did you mean here, assuming this comment is related to the next one.

Quoted 5 lines

Quoted 5 lines

mm/kasan/report.c +1 -0

      dump_stack();
+#endif
 }
 

Quoted 8 lines

mm/kasan/report.c static void kasan_report_error(struct kasan_access_info *info) · line 272

Quoted 6 lines

mm/kasan/report.c +1 -0

 static bool row_is_guilty(const void *row, const void *guilty)

@@ -233,6 +269,9 @@ static void kasan_report_error(struct kasan_access_info *info)
              dump_stack();
      } else {
              print_error_description(info);
+#ifdef CONFIG_SLAB

I'm lost here. What's the point of reordering dump_stack() for CONFIG_SLAB=y?

I should have documented this in the patch description properly. My intention is to make the KASAN reports look more like those in the userspace AddressSanitizer, so I'm moving the memory access stack to the top of the report.
Having seen hundreds and hundreds of ASan reports, we believe that important information must go at the beginning of the error report. First, people usually do not need to read further once they see the access stack.
Second, the whole report may simply not make it to the log (e.g. in the case of a premature shutdown or remote log collection). As said before, I wasn't going to touch the SLUB output format in this patch set, but that also needs to be fixed (I'd also remove some unnecessary info, e.g. the memory dump).

Quoted 11 lines

Quoted 11 lines

mm/kasan/report.c +2 -0

+             dump_stack();
+#endif
              print_address_description(info);
              print_shadow_for_address(info->first_bad_addr);
      }

mm/slab.c +0 -0

index 621fbcb..805b39b 100644

Quoted 12 lines

mm/slab.c free_done: · line 3380

Quoted 10 lines

if (gfpflags_allow_blocking(local_flags))

mm/slab.c +4 -1

@@ -3364,7 +3374,10 @@ free_done:
 static inline void __cache_free(struct kmem_cache *cachep, void *objp,
                              unsigned long caller)
 {
-     struct array_cache *ac = cpu_cache_get(cachep);
+     struct array_cache *ac;
+
+     kasan_slab_free(cachep, objp);
+     ac = cpu_cache_get(cachep);

Why cpu_cache_get() was moved? Looks like unnecessary change.

Agreed.

Quoted 10 lines

mm/slab.c static inline void __cache_free(struct kmem_cache *cachep, void *objp, · line 3419

Quoted 8 lines

mm/slab.c +1 -0

      check_irq_off();
      kmemleak_free_recursive(objp, cachep->flags);

@@ -3403,6 +3416,8 @@ static inline void __cache_free(struct kmem_cache *cachep, void *objp,
 void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
 {
      void *ret = slab_alloc(cachep, flags, _RET_IP_);
+     if (ret)

kasan_slab_alloc() should deal fine with ret == NULL.

And it actually does. I'll remove this code in the updated patch set.

Quoted 5 lines

mm/slab.c static inline void __cache_free(struct kmem_cache *cachep, void *objp, · line 3423

Quoted 5 lines

mm/slab.c +1 -0

+             kasan_slab_alloc(cachep, ret);
 
      trace_kmem_cache_alloc(_RET_IP_, ret,
                             cachep->object_size, cachep->size, flags);

--
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
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks.

Quoted 15 lines

Quoted 15 lines

static void print_address_description(struct kasan_access_info *info)

 {
      const void *addr = info->access_addr;

mm/kasan/report.c +0 -8

@@ -126,17 +164,14 @@ static void print_address_description(struct kasan_access_info *info)
              if (PageSlab(page)) {
                      void *object;
                      struct kmem_cache *cache = page->slab_cache;
-                     void *last_object;
-
-                     object = virt_to_obj(cache, page_address(page), addr);
-                     last_object = page_address(page) +
-                             page->objects * cache->size;
-
-                     if (unlikely(object > last_object))
-                             object = last_object; /* we hit into padding */

Quoted 8 lines

mm/kasan/report.c static void print_address_description(struct kasan_access_info *info) · line 171

Quoted 6 lines

mm/kasan/report.c +5 -1

-
+                     object = nearest_obj(cache, page,
+                                             (void *)info->access_addr);
+#ifdef CONFIG_SLAB
+                     print_object(cache, object);
+#else

Instead of these ifdefs, please, make universal API for printing object's information.

My intention here was to touch the SLUB functionality as little as possible to avoid the mess and feature regressions.
I'll be happy to refactor the code in the upcoming patches once this one is landed.

Quoted 7 lines

Quoted 7 lines

mm/kasan/report.c +2 -1

                      object_err(cache, page, object,
-                             "kasan: bad access detected");
+                                     "kasan: bad access detected");
+#endif
                      return;
              }
              dump_page(page, "kasan: bad access detected");

Quoted 8 lines

mm/kasan/report.c static void print_address_description(struct kasan_access_info *info) · line 184

Quoted 6 lines

mm/kasan/report.c +1 -1

@@ -146,8 +181,9 @@ static void print_address_description(struct kasan_access_info *info)
              if (!init_task_stack_addr(addr))
                      pr_err("Address belongs to variable %pS\n", addr);
      }
-
+#ifdef CONFIG_SLUB

???

Not sure what did you mean here, assuming this comment is related to the next one.

Quoted 5 lines

Quoted 5 lines

mm/kasan/report.c +1 -0

      dump_stack();
+#endif
 }
 

Quoted 8 lines

mm/kasan/report.c static void kasan_report_error(struct kasan_access_info *info) · line 272

Quoted 6 lines

mm/kasan/report.c +1 -0

 static bool row_is_guilty(const void *row, const void *guilty)

@@ -233,6 +269,9 @@ static void kasan_report_error(struct kasan_access_info *info)
              dump_stack();
      } else {
              print_error_description(info);
+#ifdef CONFIG_SLAB

I'm lost here. What's the point of reordering dump_stack() for CONFIG_SLAB=y?

I should have documented this in the patch description properly. My intention is to make the KASAN reports look more like those in the userspace AddressSanitizer, so I'm moving the memory access stack to the top of the report.
Having seen hundreds and hundreds of ASan reports, we believe that important information must go at the beginning of the error report. First, people usually do not need to read further once they see the access stack.
Second, the whole report may simply not make it to the log (e.g. in the case of a premature shutdown or remote log collection).

As said before, I wasn't going to touch the SLUB output format in this patch set, but that also needs to be fixed (I'd also remove some unnecessary info, e.g. the memory dump).

Quoted 8 lines

mm/slab.c

Quoted 7 lines

mm/kasan/report.c +2 -0

+             dump_stack();
+#endif
              print_address_description(info);
              print_shadow_for_address(info->first_bad_addr);
      }

mm/slab.c +0 -0

index 621fbcb..805b39b 100644

KASAN needs to know whether the allocation happens in an IRQ handler. This lets us strip everything below the IRQ entry point to reduce the number of unique stack traces needed to be stored.

Move the definition of __irq_entry to <linux/interrupt.h> so that the users don't need to pull in <linux/ftrace.h>. Also introduce the __softirq_entry macro which is similar to __irq_entry, but puts the corresponding functions to the .softirqentry.text section.

Signed-off-by Alexander Potapenko <[email protected]>

---
v2: - per request from Steven Rostedt, moved the declarations of __softirq_entry and __irq_entry to <linux/interrupt.h>

v3: - minor description changes

---
 arch/arm/kernel/vmlinux.lds.S        |  1 +
 arch/arm64/kernel/vmlinux.lds.S      |  1 +
 arch/blackfin/kernel/vmlinux.lds.S   |  1 +
 arch/c6x/kernel/vmlinux.lds.S        |  1 +
 arch/metag/kernel/vmlinux.lds.S      |  1 +
 arch/microblaze/kernel/vmlinux.lds.S |  1 +
 arch/mips/kernel/vmlinux.lds.S       |  1 +
 arch/nios2/kernel/vmlinux.lds.S      |  1 +
 arch/openrisc/kernel/vmlinux.lds.S   |  1 +
 arch/parisc/kernel/vmlinux.lds.S     |  1 +
 arch/powerpc/kernel/vmlinux.lds.S    |  1 +
 arch/s390/kernel/vmlinux.lds.S       |  1 +
 arch/sh/kernel/vmlinux.lds.S         |  1 +
 arch/sparc/kernel/vmlinux.lds.S      |  1 +

 arch/tile/kernel/vmlinux.lds.S       |  1 +
 arch/x86/kernel/vmlinux.lds.S        |  1 +
 include/asm-generic/vmlinux.lds.h    | 12 +++++++++++-
 include/linux/ftrace.h               | 11 -----------
 include/linux/interrupt.h            | 20 ++++++++++++++++++++
 kernel/softirq.c                     |  2 +-
 kernel/trace/trace_functions_graph.c |  1 +
 21 files changed, 49 insertions(+), 13 deletions(-)

Stack depot will allow KASAN store allocation/deallocation stack traces for memory chunks. The stack traces are stored in a hash table and referenced by handles which reside in the kasan_alloc_meta and kasan_free_meta structures in the allocated memory chunks.

IRQ stack traces are cut below the IRQ entry point to avoid unnecessary duplication.

Right now stackdepot support is only enabled in SLAB allocator. Once KASAN features in SLAB are on par with those in SLUB we can switch SLUB to stackdepot as well, thus removing the dependency on SLUB stack bookkeeping, which wastes a lot of memory.

This patch is based on the "mm: kasan: stack depots" patch originally prepared by Dmitry Chernenkov.

Signed-off-by Alexander Potapenko <[email protected]>

---
v2: - per request from Joonsoo Kim, moved the stackdepot implementation to lib/, as there's a plan to use it for page owner
- added copyright comments
- added comments about smp_load_acquire()/smp_store_release()

v3: - minor description changes
---

lib/Makefile +0 -0

index a7c26a4..10a4ae3 100644

lib/Makefile +5 -0

+	obj-y	+= stackdepot.o
+	KASAN_SANITIZE_slub.o := n
+endif
+endif
+
 libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \
 	       fdt_empty_tree.o
 $(foreach file, $(libfdt_files), \

lib/stackdepot.c new file +2 -0

new file mode 100644
index 0000000..f09b0da
 
 
+/* Allocation of a new stack in raw storage */
+static struct stack_record *depot_alloc_stack(unsigned long *entries, int size,

lib/stackdepot.c new file +41 -0

+	depot_offset += required_size;
+
+	return stack;
+}
+
 
 
+/*
+ * depot_save_stack - save stack in a stack depot.
+ * @trace - the stacktrace to save.
+ * @alloc_flags - flags for allocating additional memory if required.
+ *
+ * Returns the handle of the stack struct stored in depot.
+ */
+depot_stack_handle depot_save_stack(struct stack_trace *trace,
+				    gfp_t alloc_flags)
+{
+	u32 hash;
+	depot_stack_handle retval = 0;
+	struct stack_record *found = NULL, **bucket;
+	unsigned long flags;
+	struct page *page = NULL;
+	void *prealloc = NULL;
+
+	if (unlikely(trace->nr_entries == 0))
+		goto exit;
+
+	hash = hash_stack(trace->entries, trace->nr_entries);
+	/* Bad luck, we won't store this stack. */
+	if (hash == 0)
+		goto exit;
+
+	bucket = &stack_table[hash & STACK_HASH_MASK];
+
+	/* Fast path: look the stack trace up without locking.
+	 *
+	 * The smp_load_acquire() here pairs with smp_store_release() to
+	 * |bucket| below.
+	 */
+	found = find_stack(smp_load_acquire(bucket), trace->entries,
+			   trace->nr_entries, hash);
+	if (found)
+		goto exit;

mm/kasan/kasan.h +1 -0

index 7b9e4ab9..b4e5942 100644
@@ -2,6 +2,7 @@
 #define __MM_KASAN_KASAN_H
 
 #include <linux/kasan.h>
+#include <linux/stackdepot.h>
 
 #define KASAN_SHADOW_SCALE_SIZE (1UL << KASAN_SHADOW_SCALE_SHIFT)
 #define KASAN_SHADOW_MASK       (KASAN_SHADOW_SCALE_SIZE - 1)

Quoted 33 lines · Alexander Potapenko

Stack depot will allow KASAN store allocation/deallocation stack traces for memory chunks. The stack traces are stored in a hash table and referenced by handles which reside in the kasan_alloc_meta and kasan_free_meta structures in the allocated memory chunks.

IRQ stack traces are cut below the IRQ entry point to avoid unnecessary duplication.

Right now stackdepot support is only enabled in SLAB allocator. Once KASAN features in SLAB are on par with those in SLUB we can switch SLUB to stackdepot as well, thus removing the dependency on SLUB stack bookkeeping, which wastes a lot of memory.

This patch is based on the "mm: kasan: stack depots" patch originally prepared by Dmitry Chernenkov.

Signed-off-by Alexander Potapenko <[email protected]>

---
v2: - per request from Joonsoo Kim, moved the stackdepot implementation to lib/, as there's a plan to use it for page owner
- added copyright comments
- added comments about smp_load_acquire()/smp_store_release()

v3: - minor description changes
---

lib/Makefile +0 -0

index a7c26a4..10a4ae3 100644

lib/Makefile obj-$(CONFIG_SG_SPLIT) += sg_split.o · line 171

Quoted 5 lines

lib/Makefile +2 -0

@@ -167,6 +167,13 @@ obj-$(CONFIG_SG_SPLIT) += sg_split.o
 obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
 obj-$(CONFIG_IRQ_POLL) += irq_poll.o
 
+ifeq ($(CONFIG_KASAN),y)
+ifeq ($(CONFIG_SLAB),y)

Just try to imagine that another subsystem wants to use stackdepot. How this gonna look like?

We have Kconfig to describe dependencies. So, this should be under CONFIG_STACKDEPOT. So any user of this feature can just do 'select STACKDEPOT' in Kconfig.

Quoted 19 lines

lib/Makefile +5 -0

+     obj-y   += stackdepot.o
+     KASAN_SANITIZE_slub.o := n
+endif
+endif
+
 libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \
             fdt_empty_tree.o
 $(foreach file, $(libfdt_files), \

lib/stackdepot.c new file +4 -0

new file mode 100644
index 0000000..f09b0da
 
 
+/* Allocation of a new stack in raw storage */
+static struct stack_record *depot_alloc_stack(unsigned long *entries, int size,
+             u32 hash, void **prealloc, gfp_t alloc_flags)
+{

lib/stackdepot.c line 135

Quoted 6 lines

lib/stackdepot.c new file +6 -0

+
+     stack->hash = hash;
+     stack->size = size;
+     stack->handle.slabindex = depot_index;
+     stack->handle.offset = depot_offset >> STACK_ALLOC_ALIGN;
+     __memcpy(stack->entries, entries, size * sizeof(unsigned long));

s/__memcpy/memcpy/

Quoted 49 lines

lib/stackdepot.c new file +47 -0

+     depot_offset += required_size;
+
+     return stack;
+}
+
 
 
+/*
+ * depot_save_stack - save stack in a stack depot.
+ * @trace - the stacktrace to save.
+ * @alloc_flags - flags for allocating additional memory if required.
+ *
+ * Returns the handle of the stack struct stored in depot.
+ */
+depot_stack_handle depot_save_stack(struct stack_trace *trace,
+                                 gfp_t alloc_flags)
+{
+     u32 hash;
+     depot_stack_handle retval = 0;
+     struct stack_record *found = NULL, **bucket;
+     unsigned long flags;
+     struct page *page = NULL;
+     void *prealloc = NULL;
+
+     if (unlikely(trace->nr_entries == 0))
+             goto exit;
+
+     hash = hash_stack(trace->entries, trace->nr_entries);
+     /* Bad luck, we won't store this stack. */
+     if (hash == 0)
+             goto exit;
+
+     bucket = &stack_table[hash & STACK_HASH_MASK];
+
+     /* Fast path: look the stack trace up without locking.
+      *
+      * The smp_load_acquire() here pairs with smp_store_release() to
+      * |bucket| below.
+      */
+     found = find_stack(smp_load_acquire(bucket), trace->entries,
+                        trace->nr_entries, hash);
+     if (found)
+             goto exit;
+
+     /* Check if the current or the next stack slab need to be initialized.
+      * If so, allocate the memory - we won't be able to do that under the
+      * lock.
+      *
+      * The smp_load_acquire() here pairs with smp_store_release() to

lib/stackdepot.c line 233

Quoted 4 lines

lib/stackdepot.c new file +4 -0

+      * |next_slab_inited| in depot_alloc_stack() and init_stack_slab().
+      */
+     if (unlikely(!smp_load_acquire(&next_slab_inited))) {
+             if (!preempt_count() && !in_irq()) {

If you trying to detect atomic context here, than this doesn't work. E.g. you can't know about held spinlocks in non-preemptible kernel.
And I'm not sure why need this. You know gfp flags here, so allocation in atomic context shouldn't be problem.

lib/stackdepot.c line 236

Quoted 3 lines

lib/stackdepot.c new file +3 -0

+                     alloc_flags &= (__GFP_RECLAIM | __GFP_IO | __GFP_FS |
+                             __GFP_NOWARN | __GFP_NORETRY |
+                             __GFP_NOMEMALLOC | __GFP_DIRECT_RECLAIM);

I think blacklist approach would be better here.

Quoted 1 line

+ page = alloc_pages(alloc_flags, STACK_ALLOC_ORDER);

STACK_ALLOC_ORDER = 4 - that's a lot. Do you really need that much?

mm/kasan/Makefile CFLAGS_REMOVE_kasan.o = -pg · line 10

Quoted 9 lines

mm/kasan/Makefile +1 -0

index a61460d..32bd73a 100644
@@ -7,3 +7,4 @@ CFLAGS_REMOVE_kasan.o = -pg
 CFLAGS_kasan.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
 
 obj-y := kasan.o report.o kasan_init.o
+

Extra newline.

Quoted 16 lines

mm/kasan/kasan.h +1 -0

index 7b9e4ab9..b4e5942 100644
@@ -2,6 +2,7 @@
 #define __MM_KASAN_KASAN_H
 
 #include <linux/kasan.h>
+#include <linux/stackdepot.h>
 
 #define KASAN_SHADOW_SCALE_SIZE (1UL << KASAN_SHADOW_SCALE_SHIFT)
 #define KASAN_SHADOW_MASK       (KASAN_SHADOW_SCALE_SIZE - 1)

@@ -64,10 +65,13 @@ enum kasan_state {
      KASAN_STATE_FREE
 };
 

mm/kasan/kasan.h enum kasan_state { · line 68

Quoted 1 line

mm/kasan/kasan.h +1 -0

+#define KASAN_STACK_DEPTH 64

I think, you can reduce this (32 perhaps?). Kernel stacks are not so deep usually.

mm/kasan/kasan.h enum kasan_state { · line 76

Quoted 8 lines

mm/kasan/kasan.h +2 -0

+
 struct kasan_track {
      u64 cpu : 6;                    /* for NR_CPUS = 64 */
      u64 pid : 16;                   /* 65536 processes */
      u64 when : 42;                  /* ~140 years */
+     depot_stack_handle stack : sizeof(depot_stack_handle);
 };
 

Quoted 13 lines

lib/Makefile obj-$(CONFIG_SG_SPLIT) += sg_split.o · line 171

Quoted 10 lines

lib/Makefile +2 -0

index a7c26a4..10a4ae3 100644
@@ -167,6 +167,13 @@ obj-$(CONFIG_SG_SPLIT) += sg_split.o
 obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
 obj-$(CONFIG_IRQ_POLL) += irq_poll.o
 
+ifeq ($(CONFIG_KASAN),y)
+ifeq ($(CONFIG_SLAB),y)

Just try to imagine that another subsystem wants to use stackdepot. How this gonna look like?

Quoted 5 lines

We have Kconfig to describe dependencies. So, this should be under CONFIG_STACKDEPOT. So any user of this feature can just do 'select STACKDEPOT' in Kconfig.

Quoted 2 lines

lib/Makefile +2 -0

+     obj-y   += stackdepot.o
+     KASAN_SANITIZE_slub.o := n

Quoted 8 lines

Quoted 7 lines

lib/Makefile +6 -0

+
+     stack->hash = hash;
+     stack->size = size;
+     stack->handle.slabindex = depot_index;
+     stack->handle.offset = depot_offset >> STACK_ALLOC_ALIGN;
+     __memcpy(stack->entries, entries, size * sizeof(unsigned long));

Quoted 1 line

s/__memcpy/memcpy/

memcpy should be instrumented by asan/tsan, and we would like to avoid that instrumentation here.

Quoted 6 lines

lib/Makefile obj-$(CONFIG_SG_SPLIT) += sg_split.o

Quoted 2 lines

lib/Makefile +2 -0

+     if (unlikely(!smp_load_acquire(&next_slab_inited))) {
+             if (!preempt_count() && !in_irq()) {

If you trying to detect atomic context here, than this doesn't work. E.g. you can't know about held spinlocks in non-preemptible kernel.
And I'm not sure why need this. You know gfp flags here, so allocation in atomic context shouldn't be problem.

We don't have gfp flags for kfree.
I wonder how CONFIG_DEBUG_ATOMIC_SLEEP handles this. Maybe it has the answer.

Alternatively, we can always assume that we are in atomic context in kfree.

Quoted 11 lines

lib/Makefile

Quoted 5 lines

lib/Makefile +3 -0

+                     alloc_flags &= (__GFP_RECLAIM | __GFP_IO | __GFP_FS |
+                             __GFP_NOWARN | __GFP_NORETRY |
+                             __GFP_NOMEMALLOC | __GFP_DIRECT_RECLAIM);

I think blacklist approach would be better here.

Quoted 1 line

+ page = alloc_pages(alloc_flags, STACK_ALLOC_ORDER);

STACK_ALLOC_ORDER = 4 - that's a lot. Do you really need that much?

Part of the issue the atomic context above. When we can't allocate memory we still want to save the stack trace. When we have less than STACK_ALLOC_ORDER memory, we try to preallocate another STACK_ALLOC_ORDER in advance. So in the worst case, we have STACK_ALLOC_ORDER memory and that should be enough to handle all kmalloc/kfree in the atomic context. 1 page does not look enough. I think Alex did some measuring of the failure race (when we are out of memory and can't allocate more).

Quoted 13 lines · Dmitry Vyukov

Quoted 13 lines

lib/Makefile obj-$(CONFIG_SG_SPLIT) += sg_split.o · line 171

Quoted 10 lines

lib/Makefile +2 -0

index a7c26a4..10a4ae3 100644
@@ -167,6 +167,13 @@ obj-$(CONFIG_SG_SPLIT) += sg_split.o
 obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
 obj-$(CONFIG_IRQ_POLL) += irq_poll.o
 
+ifeq ($(CONFIG_KASAN),y)
+ifeq ($(CONFIG_SLAB),y)

Just try to imagine that another subsystem wants to use stackdepot. How this gonna look like?

Quoted 5 lines

Quoted 5 lines

We have Kconfig to describe dependencies. So, this should be under CONFIG_STACKDEPOT. So any user of this feature can just do 'select STACKDEPOT' in Kconfig.

Quoted 2 lines

lib/Makefile +2 -0

+     obj-y   += stackdepot.o
+     KASAN_SANITIZE_slub.o := n

_stackdepot.o

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

lib/Makefile +6 -0

+
+     stack->hash = hash;
+     stack->size = size;
+     stack->handle.slabindex = depot_index;
+     stack->handle.offset = depot_offset >> STACK_ALLOC_ALIGN;
+     __memcpy(stack->entries, entries, size * sizeof(unsigned long));

Quoted 4 lines

Quoted 1 line

s/__memcpy/memcpy/

memcpy should be instrumented by asan/tsan, and we would like to avoid that instrumentation here.

KASAN_SANITIZE_* := n already takes care about this.
__memcpy() is a special thing solely for kasan internals and some assembly code. And it's not available generally.

Quoted 10 lines

Quoted 6 lines

lib/Makefile obj-$(CONFIG_SG_SPLIT) += sg_split.o

Quoted 2 lines

lib/Makefile +2 -0

+     if (unlikely(!smp_load_acquire(&next_slab_inited))) {
+             if (!preempt_count() && !in_irq()) {

If you trying to detect atomic context here, than this doesn't work. E.g. you can't know about held spinlocks in non-preemptible kernel.
And I'm not sure why need this. You know gfp flags here, so allocation in atomic context shouldn't be problem.

We don't have gfp flags for kfree.
I wonder how CONFIG_DEBUG_ATOMIC_SLEEP handles this. Maybe it has the answer.

It hasn't. It doesn't guarantee that atomic context always will be detected.

Quoted 2 lines

Alternatively, we can always assume that we are in atomic context in kfree.

Or do this allocation in separate context, put in work queue.

Quoted 12 lines

Quoted 11 lines

lib/Makefile

Quoted 5 lines

lib/Makefile +3 -0

+                     alloc_flags &= (__GFP_RECLAIM | __GFP_IO | __GFP_FS |
+                             __GFP_NOWARN | __GFP_NORETRY |
+                             __GFP_NOMEMALLOC | __GFP_DIRECT_RECLAIM);

I think blacklist approach would be better here.

Quoted 1 line

+ page = alloc_pages(alloc_flags, STACK_ALLOC_ORDER);

STACK_ALLOC_ORDER = 4 - that's a lot. Do you really need that much?

Quoted 9 lines

Part of the issue the atomic context above. When we can't allocate memory we still want to save the stack trace. When we have less than STACK_ALLOC_ORDER memory, we try to preallocate another STACK_ALLOC_ORDER in advance. So in the worst case, we have STACK_ALLOC_ORDER memory and that should be enough to handle all kmalloc/kfree in the atomic context. 1 page does not look enough. I think Alex did some measuring of the failure race (when we are out of memory and can't allocate more).

A lot of 4-order pages will lead to high fragmentation. You don't need physically contiguous memory here, so try to use vmalloc(). It is slower, but fragmentation won't be problem.

And one more thing. Take a look at mempool, because it's generally used to solve the problem you have here (guaranteed allocation in atomic context).

Quoted 38 lines · Dmitry Vyukov

Quoted 14 lines

Quoted 14 lines

lib/Makefile obj-$(CONFIG_SG_SPLIT) += sg_split.o · line 171

Quoted 11 lines

lib/Makefile +2 -0

index a7c26a4..10a4ae3 100644
@@ -167,6 +167,13 @@ obj-$(CONFIG_SG_SPLIT) += sg_split.o
 obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
 obj-$(CONFIG_IRQ_POLL) += irq_poll.o
 
+ifeq ($(CONFIG_KASAN),y)
+ifeq ($(CONFIG_SLAB),y)

Just try to imagine that another subsystem wants to use stackdepot. How this gonna look like?

Quoted 5 lines

Quoted 5 lines

We have Kconfig to describe dependencies. So, this should be under CONFIG_STACKDEPOT. So any user of this feature can just do 'select STACKDEPOT' in Kconfig.

Quoted 2 lines

lib/Makefile +2 -0

+     obj-y   += stackdepot.o
+     KASAN_SANITIZE_slub.o := n

_stackdepot.o

Quoted 12 lines

Quoted 9 lines

lib/Makefile obj-$(CONFIG_SG_SPLIT) += sg_split.o · line 179

Quoted 7 lines

lib/Makefile +6 -0

+
+     stack->hash = hash;
+     stack->size = size;
+     stack->handle.slabindex = depot_index;
+     stack->handle.offset = depot_offset >> STACK_ALLOC_ALIGN;
+     __memcpy(stack->entries, entries, size * sizeof(unsigned long));

s/__memcpy/memcpy/

memcpy should be instrumented by asan/tsan, and we would like to avoid that instrumentation here.

Quoted 3 lines

KASAN_SANITIZE_* := n already takes care about this.
__memcpy() is a special thing solely for kasan internals and some assembly code. And it's not available generally.

As far as I can see, KASAN_SANITIZE_*:=n does not guarantee it. It just removes KASAN flags from GCC command line, it does not necessarily replace memcpy() calls with some kind of a non-instrumented memcpy().

2016-03-04 17:52 GMT+03:00 Alexander Potapenko <[email protected]>:

Quoted 39 lines · Andrey Ryabinin

Quoted 38 lines · Dmitry Vyukov

Quoted 14 lines

Quoted 14 lines

lib/Makefile obj-$(CONFIG_SG_SPLIT) += sg_split.o · line 171

Quoted 11 lines

lib/Makefile +2 -0

index a7c26a4..10a4ae3 100644
@@ -167,6 +167,13 @@ obj-$(CONFIG_SG_SPLIT) += sg_split.o
 obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
 obj-$(CONFIG_IRQ_POLL) += irq_poll.o
 
+ifeq ($(CONFIG_KASAN),y)
+ifeq ($(CONFIG_SLAB),y)

Just try to imagine that another subsystem wants to use stackdepot. How this gonna look like?

Quoted 5 lines

Quoted 5 lines

We have Kconfig to describe dependencies. So, this should be under CONFIG_STACKDEPOT. So any user of this feature can just do 'select STACKDEPOT' in Kconfig.

Quoted 2 lines

lib/Makefile +2 -0

+     obj-y   += stackdepot.o
+     KASAN_SANITIZE_slub.o := n

_stackdepot.o

Quoted 12 lines

Quoted 9 lines

lib/Makefile obj-$(CONFIG_SG_SPLIT) += sg_split.o · line 179

Quoted 7 lines

lib/Makefile +6 -0

+
+     stack->hash = hash;
+     stack->size = size;
+     stack->handle.slabindex = depot_index;
+     stack->handle.offset = depot_offset >> STACK_ALLOC_ALIGN;
+     __memcpy(stack->entries, entries, size * sizeof(unsigned long));

s/__memcpy/memcpy/

memcpy should be instrumented by asan/tsan, and we would like to avoid that instrumentation here.

Quoted 8 lines

Quoted 3 lines

KASAN_SANITIZE_* := n already takes care about this.
__memcpy() is a special thing solely for kasan internals and some assembly code. And it's not available generally.

As far as I can see, KASAN_SANITIZE_*:=n does not guarantee it. It just removes KASAN flags from GCC command line, it does not necessarily replace memcpy() calls with some kind of a non-instrumented memcpy().

With removed kasan cflags '__SANITIZE_ADDRESS__' is not defined, hence enable the following defines from arch/x86/include/asm/string_64.h:

#if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)

/*
* For files that not instrumented (e.g. mm/slub.c) we
* should use not instrumented version of mem* functions. */

#undef memcpy
#define memcpy(dst, src, len) __memcpy(dst, src, len) #define memmove(dst, src, len) __memmove(dst, src, len) #define memset(s, c, n) __memset(s, c, n) #endif

Quoted 33 lines

2016-03-04 17:52 GMT+03:00 Alexander Potapenko <[email protected]>:

Quoted 13 lines · Andrey Ryabinin

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+
+     stack->hash = hash;
+     stack->size = size;
+     stack->handle.slabindex = depot_index;
+     stack->handle.offset = depot_offset >> STACK_ALLOC_ALIGN;
+     __memcpy(stack->entries, entries, size * sizeof(unsigned long));

s/__memcpy/memcpy/

memcpy should be instrumented by asan/tsan, and we would like to avoid that instrumentation here.

Quoted 8 lines

Quoted 3 lines

KASAN_SANITIZE_* := n already takes care about this.
__memcpy() is a special thing solely for kasan internals and some assembly code. And it's not available generally.

As far as I can see, KASAN_SANITIZE_*:=n does not guarantee it. It just removes KASAN flags from GCC command line, it does not necessarily replace memcpy() calls with some kind of a non-instrumented memcpy().

With removed kasan cflags '__SANITIZE_ADDRESS__' is not defined, hence enable the following defines from arch/x86/include/asm/string_64.h:

#if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)

/*
* For files that not instrumented (e.g. mm/slub.c) we
* should use not instrumented version of mem* functions. */

Quoted 5 lines

#undef memcpy
#define memcpy(dst, src, len) __memcpy(dst, src, len) #define memmove(dst, src, len) __memmove(dst, src, len) #define memset(s, c, n) __memset(s, c, n) #endif

Nice!
What do you think about providing stub .c files to decouple the shared code used by KASAN runtime from the rest of kernel?

(This is a completely different story though and can be done separately).

--
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

Quoted 13 lines · Dmitry Vyukov

Quoted 13 lines

lib/Makefile obj-$(CONFIG_SG_SPLIT) += sg_split.o · line 171

Quoted 10 lines

lib/Makefile +2 -0

index a7c26a4..10a4ae3 100644
@@ -167,6 +167,13 @@ obj-$(CONFIG_SG_SPLIT) += sg_split.o
 obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
 obj-$(CONFIG_IRQ_POLL) += irq_poll.o
 
+ifeq ($(CONFIG_KASAN),y)
+ifeq ($(CONFIG_SLAB),y)

Just try to imagine that another subsystem wants to use stackdepot. How this gonna look like?

Quoted 5 lines

Quoted 5 lines

We have Kconfig to describe dependencies. So, this should be under CONFIG_STACKDEPOT. So any user of this feature can just do 'select STACKDEPOT' in Kconfig.

Quoted 2 lines

lib/Makefile +2 -0

+     obj-y   += stackdepot.o
+     KASAN_SANITIZE_slub.o := n

_stackdepot.o

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

lib/Makefile +6 -0

+
+     stack->hash = hash;
+     stack->size = size;
+     stack->handle.slabindex = depot_index;
+     stack->handle.offset = depot_offset >> STACK_ALLOC_ALIGN;
+     __memcpy(stack->entries, entries, size * sizeof(unsigned long));

Quoted 4 lines

Quoted 1 line

s/__memcpy/memcpy/

memcpy should be instrumented by asan/tsan, and we would like to avoid that instrumentation here.

KASAN_SANITIZE_* := n already takes care about this.
__memcpy() is a special thing solely for kasan internals and some assembly code. And it's not available generally.

Quoted 10 lines

Quoted 6 lines

lib/Makefile obj-$(CONFIG_SG_SPLIT) += sg_split.o

Quoted 2 lines

lib/Makefile +2 -0

+     if (unlikely(!smp_load_acquire(&next_slab_inited))) {
+             if (!preempt_count() && !in_irq()) {

If you trying to detect atomic context here, than this doesn't work. E.g. you can't know about held spinlocks in non-preemptible kernel.
And I'm not sure why need this. You know gfp flags here, so allocation in atomic context shouldn't be problem.

We don't have gfp flags for kfree.
I wonder how CONFIG_DEBUG_ATOMIC_SLEEP handles this. Maybe it has the answer.

It hasn't. It doesn't guarantee that atomic context always will be detected.

Quoted 2 lines

Alternatively, we can always assume that we are in atomic context in kfree.

Or do this allocation in separate context, put in work queue.

Quoted 12 lines

Quoted 11 lines

lib/Makefile

Quoted 5 lines

lib/Makefile +3 -0

+                     alloc_flags &= (__GFP_RECLAIM | __GFP_IO | __GFP_FS |
+                             __GFP_NOWARN | __GFP_NORETRY |
+                             __GFP_NOMEMALLOC | __GFP_DIRECT_RECLAIM);

I think blacklist approach would be better here.

Quoted 1 line

+ page = alloc_pages(alloc_flags, STACK_ALLOC_ORDER);

STACK_ALLOC_ORDER = 4 - that's a lot. Do you really need that much?

Quoted 9 lines

Part of the issue the atomic context above. When we can't allocate memory we still want to save the stack trace. When we have less than STACK_ALLOC_ORDER memory, we try to preallocate another STACK_ALLOC_ORDER in advance. So in the worst case, we have STACK_ALLOC_ORDER memory and that should be enough to handle all kmalloc/kfree in the atomic context. 1 page does not look enough. I think Alex did some measuring of the failure race (when we are out of memory and can't allocate more).

A lot of 4-order pages will lead to high fragmentation. You don't need physically contiguous memory here, so try to use vmalloc(). It is slower, but fragmentation won't be problem.

And one more thing. Take a look at mempool, because it's generally used to solve the problem you have here (guaranteed allocation in atomic context).

Quoted 15 lines

Quoted 14 lines

Quoted 4 lines

Quoted 2 lines

+ page = alloc_pages(alloc_flags, STACK_ALLOC_ORDER);

STACK_ALLOC_ORDER = 4 - that's a lot. Do you really need that much?

Part of the issue the atomic context above. When we can't allocate memory we still want to save the stack trace. When we have less than STACK_ALLOC_ORDER memory, we try to preallocate another STACK_ALLOC_ORDER in advance. So in the worst case, we have STACK_ALLOC_ORDER memory and that should be enough to handle all kmalloc/kfree in the atomic context. 1 page does not look enough. I think Alex did some measuring of the failure race (when we are out of memory and can't allocate more).

Quoted 2 lines

A lot of 4-order pages will lead to high fragmentation. You don't need physically contiguous memory here, so try to use vmalloc(). It is slower, but fragmentation won't be problem.

I've tried using vmalloc(), but turned out it's calling KASAN hooks again. Dealing with reentrancy in this case sounds like an overkill.

Given that we only require 9 Mb most of the time, is allocating physical pages still a problem?

Quoted 2 lines

And one more thing. Take a look at mempool, because it's generally used to solve the problem you have here (guaranteed allocation in atomic context).

As far as I understood the docs, mempools have a drawback of allocating too much memory which won't be available for any other use.

O'Reily's "Linux Device Drivers" even suggests not using mempools in any case when it's easier to deal with allocation failures (that advice is for device drivers, not sure if that stands for other subsystems though).

--
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

2016-03-08 14:42 GMT+03:00 Alexander Potapenko <[email protected]>:

Quoted 16 lines · Andrey Ryabinin

Quoted 15 lines

Quoted 14 lines

Quoted 4 lines

Quoted 2 lines

+ page = alloc_pages(alloc_flags, STACK_ALLOC_ORDER);

STACK_ALLOC_ORDER = 4 - that's a lot. Do you really need that much?

Part of the issue the atomic context above. When we can't allocate memory we still want to save the stack trace. When we have less than STACK_ALLOC_ORDER memory, we try to preallocate another STACK_ALLOC_ORDER in advance. So in the worst case, we have STACK_ALLOC_ORDER memory and that should be enough to handle all kmalloc/kfree in the atomic context. 1 page does not look enough. I think Alex did some measuring of the failure race (when we are out of memory and can't allocate more).

Quoted 4 lines

Quoted 2 lines

A lot of 4-order pages will lead to high fragmentation. You don't need physically contiguous memory here, so try to use vmalloc(). It is slower, but fragmentation won't be problem.

I've tried using vmalloc(), but turned out it's calling KASAN hooks again. Dealing with reentrancy in this case sounds like an overkill.

We'll have to deal with recursion eventually. Using stackdepot for page owner will cause recursion.

Quoted 3 lines

Given that we only require 9 Mb most of the time, is allocating physical pages still a problem?

This is not about size, this about fragmentation. vmalloc allows to utilize available low-order pages,
hence reduce the fragmentation.

Quoted 4 lines

Quoted 2 lines

And one more thing. Take a look at mempool, because it's generally used to solve the problem you have here (guaranteed allocation in atomic context).

As far as I understood the docs, mempools have a drawback of allocating too much memory which won't be available for any other use.

As far as I understood your code, it has a drawback of allocating too much memory which won't be available for any other use ;)

However, now I think that mempool doesn't fit here. We never free memory => never return it to pool.
And this will cause 5sec delays between allocation retries in mempool_alloc().

Quoted 17 lines

O'Reily's "Linux Device Drivers" even suggests not using mempools in any case when it's easier to deal with allocation failures (that advice is for device drivers, not sure if that stands for other subsystems though).

--
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

Quoted 25 lines · Andrey Ryabinin

2016-03-08 14:42 GMT+03:00 Alexander Potapenko <[email protected]>:

Quoted 16 lines · Andrey Ryabinin

Quoted 15 lines

Quoted 14 lines

Quoted 4 lines

Quoted 2 lines

+ page = alloc_pages(alloc_flags, STACK_ALLOC_ORDER);

STACK_ALLOC_ORDER = 4 - that's a lot. Do you really need that much?

Part of the issue the atomic context above. When we can't allocate memory we still want to save the stack trace. When we have less than STACK_ALLOC_ORDER memory, we try to preallocate another STACK_ALLOC_ORDER in advance. So in the worst case, we have STACK_ALLOC_ORDER memory and that should be enough to handle all kmalloc/kfree in the atomic context. 1 page does not look enough. I think Alex did some measuring of the failure race (when we are out of memory and can't allocate more).

Quoted 4 lines

Quoted 2 lines

A lot of 4-order pages will lead to high fragmentation. You don't need physically contiguous memory here, so try to use vmalloc(). It is slower, but fragmentation won't be problem.

I've tried using vmalloc(), but turned out it's calling KASAN hooks again. Dealing with reentrancy in this case sounds like an overkill.

We'll have to deal with recursion eventually. Using stackdepot for page owner will cause recursion.

Quoted 7 lines

Quoted 3 lines

Given that we only require 9 Mb most of the time, is allocating physical pages still a problem?

This is not about size, this about fragmentation. vmalloc allows to utilize available low-order pages,
hence reduce the fragmentation.

I've attempted to add __vmalloc(STACK_ALLOC_SIZE, alloc_flags, PAGE_KERNEL) (also tried vmalloc(STACK_ALLOC_SIZE)) instead of page_alloc() and am now getting a crash in kmem_cache_alloc_node_trace() in mm/slab.c, because it doesn't allow the kmem_cache pointer to be NULL (it's dereferenced when calling trace_kmalloc_node()).

Steven, do you know if this because of my code violating some contract (e.g. I'm calling vmalloc() too early, when kmalloc_caches[] haven't been initialized),

or is this a bug in kmem_cache_alloc_node_trace()
itself?

Quoted 40 lines · Alexander Potapenko

Quoted 25 lines · Andrey Ryabinin

2016-03-08 14:42 GMT+03:00 Alexander Potapenko <[email protected]>:

Quoted 16 lines · Andrey Ryabinin

Quoted 15 lines

Quoted 14 lines

Quoted 4 lines

Quoted 2 lines

+ page = alloc_pages(alloc_flags, STACK_ALLOC_ORDER);

STACK_ALLOC_ORDER = 4 - that's a lot. Do you really need that much?

Part of the issue the atomic context above. When we can't allocate memory we still want to save the stack trace. When we have less than STACK_ALLOC_ORDER memory, we try to preallocate another STACK_ALLOC_ORDER in advance. So in the worst case, we have STACK_ALLOC_ORDER memory and that should be enough to handle all kmalloc/kfree in the atomic context. 1 page does not look enough. I think Alex did some measuring of the failure race (when we are out of memory and can't allocate more).

Quoted 4 lines

Quoted 2 lines

A lot of 4-order pages will lead to high fragmentation. You don't need physically contiguous memory here, so try to use vmalloc(). It is slower, but fragmentation won't be problem.

I've tried using vmalloc(), but turned out it's calling KASAN hooks again. Dealing with reentrancy in this case sounds like an overkill.

We'll have to deal with recursion eventually. Using stackdepot for page owner will cause recursion.

Quoted 7 lines

Quoted 3 lines

Given that we only require 9 Mb most of the time, is allocating physical pages still a problem?

This is not about size, this about fragmentation. vmalloc allows to utilize available low-order pages,
hence reduce the fragmentation.

I've attempted to add __vmalloc(STACK_ALLOC_SIZE, alloc_flags, PAGE_KERNEL) (also tried vmalloc(STACK_ALLOC_SIZE)) instead of page_alloc() and am now getting a crash in kmem_cache_alloc_node_trace() in mm/slab.c, because it doesn't allow the kmem_cache pointer to be NULL (it's dereferenced when calling trace_kmalloc_node()).

Quoted 3 lines

Steven, do you know if this because of my code violating some contract (e.g. I'm calling vmalloc() too early, when kmalloc_caches[] haven't been initialized),

Probably. kmem_cache_init() goes before vmalloc_init().

Quoted 4 lines

or is this a bug in kmem_cache_alloc_node_trace()
itself?