On 02/19/2015 02:10 AM, Rusty Russell wrote:
> Andrey Ryabinin <a.ryabi...@samsung.com> writes:
>> Current approach in handling shadow memory for modules is broken.
>>
>> Shadow memory could be freed only after memory shadow corresponds
>> it is no longer used.
>> vfree() called from interrupt context could use memory its
>> freeing to store 'struct llist_node' in it:
>>
>> void vfree(const void *addr)
>> {
>> ...
>> if (unlikely(in_interrupt())) {
>> struct vfree_deferred *p = this_cpu_ptr(&vfree_deferred);
>> if (llist_add((struct llist_node *)addr, &p->list))
>> schedule_work(&p->wq);
>>
>> Latter this list node used in free_work() which actually frees memory.
>> Currently module_memfree() called in interrupt context will free
>> shadow before freeing module's memory which could provoke kernel
>> crash.
>> So shadow memory should be freed after module's memory.
>> However, such deallocation order could race with kasan_module_alloc()
>> in module_alloc().
>>
>> To fix this we could move kasan hooks into vmalloc code. This allows
>> us to allocate/free shadow memory in appropriate time and order.
>>
>> This hooks also might be helpful in future if we decide to track
>> other vmalloc'ed memory.
>
> This is not portable. Other archs don't use vmalloc, or don't use
> (or define) MODULES_VADDR. If you really want to hook here, you'd
> need a new flag (or maybe use PAGE_KERNEL_EXEC after an audit).
>
Well, instead of explicit (addr >= MODULES_VADDR && addr < MODULES_END)
I could hide this into arch-specific function:
'kasan_need_to_allocate_shadow(const void *addr)'
or make make all those functions weak and allow arch code to redefine them.
> Thus I think modifying the callers is the better choice.
>
I could suggest following (though, I still prefer 'modifying vmalloc' approach):
* In do_init_module(), instead of call_rcu(&freeinit->rcu, do_free_init);
use synchronyze_rcu() + module_memfree(). Of course this will be under
CONFIG_KASAN.
As you said there other module_memfree() users, so what if they will decide
to free memory in atomic context?
* And another option would be deferred kasan_module_free() in patch bellow.
This is mostly copy-paste of deferred vfree(), thus I don't like it.
---
arch/x86/mm/kasan_init_64.c | 1 +
include/linux/kasan.h | 1 +
kernel/module.c | 6 ++++--
mm/kasan/kasan.c | 42 +++++++++++++++++++++++++++++++++++++++++-
4 files changed, 47 insertions(+), 3 deletions(-)
diff --git a/arch/x86/mm/kasan_init_64.c b/arch/x86/mm/kasan_init_64.c
index 4860906..66d2dba 100644
--- a/arch/x86/mm/kasan_init_64.c
+++ b/arch/x86/mm/kasan_init_64.c
@@ -173,6 +173,7 @@ void __init kasan_init(void)
#ifdef CONFIG_KASAN_INLINE
register_die_notifier(&kasan_die_notifier);
#endif
+ kasan_modules_init();
memcpy(early_level4_pgt, init_level4_pgt, sizeof(early_level4_pgt));
load_cr3(early_level4_pgt);
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 72ba725..dba26f3 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -53,6 +53,7 @@ void kasan_slab_free(struct kmem_cache *s, void *object);
int kasan_module_alloc(void *addr, size_t size);
void kasan_module_free(void *addr);
+void kasan_modules_init(void);
#else /* CONFIG_KASAN */
diff --git a/kernel/module.c b/kernel/module.c
index 8426ad4..e3d1a45 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1813,8 +1813,10 @@ static void unset_module_init_ro_nx(struct module *mod)
{ }
void __weak module_memfree(void *module_region)
{
- vfree(module_region);
- kasan_module_free(module_region);
+ if (IS_ENABLED(CONFIG_KASAN))
+ kasan_module_free(module_region);
+ else
+ vfree(module_region);
}
void __weak module_arch_cleanup(struct module *mod)
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 78fee63..333241e 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -19,6 +19,7 @@
#include <linux/export.h>
#include <linux/init.h>
#include <linux/kernel.h>
+#include <linux/llist.h>
#include <linux/memblock.h>
#include <linux/memory.h>
#include <linux/mm.h>
@@ -29,6 +30,7 @@
#include <linux/stacktrace.h>
#include <linux/string.h>
#include <linux/types.h>
+#include <linux/vmalloc.h>
#include <linux/kasan.h>
#include "kasan.h"
@@ -417,9 +419,47 @@ int kasan_module_alloc(void *addr, size_t size)
return ret ? 0 : -ENOMEM;
}
+struct vfree_deferred {
+ struct llist_head list;
+ struct work_struct wq;
+};
+static DEFINE_PER_CPU(struct vfree_deferred, vfree_deferred);
+
+static void free_work(struct work_struct *w)
+{
+ struct vfree_deferred *p = container_of(w, struct vfree_deferred, wq);
+ struct llist_node *llnode = llist_del_all(&p->list);
+ while (llnode) {
+ void *p = llnode;
+ llnode = llist_next(llnode);
+ vfree(kasan_mem_to_shadow(p));
+ vfree(p);
+ }
+}
+
void kasan_module_free(void *addr)
{
- vfree(kasan_mem_to_shadow(addr));
+ if (unlikely(in_interrupt())) {
+ struct vfree_deferred *p = this_cpu_ptr(&vfree_deferred);
+ if (llist_add((struct llist_node *)addr, &p->list))
+ schedule_work(&p->wq);
+ } else {
+ vfree(kasan_mem_to_shadow(addr));
+ vfree(addr);
+ }
+}
+
+void __init kasan_modules_init(void)
+{
+ int i;
+
+ for_each_possible_cpu(i) {
+ struct vfree_deferred *p;
+
+ p = &per_cpu(vfree_deferred, i);
+ init_llist_head(&p->list);
+ INIT_WORK(&p->wq, free_work);
+ }
}
static void register_global(struct kasan_global *global)
--
2.3.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
