Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1344455 > unrolled thread
| Started by | Alexander Potapenko <glider@google.com> |
|---|---|
| First post | 2016-02-26 18:00 +0100 |
| Last post | 2016-02-26 23:30 +0100 |
| Articles | 9 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH v4 0/7] SLAB support for KASAN Alexander Potapenko <glider@google.com> - 2016-02-26 18:00 +0100
[PATCH v4 2/7] mm, kasan: SLAB support Alexander Potapenko <glider@google.com> - 2016-02-26 18:00 +0100
Re: [PATCH v4 2/7] mm, kasan: SLAB support Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2016-02-29 16:20 +0100
Re: [PATCH v4 2/7] mm, kasan: SLAB support Alexander Potapenko <glider@google.com> - 2016-02-29 19:30 +0100
Re: [PATCH v4 2/7] mm, kasan: SLAB support Alexander Potapenko <glider@google.com> - 2016-02-29 19:40 +0100
Re: [PATCH v4 2/7] mm, kasan: SLAB support Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2016-03-01 15:40 +0100
[PATCH v4 4/7] arch, ftrace: For KASAN put hard/soft IRQ entries into separate sections Alexander Potapenko <glider@google.com> - 2016-02-26 18:00 +0100
Re: [PATCH v4 4/7] arch, ftrace: For KASAN put hard/soft IRQ entries into separate sections Steven Rostedt <rostedt@goodmis.org> - 2016-03-02 18:50 +0100
Re: [PATCH v4 0/7] SLAB support for KASAN Andrew Morton <akpm@linux-foundation.org> - 2016-02-26 23:30 +0100
| From | Alexander Potapenko <glider@google.com> |
|---|---|
| Date | 2016-02-26 18:00 +0100 |
| Subject | [PATCH v4 0/7] SLAB support for KASAN |
| Message-ID | <r6tKx-67J-7@gated-at.bofh.it> |
This patch set implements SLAB support for KASAN
Unlike SLUB, SLAB doesn't store allocation/deallocation stacks for heap
objects, therefore we reimplement this feature in mm/kasan/stackdepot.c.
The intention is to ultimately switch SLUB to use this implementation as
well, which will save a lot of memory (right now SLUB bloats each object
by 256 bytes to store the allocation/deallocation stacks).
Also neither SLUB nor SLAB delay the reuse of freed memory chunks, which
is necessary for better detection of use-after-free errors. We introduce
memory quarantine (mm/kasan/quarantine.c), which allows delayed reuse of
deallocated memory.
Alexander Potapenko (7):
kasan: Modify kmalloc_large_oob_right(), add
kmalloc_pagealloc_oob_right()
mm, kasan: SLAB support
mm, kasan: Added GFP flags to KASAN API
arch, ftrace: For KASAN put hard/soft IRQ entries into separate
sections
mm, kasan: Stackdepot implementation. Enable stackdepot for SLAB
kasan: Test fix: Warn if the UAF could not be detected in kmalloc_uaf2
mm: kasan: Initial memory quarantine implementation
---
v2: - merged two patches that touched kmalloc_large_oob_right
- moved stackdepot implementation to lib/
- moved IRQ definitions to include/linux/interrupt.h
v3: - minor description changes
- store deallocation info in the "mm, kasan: SLAB support" patch
v4: - fix kbuild error reports
---
Documentation/kasan.txt | 5 +-
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/Makefile | 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 +++
include/linux/kasan.h | 63 ++++++--
include/linux/slab.h | 10 +-
include/linux/slab_def.h | 14 ++
include/linux/slub_def.h | 11 ++
include/linux/stackdepot.h | 32 ++++
kernel/softirq.c | 2 +-
kernel/trace/trace_functions_graph.c | 1 +
lib/Kconfig.kasan | 4 +-
lib/Makefile | 7 +
lib/stackdepot.c | 274 +++++++++++++++++++++++++++++++
lib/test_kasan.c | 59 ++++++-
mm/Makefile | 1 +
mm/kasan/Makefile | 4 +
mm/kasan/kasan.c | 221 +++++++++++++++++++++++--
mm/kasan/kasan.h | 45 ++++++
mm/kasan/quarantine.c | 306 +++++++++++++++++++++++++++++++++++
mm/kasan/report.c | 71 ++++++--
mm/mempool.c | 23 +--
mm/page_alloc.c | 2 +-
mm/slab.c | 58 ++++++-
mm/slab.h | 2 +
mm/slab_common.c | 8 +-
mm/slub.c | 21 +--
44 files changed, 1214 insertions(+), 90 deletions(-)
create mode 100644 include/linux/stackdepot.h
create mode 100644 lib/stackdepot.c
create mode 100644 mm/kasan/quarantine.c
--
2.7.0.rc3.207.g0ac5344
[toc] | [next] | [standalone]
| From | Alexander Potapenko <glider@google.com> |
|---|---|
| Date | 2016-02-26 18:00 +0100 |
| Subject | [PATCH v4 2/7] mm, kasan: SLAB support |
| Message-ID | <r6tUd-6be-5@gated-at.bofh.it> |
| In reply to | #1344455 |
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 <glider@google.com>
---
v3: - minor description changes
- store deallocation info in kasan_slab_free()
v4: - fix kbuild compile-time warnings in print_track()
---
Documentation/kasan.txt | 5 +--
include/linux/kasan.h | 12 ++++++
include/linux/slab.h | 6 +++
include/linux/slab_def.h | 14 +++++++
include/linux/slub_def.h | 11 +++++
lib/Kconfig.kasan | 4 +-
mm/Makefile | 1 +
mm/kasan/kasan.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++
mm/kasan/kasan.h | 34 ++++++++++++++++
mm/kasan/report.c | 61 +++++++++++++++++++++++-----
mm/slab.c | 46 ++++++++++++++++++---
mm/slab_common.c | 2 +-
12 files changed, 277 insertions(+), 21 deletions(-)
diff --git a/Documentation/kasan.txt b/Documentation/kasan.txt
index aa1e0c9..7dd95b3 100644
--- a/Documentation/kasan.txt
+++ b/Documentation/kasan.txt
@@ -12,8 +12,7 @@ KASAN uses compile-time instrumentation for checking every memory access,
therefore you will need a GCC version 4.9.2 or later. GCC 5.0 or later is
required for detection of out-of-bounds accesses to stack or global variables.
-Currently KASAN is supported only for x86_64 architecture and requires the
-kernel to be built with the SLUB allocator.
+Currently KASAN is supported only for x86_64 architecture.
1. Usage
========
@@ -27,7 +26,7 @@ inline are compiler instrumentation types. The former produces smaller binary
the latter is 1.1 - 2 times faster. Inline instrumentation requires a GCC
version 5.0 or later.
-Currently KASAN works only with the SLUB memory allocator.
+KASAN works with both SLUB and SLAB memory allocators.
For better bug detection and nicer reporting, enable CONFIG_STACKTRACE.
To disable instrumentation for specific files or directories, add a line
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 4b9f85c..4405a35 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -46,6 +46,9 @@ void kasan_unpoison_shadow(const void *address, size_t size);
void kasan_alloc_pages(struct page *page, unsigned int order);
void kasan_free_pages(struct page *page, unsigned int order);
+void kasan_cache_create(struct kmem_cache *cache, size_t *size,
+ unsigned long *flags);
+
void kasan_poison_slab(struct page *page);
void kasan_unpoison_object_data(struct kmem_cache *cache, void *object);
void kasan_poison_object_data(struct kmem_cache *cache, void *object);
@@ -59,6 +62,11 @@ void kasan_krealloc(const void *object, size_t new_size);
void kasan_slab_alloc(struct kmem_cache *s, void *object);
void kasan_slab_free(struct kmem_cache *s, void *object);
+struct kasan_cache {
+ int alloc_meta_offset;
+ int free_meta_offset;
+};
+
int kasan_module_alloc(void *addr, size_t size);
void kasan_free_shadow(const struct vm_struct *vm);
@@ -72,6 +80,10 @@ static inline void kasan_disable_current(void) {}
static inline void kasan_alloc_pages(struct page *page, unsigned int order) {}
static inline void kasan_free_pages(struct page *page, unsigned int order) {}
+static inline void kasan_cache_create(struct kmem_cache *cache,
+ size_t *size,
+ unsigned long *flags) {}
+
static inline void kasan_poison_slab(struct page *page) {}
static inline void kasan_unpoison_object_data(struct kmem_cache *cache,
void *object) {}
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 3627d5c..840e652 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -92,6 +92,12 @@
# define SLAB_ACCOUNT 0x00000000UL
#endif
+#ifdef CONFIG_KASAN
+#define SLAB_KASAN 0x08000000UL
+#else
+#define SLAB_KASAN 0x00000000UL
+#endif
+
/* The following flags affect the page allocator grouping pages by mobility */
#define SLAB_RECLAIM_ACCOUNT 0x00020000UL /* Objects are reclaimable */
#define SLAB_TEMPORARY SLAB_RECLAIM_ACCOUNT /* Objects are short-lived */
diff --git a/include/linux/slab_def.h b/include/linux/slab_def.h
index cf139d3..f57232c 100644
--- a/include/linux/slab_def.h
+++ b/include/linux/slab_def.h
@@ -73,8 +73,22 @@ struct kmem_cache {
#ifdef CONFIG_MEMCG
struct memcg_cache_params memcg_params;
#endif
+#ifdef CONFIG_KASAN
+ struct kasan_cache kasan_info;
+#endif
struct kmem_cache_node *node[MAX_NUMNODES];
};
+static inline void *nearest_obj(struct kmem_cache *cache, struct page *page,
+ void *x) {
+ void *object = x - (x - page->s_mem) % cache->size;
+ void *last_object = page->s_mem + (cache->num - 1) * cache->size;
+
+ if (unlikely(object > last_object))
+ return last_object;
+ else
+ return object;
+}
+
#endif /* _LINUX_SLAB_DEF_H */
diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index b7e57927..c6970a0 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -129,4 +129,15 @@ static inline void *virt_to_obj(struct kmem_cache *s,
void object_err(struct kmem_cache *s, struct page *page,
u8 *object, char *reason);
+static inline void *nearest_obj(struct kmem_cache *cache, struct page *page,
+ void *x) {
+ void *object = x - (x - page_address(page)) % cache->size;
+ void *last_object = page_address(page) +
+ (page->objects - 1) * cache->size;
+ if (unlikely(object > last_object))
+ return last_object;
+ else
+ return object;
+}
+
#endif /* _LINUX_SLUB_DEF_H */
diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index 0fee5ac..0e4d2b3 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -5,7 +5,7 @@ if HAVE_ARCH_KASAN
config KASAN
bool "KASan: runtime memory debugger"
- depends on SLUB_DEBUG
+ depends on SLUB_DEBUG || (SLAB && !DEBUG_SLAB)
select CONSTRUCTORS
help
Enables kernel address sanitizer - runtime memory debugger,
@@ -16,6 +16,8 @@ config KASAN
This feature consumes about 1/8 of available memory and brings about
~x3 performance slowdown.
For better error detection enable CONFIG_STACKTRACE.
+ Currently CONFIG_KASAN doesn't work with CONFIG_DEBUG_SLAB
+ (the resulting kernel does not boot).
choice
prompt "Instrumentation type"
diff --git a/mm/Makefile b/mm/Makefile
index 2ed4319..d675b37 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -3,6 +3,7 @@
#
KASAN_SANITIZE_slab_common.o := n
+KASAN_SANITIZE_slab.o := n
KASAN_SANITIZE_slub.o := n
mmu-y := nommu.o
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index bc0a8d8..d26ffb4 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -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),
@@ -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);
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 4f6c62e..7b9e4ab9 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -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,
+ const void *object);
+struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
+ const void *object);
+
+
static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
{
return (void *)(((unsigned long)shadow_addr - KASAN_SHADOW_OFFSET)
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 12f222d..2c1407f 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -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);
+}
+
+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) {
+ 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 */
-
+ object = nearest_obj(cache, page,
+ (void *)info->access_addr);
+#ifdef CONFIG_SLAB
+ print_object(cache, object);
+#else
object_err(cache, page, object,
- "kasan: bad access detected");
+ "kasan: bad access detected");
+#endif
return;
}
dump_page(page, "kasan: bad access detected");
@@ -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
dump_stack();
+#endif
}
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
+ dump_stack();
+#endif
print_address_description(info);
print_shadow_for_address(info->first_bad_addr);
}
diff --git a/mm/slab.c b/mm/slab.c
index 621fbcb..805b39b 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2196,6 +2196,7 @@ __kmem_cache_create (struct kmem_cache *cachep, unsigned long flags)
#endif
#endif
+ kasan_cache_create(cachep, &size, &flags);
/*
* Determine if the slab management is 'on' or 'off' slab.
* (bootstrapping cannot cope with offslab caches so don't do
@@ -2503,8 +2504,13 @@ static void cache_init_objs(struct kmem_cache *cachep,
* cache which they are a constructor for. Otherwise, deadlock.
* They must also be threaded.
*/
- if (cachep->ctor && !(cachep->flags & SLAB_POISON))
+ if (cachep->ctor && !(cachep->flags & SLAB_POISON)) {
+ kasan_unpoison_object_data(cachep,
+ objp + obj_offset(cachep));
cachep->ctor(objp + obj_offset(cachep));
+ kasan_poison_object_data(
+ cachep, objp + obj_offset(cachep));
+ }
if (cachep->flags & SLAB_RED_ZONE) {
if (*dbg_redzone2(cachep, objp) != RED_INACTIVE)
@@ -2519,8 +2525,11 @@ static void cache_init_objs(struct kmem_cache *cachep,
kernel_map_pages(virt_to_page(objp),
cachep->size / PAGE_SIZE, 0);
#else
- if (cachep->ctor)
+ if (cachep->ctor) {
+ kasan_unpoison_object_data(cachep, objp);
cachep->ctor(objp);
+ kasan_poison_object_data(cachep, objp);
+ }
#endif
set_obj_status(page, i, OBJECT_FREE);
set_free_obj(page, i, i);
@@ -2650,6 +2659,7 @@ static int cache_grow(struct kmem_cache *cachep,
slab_map_pages(cachep, page, freelist);
+ kasan_poison_slab(page);
cache_init_objs(cachep, page);
if (gfpflags_allow_blocking(local_flags))
@@ -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);
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(cachep, ret);
trace_kmem_cache_alloc(_RET_IP_, ret,
cachep->object_size, cachep->size, flags);
@@ -3432,6 +3447,8 @@ kmem_cache_alloc_trace(struct kmem_cache *cachep, gfp_t flags, size_t size)
ret = slab_alloc(cachep, flags, _RET_IP_);
+ if (ret)
+ kasan_kmalloc(cachep, ret, size);
trace_kmalloc(_RET_IP_, ret,
size, cachep->size, flags);
return ret;
@@ -3455,6 +3472,8 @@ void *kmem_cache_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid)
{
void *ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
+ if (ret)
+ kasan_slab_alloc(cachep, ret);
trace_kmem_cache_alloc_node(_RET_IP_, ret,
cachep->object_size, cachep->size,
flags, nodeid);
@@ -3473,6 +3492,8 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *cachep,
ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
+ if (ret)
+ kasan_kmalloc(cachep, ret, size);
trace_kmalloc_node(_RET_IP_, ret,
size, cachep->size,
flags, nodeid);
@@ -3485,11 +3506,16 @@ static __always_inline void *
__do_kmalloc_node(size_t size, gfp_t flags, int node, unsigned long caller)
{
struct kmem_cache *cachep;
+ void *ret;
cachep = kmalloc_slab(size, flags);
if (unlikely(ZERO_OR_NULL_PTR(cachep)))
return cachep;
- return kmem_cache_alloc_node_trace(cachep, flags, node, size);
+ ret = kmem_cache_alloc_node_trace(cachep, flags, node, size);
+ if (ret)
+ kasan_kmalloc(cachep, ret, size);
+
+ return ret;
}
void *__kmalloc_node(size_t size, gfp_t flags, int node)
@@ -3523,6 +3549,8 @@ static __always_inline void *__do_kmalloc(size_t size, gfp_t flags,
return cachep;
ret = slab_alloc(cachep, flags, caller);
+ if (ret)
+ kasan_kmalloc(cachep, ret, size);
trace_kmalloc(caller, ret,
size, cachep->size, flags);
@@ -4240,10 +4268,18 @@ module_init(slab_proc_init);
*/
size_t ksize(const void *objp)
{
+ size_t size;
+
BUG_ON(!objp);
if (unlikely(objp == ZERO_SIZE_PTR))
return 0;
- return virt_to_cache(objp)->object_size;
+ size = virt_to_cache(objp)->object_size;
+ /* We assume that ksize callers could use whole allocated area,
+ * so we need to unpoison this area.
+ */
+ kasan_krealloc(objp, size);
+
+ return size;
}
EXPORT_SYMBOL(ksize);
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 065b7bd..bf04ec7 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -35,7 +35,7 @@ struct kmem_cache *kmem_cache;
*/
#define SLAB_NEVER_MERGE (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER | \
SLAB_TRACE | SLAB_DESTROY_BY_RCU | SLAB_NOLEAKTRACE | \
- SLAB_FAILSLAB)
+ SLAB_FAILSLAB | SLAB_KASAN)
#define SLAB_MERGE_SAME (SLAB_RECLAIM_ACCOUNT | SLAB_CACHE_DMA | \
SLAB_NOTRACK | SLAB_ACCOUNT)
--
2.7.0.rc3.207.g0ac5344
[toc] | [prev] | [next] | [standalone]
| From | Andrey Ryabinin <ryabinin.a.a@gmail.com> |
|---|---|
| Date | 2016-02-29 16:20 +0100 |
| Subject | Re: [PATCH v4 2/7] mm, kasan: SLAB support |
| Message-ID | <r7xM6-4Jh-5@gated-at.bofh.it> |
| In reply to | #1344456 |
On 02/26/2016 07:48 PM, Alexander Potapenko wrote:
> 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 <glider@google.com>
> ---
> v3: - minor description changes
> - store deallocation info in kasan_slab_free()
>
> v4: - fix kbuild compile-time warnings in print_track()
> ---
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index bc0a8d8..d26ffb4 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -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),
> @@ -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);
>
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 4f6c62e..7b9e4ab9 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -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,
> + 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.
> static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
> {
> return (void *)(((unsigned long)shadow_addr - KASAN_SHADOW_OFFSET)
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 12f222d..2c1407f 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -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);
> +}
> +
> +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.
> + 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 */
> -
> + 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.
> object_err(cache, page, object,
> - "kasan: bad access detected");
> + "kasan: bad access detected");
> +#endif
> return;
> }
> dump_page(page, "kasan: bad access detected");
> @@ -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
???
> dump_stack();
> +#endif
> }
>
> 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?
> + dump_stack();
> +#endif
> print_address_description(info);
> print_shadow_for_address(info->first_bad_addr);
> }
> diff --git a/mm/slab.c b/mm/slab.c
> index 621fbcb..805b39b 100644
>
> if (gfpflags_allow_blocking(local_flags))
> @@ -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.
>
> 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.
> + kasan_slab_alloc(cachep, ret);
>
> trace_kmem_cache_alloc(_RET_IP_, ret,
> cachep->object_size, cachep->size, flags);
[toc] | [prev] | [next] | [standalone]
| From | Alexander Potapenko <glider@google.com> |
|---|---|
| Date | 2016-02-29 19:30 +0100 |
| Subject | Re: [PATCH v4 2/7] mm, kasan: SLAB support |
| Message-ID | <r7AJY-6Ab-29@gated-at.bofh.it> |
| In reply to | #1345897 |
On Mon, Feb 29, 2016 at 4:10 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
>
>
> On 02/26/2016 07:48 PM, Alexander Potapenko wrote:
>> 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 <glider@google.com>
>> ---
>> v3: - minor description changes
>> - store deallocation info in kasan_slab_free()
>>
>> v4: - fix kbuild compile-time warnings in print_track()
>> ---
>
>
>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
>> index bc0a8d8..d26ffb4 100644
>> --- a/mm/kasan/kasan.c
>> +++ b/mm/kasan/kasan.c
>> @@ -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),
>> @@ -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);
>>
>> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
>> index 4f6c62e..7b9e4ab9 100644
>> --- a/mm/kasan/kasan.h
>> +++ b/mm/kasan/kasan.h
>> @@ -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,
>> + 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.
>> static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
>> {
>> return (void *)(((unsigned long)shadow_addr - KASAN_SHADOW_OFFSET)
>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>> index 12f222d..2c1407f 100644
>> --- a/mm/kasan/report.c
>> +++ b/mm/kasan/report.c
>> @@ -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);
>> +}
>> +
>> +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.
>
>> + 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 */
>> -
>> + 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.
>> object_err(cache, page, object,
>> - "kasan: bad access detected");
>> + "kasan: bad access detected");
>> +#endif
>> return;
>> }
>> dump_page(page, "kasan: bad access detected");
>> @@ -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.
>
>> dump_stack();
>> +#endif
>> }
>>
>> 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).
>> + dump_stack();
>> +#endif
>> print_address_description(info);
>> print_shadow_for_address(info->first_bad_addr);
>> }
>> diff --git a/mm/slab.c b/mm/slab.c
>> index 621fbcb..805b39b 100644
>
>
>
>>
>> if (gfpflags_allow_blocking(local_flags))
>> @@ -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.
>>
>> 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.
>
>> + 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.
[toc] | [prev] | [next] | [standalone]
| From | Alexander Potapenko <glider@google.com> |
|---|---|
| Date | 2016-02-29 19:40 +0100 |
| Subject | Re: [PATCH v4 2/7] mm, kasan: SLAB support |
| Message-ID | <r7ATE-6F6-13@gated-at.bofh.it> |
| In reply to | #1346072 |
On Mon, Feb 29, 2016 at 7:28 PM, Alexander Potapenko <glider@google.com> wrote:
> On Mon, Feb 29, 2016 at 4:10 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
>>
>>
>> On 02/26/2016 07:48 PM, Alexander Potapenko wrote:
>>> 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 <glider@google.com>
>>> ---
>>> v3: - minor description changes
>>> - store deallocation info in kasan_slab_free()
>>>
>>> v4: - fix kbuild compile-time warnings in print_track()
>>> ---
>>
>>
>>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
>>> index bc0a8d8..d26ffb4 100644
>>> --- a/mm/kasan/kasan.c
>>> +++ b/mm/kasan/kasan.c
>>> @@ -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),
>>> @@ -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);
>>>
>>> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
>>> index 4f6c62e..7b9e4ab9 100644
>>> --- a/mm/kasan/kasan.h
>>> +++ b/mm/kasan/kasan.h
>>> @@ -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,
>>> + 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.
>
>
>>> static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
>>> {
>>> return (void *)(((unsigned long)shadow_addr - KASAN_SHADOW_OFFSET)
>>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>>> index 12f222d..2c1407f 100644
>>> --- a/mm/kasan/report.c
>>> +++ b/mm/kasan/report.c
>>> @@ -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);
>>> +}
>>> +
>>> +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.
Don't think it's a good idea to rely on the shadow values here, the
state is a different conception.
For example, shadow values don't distinguish between the freed memory
and memory in the quarantine.
In the userspace tool it's also possible for the user to manually
poison and unpoison parts of the memory (and I can imagine having the
same mechanism in KASAN), but this does not necessarily make them
change the state.
>>> + 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 */
>>> -
>>> + 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.
>
>>> object_err(cache, page, object,
>>> - "kasan: bad access detected");
>>> + "kasan: bad access detected");
>>> +#endif
>>> return;
>>> }
>>> dump_page(page, "kasan: bad access detected");
>>> @@ -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.
>>
>>> dump_stack();
>>> +#endif
>>> }
>>>
>>> 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).
>
>>> + dump_stack();
>>> +#endif
>>> print_address_description(info);
>>> print_shadow_for_address(info->first_bad_addr);
>>> }
>>> diff --git a/mm/slab.c b/mm/slab.c
>>> index 621fbcb..805b39b 100644
>>
>>
>>
>>>
>>> if (gfpflags_allow_blocking(local_flags))
>>> @@ -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.
>
>>>
>>> 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.
>>
>>> + 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.
--
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.
[toc] | [prev] | [next] | [standalone]
| From | Andrey Ryabinin <ryabinin.a.a@gmail.com> |
|---|---|
| Date | 2016-03-01 15:40 +0100 |
| Subject | Re: [PATCH v4 2/7] mm, kasan: SLAB support |
| Message-ID | <r7TCV-1Ub-7@gated-at.bofh.it> |
| In reply to | #1346072 |
On 02/29/2016 09:28 PM, Alexander Potapenko wrote:
>>> 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 */
>>> -
>>> + 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.
>
Avoid mess? You create one.
Although I don't understand that don't touch slub thing, but you can just
have object_err(cache, page, str) for slab without touching slub.
>>> object_err(cache, page, object,
>>> - "kasan: bad access detected");
>>> + "kasan: bad access detected");
>>> +#endif
>>> return;
>>> }
>>> dump_page(page, "kasan: bad access detected");
>>> @@ -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.
>>
>>> dump_stack();
>>> +#endif
>>> }
>>>
>>> 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).
>
That's all sounds fine, but this doesn't explain:
a) How this change related to this patch? (the answer is - it doesn't).
b) Why the output of non sl[a,u]b bugs depends on CONFIG_SL[A,U]B ?
So, in SLAB's print_objects() you can print stacks in whatever order you like.
That's it, don't change anything else here.
If you are not satisfied with current format output, change it, but in separate patch[es],
with reasoning described in changelog and without weird config dependencies.
>>> + dump_stack();
>>> +#endif
>>> print_address_description(info);
>>> print_shadow_for_address(info->first_bad_addr);
>>> }
>>> diff --git a/mm/slab.c b/mm/slab.c
>>> index 621fbcb..805b39b 100644
>>
[toc] | [prev] | [next] | [standalone]
| From | Alexander Potapenko <glider@google.com> |
|---|---|
| Date | 2016-02-26 18:00 +0100 |
| Subject | [PATCH v4 4/7] arch, ftrace: For KASAN put hard/soft IRQ entries into separate sections |
| Message-ID | <r6tUe-6be-23@gated-at.bofh.it> |
| In reply to | #1344455 |
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 <glider@google.com>
---
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(-)
diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
index 8b60fde..28b690fc 100644
--- a/arch/arm/kernel/vmlinux.lds.S
+++ b/arch/arm/kernel/vmlinux.lds.S
@@ -105,6 +105,7 @@ SECTIONS
*(.exception.text)
__exception_text_end = .;
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
TEXT_TEXT
SCHED_TEXT
LOCK_TEXT
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index e3928f5..b9242b7 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -102,6 +102,7 @@ SECTIONS
*(.exception.text)
__exception_text_end = .;
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
TEXT_TEXT
SCHED_TEXT
LOCK_TEXT
diff --git a/arch/blackfin/kernel/vmlinux.lds.S b/arch/blackfin/kernel/vmlinux.lds.S
index c9eec84..d920b95 100644
--- a/arch/blackfin/kernel/vmlinux.lds.S
+++ b/arch/blackfin/kernel/vmlinux.lds.S
@@ -35,6 +35,7 @@ SECTIONS
#endif
LOCK_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
KPROBES_TEXT
#ifdef CONFIG_ROMKERNEL
__sinittext = .;
diff --git a/arch/c6x/kernel/vmlinux.lds.S b/arch/c6x/kernel/vmlinux.lds.S
index 5a6e141..50bc10f 100644
--- a/arch/c6x/kernel/vmlinux.lds.S
+++ b/arch/c6x/kernel/vmlinux.lds.S
@@ -72,6 +72,7 @@ SECTIONS
SCHED_TEXT
LOCK_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
KPROBES_TEXT
*(.fixup)
*(.gnu.warning)
diff --git a/arch/metag/kernel/vmlinux.lds.S b/arch/metag/kernel/vmlinux.lds.S
index e12055e..150ace9 100644
--- a/arch/metag/kernel/vmlinux.lds.S
+++ b/arch/metag/kernel/vmlinux.lds.S
@@ -24,6 +24,7 @@ SECTIONS
LOCK_TEXT
KPROBES_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
*(.text.*)
*(.gnu.warning)
}
diff --git a/arch/microblaze/kernel/vmlinux.lds.S b/arch/microblaze/kernel/vmlinux.lds.S
index be9488d..0a47f04 100644
--- a/arch/microblaze/kernel/vmlinux.lds.S
+++ b/arch/microblaze/kernel/vmlinux.lds.S
@@ -36,6 +36,7 @@ SECTIONS {
LOCK_TEXT
KPROBES_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
. = ALIGN (4) ;
_etext = . ;
}
diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
index 0a93e83..54d653e 100644
--- a/arch/mips/kernel/vmlinux.lds.S
+++ b/arch/mips/kernel/vmlinux.lds.S
@@ -58,6 +58,7 @@ SECTIONS
LOCK_TEXT
KPROBES_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
*(.text.*)
*(.fixup)
*(.gnu.warning)
diff --git a/arch/nios2/kernel/vmlinux.lds.S b/arch/nios2/kernel/vmlinux.lds.S
index 326fab4..e23e895 100644
--- a/arch/nios2/kernel/vmlinux.lds.S
+++ b/arch/nios2/kernel/vmlinux.lds.S
@@ -39,6 +39,7 @@ SECTIONS
SCHED_TEXT
LOCK_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
KPROBES_TEXT
} =0
_etext = .;
diff --git a/arch/openrisc/kernel/vmlinux.lds.S b/arch/openrisc/kernel/vmlinux.lds.S
index 2d69a85..d936de4 100644
--- a/arch/openrisc/kernel/vmlinux.lds.S
+++ b/arch/openrisc/kernel/vmlinux.lds.S
@@ -50,6 +50,7 @@ SECTIONS
LOCK_TEXT
KPROBES_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
*(.fixup)
*(.text.__*)
_etext = .;
diff --git a/arch/parisc/kernel/vmlinux.lds.S b/arch/parisc/kernel/vmlinux.lds.S
index 308f290..f3ead0b 100644
--- a/arch/parisc/kernel/vmlinux.lds.S
+++ b/arch/parisc/kernel/vmlinux.lds.S
@@ -72,6 +72,7 @@ SECTIONS
LOCK_TEXT
KPROBES_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
*(.text.do_softirq)
*(.text.sys_exit)
*(.text.do_sigaltstack)
diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index d41fd0a..2dd91f7 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -55,6 +55,7 @@ SECTIONS
LOCK_TEXT
KPROBES_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
#ifdef CONFIG_PPC32
*(.got1)
diff --git a/arch/s390/kernel/vmlinux.lds.S b/arch/s390/kernel/vmlinux.lds.S
index 445657f..0f41a82 100644
--- a/arch/s390/kernel/vmlinux.lds.S
+++ b/arch/s390/kernel/vmlinux.lds.S
@@ -28,6 +28,7 @@ SECTIONS
LOCK_TEXT
KPROBES_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
*(.fixup)
*(.gnu.warning)
} :text = 0x0700
diff --git a/arch/sh/kernel/vmlinux.lds.S b/arch/sh/kernel/vmlinux.lds.S
index db88cbf..235a410 100644
--- a/arch/sh/kernel/vmlinux.lds.S
+++ b/arch/sh/kernel/vmlinux.lds.S
@@ -39,6 +39,7 @@ SECTIONS
LOCK_TEXT
KPROBES_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
*(.fixup)
*(.gnu.warning)
_etext = .; /* End of text section */
diff --git a/arch/sparc/kernel/vmlinux.lds.S b/arch/sparc/kernel/vmlinux.lds.S
index f1a2f68..aadd321 100644
--- a/arch/sparc/kernel/vmlinux.lds.S
+++ b/arch/sparc/kernel/vmlinux.lds.S
@@ -48,6 +48,7 @@ SECTIONS
LOCK_TEXT
KPROBES_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
*(.gnu.warning)
} = 0
_etext = .;
diff --git a/arch/tile/kernel/vmlinux.lds.S b/arch/tile/kernel/vmlinux.lds.S
index 0e059a0..378f5d8 100644
--- a/arch/tile/kernel/vmlinux.lds.S
+++ b/arch/tile/kernel/vmlinux.lds.S
@@ -45,6 +45,7 @@ SECTIONS
LOCK_TEXT
KPROBES_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
__fix_text_end = .; /* tile-cpack won't rearrange before this */
ALIGN_FUNCTION();
*(.hottext*)
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 74e4bf1..056a97a 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -102,6 +102,7 @@ SECTIONS
KPROBES_TEXT
ENTRY_TEXT
IRQENTRY_TEXT
+ SOFTIRQENTRY_TEXT
*(.fixup)
*(.gnu.warning)
/* End of text section */
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index c4bd0e2..b470421 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -456,7 +456,7 @@
*(.entry.text) \
VMLINUX_SYMBOL(__entry_text_end) = .;
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN)
#define IRQENTRY_TEXT \
ALIGN_FUNCTION(); \
VMLINUX_SYMBOL(__irqentry_text_start) = .; \
@@ -466,6 +466,16 @@
#define IRQENTRY_TEXT
#endif
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN)
+#define SOFTIRQENTRY_TEXT \
+ ALIGN_FUNCTION(); \
+ VMLINUX_SYMBOL(__softirqentry_text_start) = .; \
+ *(.softirqentry.text) \
+ VMLINUX_SYMBOL(__softirqentry_text_end) = .;
+#else
+#define SOFTIRQENTRY_TEXT
+#endif
+
/* Section used for early init (in .S files) */
#define HEAD_TEXT *(.head.text)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index c2b340e..4da848d 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -799,16 +799,6 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,
*/
#define __notrace_funcgraph notrace
-/*
- * We want to which function is an entrypoint of a hardirq.
- * That will help us to put a signal on output.
- */
-#define __irq_entry __attribute__((__section__(".irqentry.text")))
-
-/* Limits of hardirq entrypoints */
-extern char __irqentry_text_start[];
-extern char __irqentry_text_end[];
-
#define FTRACE_NOTRACE_DEPTH 65536
#define FTRACE_RETFUNC_DEPTH 50
#define FTRACE_RETSTACK_ALLOC_SIZE 32
@@ -845,7 +835,6 @@ static inline void unpause_graph_tracing(void)
#else /* !CONFIG_FUNCTION_GRAPH_TRACER */
#define __notrace_funcgraph
-#define __irq_entry
#define INIT_FTRACE_GRAPH
static inline void ftrace_graph_init_task(struct task_struct *t) { }
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 0e95fcc..1dcecaf 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -673,4 +673,24 @@ extern int early_irq_init(void);
extern int arch_probe_nr_irqs(void);
extern int arch_early_irq_init(void);
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN)
+/*
+ * We want to know which function is an entrypoint of a hardirq or a softirq.
+ */
+#define __irq_entry __attribute__((__section__(".irqentry.text")))
+#define __softirq_entry \
+ __attribute__((__section__(".softirqentry.text")))
+
+/* Limits of hardirq entrypoints */
+extern char __irqentry_text_start[];
+extern char __irqentry_text_end[];
+/* Limits of softirq entrypoints */
+extern char __softirqentry_text_start[];
+extern char __softirqentry_text_end[];
+
+#else
+#define __irq_entry
+#define __softirq_entry
+#endif
+
#endif
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 479e443..359be4f 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -227,7 +227,7 @@ static inline bool lockdep_softirq_start(void) { return false; }
static inline void lockdep_softirq_end(bool in_hardirq) { }
#endif
-asmlinkage __visible void __do_softirq(void)
+asmlinkage __visible void __softirq_entry __do_softirq(void)
{
unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
unsigned long old_flags = current->flags;
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index a663cbb..3e6f7d4 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -8,6 +8,7 @@
*/
#include <linux/uaccess.h>
#include <linux/ftrace.h>
+#include <linux/interrupt.h>
#include <linux/slab.h>
#include <linux/fs.h>
--
2.7.0.rc3.207.g0ac5344
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-03-02 18:50 +0100 |
| Subject | Re: [PATCH v4 4/7] arch, ftrace: For KASAN put hard/soft IRQ entries into separate sections |
| Message-ID | <r8j4l-2TR-9@gated-at.bofh.it> |
| In reply to | #1344462 |
On Fri, 26 Feb 2016 17:48:44 +0100 Alexander Potapenko <glider@google.com> wrote: > 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 <glider@google.com> Acked-by: Steven Rostedt <rostedt@goodmis.org> -- Steve > --- > 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(-) > >
[toc] | [prev] | [next] | [standalone]
| From | Andrew Morton <akpm@linux-foundation.org> |
|---|---|
| Date | 2016-02-26 23:30 +0100 |
| Message-ID | <r6z3B-1uq-49@gated-at.bofh.it> |
| In reply to | #1344455 |
On Fri, 26 Feb 2016 17:48:40 +0100 Alexander Potapenko <glider@google.com> wrote: > This patch set implements SLAB support for KASAN That's a lot of code and I'm not seeing much review activity from folks. There was one ack against an earlier version of [4/7] from Steven Rostedt but that ack wasn't maintained (bad!). I scanned over these and my plan was to queue them for -next testing and to await more review/test before proceeding further. But alas, there are significant collisions with pending slab patches (all in linux-next) so could you please take a look at those?
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web