Messages in this thread Patch in this message | | | Date | Wed, 05 Nov 2008 11:30:16 -0800 | From | Hiroshi Shimamoto <> | Subject | Re: [PATCH] kprobes: disable preempt for module_text_address() |
| |
Lai Jiangshan wrote: > Masami Hiramatsu wrote: >> Hi Lai, >> >> Lai Jiangshan wrote: >>> __register_kprobe() may be preempted after module_text_address() >>> but before try_module_get(), and in this interval the module may be >>> unloaded and try_module_get(probed_mod) will access to invalid address. >>> this patch uses preempt_disable() to protect it. >> Thank you for your work. >> >> I think this is the problem of module_text_address() because it can >> return incorrect address of struct module if a preemption happens. >> So, I think the module_text_address() would better to call try_module_get() >> before returning its address, or at least they should comment that caller >> needs disabling preemption. >> >> struct module *module_text_address(unsigned long addr) >> { >> struct module *mod; >> >> preempt_disable(); /* >> * I also think this preemption disabling is not so useful >> * without try_module_get(), because caller have to >> * disable preemption... >> */ >> mod = __module_text_address(addr); >> /* here, try_module_get() is needed. >> * (or commenting "caller must disable preemption!") */ >> preempt_enable(); >> >> /* >> * !!Here!! if the preemption happened, it could return invalid "mod". >> * In that case, even if module_text_address() returns non-NULL, >> * the addr is no longer in any module. >> */ >> return mod; >> } >> >> Thank you, >> > > I don't think module_text_address() are needed to modified. > only __register_kprobe() uses module_text_address()'s return value. > other users of module_text_address() are just for testing it's > return value. so only __register_kprobe() needs disabling preemption > currently.
I guess, there is another issue if the module is unloaded before module_text_address(). p->addr is no longer valid, but in __register_kprobe(), module_text_address() returns NULL means it's not a module.
How about this? -------- From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> Subject: [PATCH] kprobe: fix module checking in __register_kprobe()
__register_kprobe() may be preempted before/after module_text_address(). In this preemption, the module may be unloaded.
If the module is unloaded before module_text_address(), kprobe address becomes invalid. If the module is unloaded after module_text_address() and before try_module_get(), try_module_get(probe_mod) will access invalid address.
Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> --- kernel/kprobes.c | 19 +++++++++++++------ 1 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index 8b57a25..71dc2e9 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -605,7 +605,7 @@ static int __kprobes __register_kprobe(struct kprobe *p, { int ret = 0; struct kprobe *old_p; - struct module *probed_mod; + struct module *probed_mod = NULL; kprobe_opcode_t *addr; addr = kprobe_addr(p); @@ -622,20 +622,27 @@ static int __kprobes __register_kprobe(struct kprobe *p, /* * Check if are we probing a module. */ - probed_mod = module_text_address((unsigned long) p->addr); - if (probed_mod) { - struct module *calling_mod = module_text_address(called_from); + if (!core_kernel_text((unsigned long) p->addr)) { + preempt_disable(); + probed_mod = __module_text_address((unsigned long) p->addr); + if (!probed_mod) { + preempt_enable(); + return -EINVAL; + } /* * We must allow modules to probe themself and in this case * avoid incrementing the module refcount, so as to allow * unloading of self probing modules. */ - if (calling_mod && calling_mod != probed_mod) { - if (unlikely(!try_module_get(probed_mod))) + if (__module_text_address(called_from) != probed_mod) { + if (unlikely(!try_module_get(probed_mod))) { + preempt_enable(); return -EINVAL; + } p->mod_refcounted = 1; } else probed_mod = NULL; + preempt_enable(); } p->nmissed = 0; -- 1.5.6
| |