kprobes: disable preempt for module_text_address()

PATCH In review Lai Jiangshan 5 participants 17 messages Nov 4, 2008 – Nov 7, 2008 kprobes

Share & tools

Lai Jiangshan

author

2008-11-04 06:01 UTC #0 lore raw

PATCH 1/1 kprobes: disable preempt for module_text_address()

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

Signed-off-by Lai Jiangshan <[email protected]>

---

kernel/kprobes.c +5 -1

index 8b57a25..8238ec5 100644
@@ -622,6 +622,7 @@ static int __kprobes __register_kprobe(struct kprobe *p,
 	/*
 	 * Check if are we probing a module.
 	 */
+	preempt_disable();
 	probed_mod = module_text_address((unsigned long) p->addr);
 	if (probed_mod) {
 		struct module *calling_mod = module_text_address(called_from);

@@ -631,12 +632,15 @@ static int __kprobes __register_kprobe(struct kprobe *p,
 		 * unloading of self probing modules.
 		 */
 		if (calling_mod && calling_mod != probed_mod) {
-			if (unlikely(!try_module_get(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;
 	INIT_LIST_HEAD(&p->list);

Ananth N Mavinakayanahalli

2008-11-04 14:29 UTC #1 lore raw

Quoted 5 lines · Lai Jiangshan

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

Looking at other users of try_module_get, I don't see this as a usage model being followed elsewhere. Also, in case such a preemption does happen, module_is_live() will fail and we should still be ok.

I don't see a reason for this patch unless there is a clear failure case (register_kprobe failing 'cos of a module unload is perfectly ok).

Ananth

Lai Jiangshan

author

2008-11-05 00:56 UTC #2 lore raw

Quoted 9 lines · Ananth N Mavinakayanahalli

Quoted 4 lines · Lai Jiangshan
__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.
Looking at other users of try_module_get, I don't see this as a usage model being followed elsewhere. Also, in case such a preemption does happen, module_is_live() will fail and we should still be ok.

when preemption happen, and mod is freed, module_is_live() will access to invalid address. So it's NOT OK.

Other users of try_module_get() are correct. most are like this:

void func(XXX, XXXX)

{
	try_module_get(XXX->owner)
}

Because we have had a reference to the module before calling try_module_get(). this means the module is still in the kernel when try_module_get() called. so we do not need any protection for using try_module_get().
<in other word, caller of func() has made sure the module will not be unloaded>

In this function __register_kprobe(), probed_mod is the return value of module_text_address(), probed_mod will go in any time before try_module_get().

Quoted 6 lines

I don't see a reason for this patch unless there is a clear failure case (register_kprobe failing 'cos of a module unload is perfectly ok).
Ananth

Masami Hiramatsu

2008-11-05 01:29 UTC #3 lore raw

In reply to Lai Jiangshan

Hi Lai,

Quoted 4 lines · Lai Jiangshan

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

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Lai Jiangshan

author

2008-11-05 01:50 UTC #4 lore raw

Quoted 38 lines · Masami Hiramatsu

Hi Lai,
Quoted 4 lines · Lai Jiangshan
__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;
}

Quoted 2 lines

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.

actually, calling __module_text_address() in __register_kprobe() is better after my fix applied. but I found that a line have exceed 80 characters, so I don't use __module_text_address().

Thanx, Lai

Hiroshi Shimamoto

2008-11-05 19:30 UTC #5 lore raw

Quoted 38 lines · Lai Jiangshan

Quoted 37 lines · Masami Hiramatsu
Hi Lai,
Quoted 4 lines · Lai Jiangshan
__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;
}

Quoted 8 lines

Quoted 2 lines
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 <[email protected]>
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 <[email protected]>

---
 kernel/kprobes.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

kernel/kprobes.c +13 -6

index 8b57a25..71dc2e9 100644
@@ -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;

Signature · 1 line

1.5.6

Masami Hiramatsu

2008-11-05 21:41 UTC #6 lore raw

In reply to Lai Jiangshan

Quoted 3 lines · Lai Jiangshan

actually, calling __module_text_address() in __register_kprobe() is better after my fix applied. but I found that a line have exceed 80 characters, so I don't use __module_text_address().

I don't think that coding style is a good reason not to fix it...:(

Anyway, I think the issue that you pointed must be fixed. I found there were same kind of issues in kprobes and updated your patch. This includes fixes which Hiroshi pointed out.

Thanks a lot! :)

__register_kprobe() can be preempted after checking probing address but before try_module_get() or module_put(), and in this interval the module can be unloaded. In that case, try_module_get(probed_mod) or module_put(mod) will access to invalid address, or kprobe will probe invalid address.

this patch uses preempt_disable() to protect it and use __module_text_address() and __kernel_text_address().

Signed-off-by Lai Jiangshan <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---
 kernel/kprobes.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

kernel/kprobes.c +15 -6

Index: 2.6.28-rc3/kernel/kprobes.c
===================================================================
@@ -613,30 +613,37 @@ static int __kprobes __register_kprobe(s
 		return -EINVAL;
 	p->addr = addr;
 
-	if (!kernel_text_address((unsigned long) p->addr) ||
-	    in_kprobes_functions((unsigned long) p->addr))
+	preempt_disable();
+	if (!__kernel_text_address((unsigned long) p->addr) ||
+	    in_kprobes_functions((unsigned long) p->addr)) {
+		preempt_enable();
 		return -EINVAL;
+	}
 
 	p->mod_refcounted = 0;
 
 	/*
 	 * Check if are we probing a module.
 	 */
-	probed_mod = module_text_address((unsigned long) p->addr);
+	probed_mod = __module_text_address((unsigned long) p->addr);
 	if (probed_mod) {
-		struct module *calling_mod = module_text_address(called_from);
+		struct module *calling_mod;
+		calling_mod = __module_text_address(called_from);
 		/*
 		 * 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 (unlikely(!try_module_get(probed_mod))) {
+				preempt_enable();
 				return -EINVAL;
+			}
 			p->mod_refcounted = 1;
 		} else
 			probed_mod = NULL;
 	}
+	preempt_enable();
 
 	p->nmissed = 0;
 	INIT_LIST_HEAD(&p->list);

@@ -718,9 +725,11 @@ static void __kprobes __unregister_kprob
 	struct kprobe *old_p;
 
 	if (p->mod_refcounted) {
-		mod = module_text_address((unsigned long)p->addr);
+		preempt_disable();
+		mod = __module_text_address((unsigned long)p->addr);
 		if (mod)
 			module_put(mod);
+		preempt_enable();
 	}
 
 	if (list_empty(&p->list) || list_is_singular(&p->list)) {

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Hiroshi Shimamoto

2008-11-05 22:46 UTC #7 lore raw

Quoted 52 lines · Masami Hiramatsu

Quoted 3 lines · Lai Jiangshan
actually, calling __module_text_address() in __register_kprobe() is better after my fix applied. but I found that a line have exceed 80 characters, so I don't use __module_text_address().
I don't think that coding style is a good reason not to fix it...:(
Anyway, I think the issue that you pointed must be fixed. I found there were same kind of issues in kprobes and updated your patch. This includes fixes which Hiroshi pointed out.
Thanks a lot! :)
__register_kprobe() can be preempted after checking probing address but before try_module_get() or module_put(), and in this interval the module can be unloaded. In that case, try_module_get(probed_mod) or module_put(mod) will access to invalid address, or kprobe will probe invalid address.
this patch uses preempt_disable() to protect it and use __module_text_address() and __kernel_text_address().
Signed-off-by Lai Jiangshan <[email protected]>
Signed-off-by Masami Hiramatsu <[email protected]>
---
 kernel/kprobes.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)
Index: 2.6.28-rc3/kernel/kprobes.c
===================================================================
kernel/kprobes.c +6 -3
@@ -613,30 +613,37 @@ static int __kprobes __register_kprobe(s
 		return -EINVAL;
 	p->addr = addr;
 
-	if (!kernel_text_address((unsigned long) p->addr) ||
-	    in_kprobes_functions((unsigned long) p->addr))
+	preempt_disable();
+	if (!__kernel_text_address((unsigned long) p->addr) ||
+	    in_kprobes_functions((unsigned long) p->addr)) {
+		preempt_enable();
 		return -EINVAL;
+	}
 
 	p->mod_refcounted = 0;
 
 	/*
 	 * Check if are we probing a module.
 	 */
-	probed_mod = module_text_address((unsigned long) p->addr);
+	probed_mod = __module_text_address((unsigned long) p->addr);

kernel/kprobes.c static int __kprobes __register_kprobe(s · line 637

Quoted 10 lines

kernel/kprobes.c +2 -1

 	if (probed_mod) {
-		struct module *calling_mod = module_text_address(called_from);
+		struct module *calling_mod;
+		calling_mod = __module_text_address(called_from);
 		/*
 		 * 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) {

One question, off topic.
If calling_mod is NULL, no try_module_get(), is that OK?

thanks,
Hiroshi Shimamoto

Quoted 28 lines

kernel/kprobes.c +7 -2

-			if (unlikely(!try_module_get(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;
 	INIT_LIST_HEAD(&p->list);

@@ -718,9 +725,11 @@ static void __kprobes __unregister_kprob
 	struct kprobe *old_p;
 
 	if (p->mod_refcounted) {
-		mod = module_text_address((unsigned long)p->addr);
+		preempt_disable();
+		mod = __module_text_address((unsigned long)p->addr);
 		if (mod)
 			module_put(mod);
+		preempt_enable();
 	}
 
 	if (list_empty(&p->list) || list_is_singular(&p->list)) {

Masami Hiramatsu

2008-11-05 23:09 UTC #8 lore raw

Quoted 5 lines · Hiroshi Shimamoto

Quoted 5 lines

kernel/kprobes.c +2 -1

 	if (probed_mod) {
-		struct module *calling_mod = module_text_address(called_from);
+		struct module *calling_mod;
+		calling_mod = __module_text_address(called_from);
 		/*

Quoted 8 lines

kernel/kprobes.c static int __kprobes __register_kprobe(s · line 637
Quoted 5 lines
kernel/kprobes.c +0 -0
 		 * 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) {
One question, off topic.
If calling_mod is NULL, no try_module_get(), is that OK?

Good question. Currently, kprobes is called only from kernel modules, so calling_mod should be always !NULL.
However, it should be fixed, because the logic is not correct.

Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Masami Hiramatsu

2008-11-06 00:08 UTC #9 lore raw

PATCH 1/1 kprobes: bugfix: try_module_get even if calling_mod is NULL

Get probed module even if the caller is in the kernel core code.

Signed-off-by Masami Hiramatsu <[email protected]>

---

Quoted 6 lines

Quoted 2 lines
One question, off topic.
If calling_mod is NULL, no try_module_get(), is that OK?
Good question. Currently, kprobes is called only from kernel modules, so calling_mod should be always !NULL.
However, it should be fixed, because the logic is not correct.

Thank you so much. So here is the additional bugfix patch.

 kernel/kprobes.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/kprobes.c +1 -1

Index: 2.6.28-rc3/kernel/kprobes.c
===================================================================
@@ -634,7 +634,7 @@ static int __kprobes __register_kprobe(s
 		 * avoid incrementing the module refcount, so as to allow
 		 * unloading of self probing modules.
 		 */
-		if (calling_mod && calling_mod != probed_mod) {
+		if (calling_mod != probed_mod) {
 			if (unlikely(!try_module_get(probed_mod))) {
 				preempt_enable();
 				return -EINVAL;

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Andrew Morton

2008-11-07 01:01 UTC #10 lore raw

Re: [PATCH] kprobes: bugfix: try_module_get even if calling_mod is NULL

Quoted 23 lines · On Wed, 05 Nov 2008 19:06:57 -0500 Masami Hiramatsu <[email protected]> wrote:

Get probed module even if the caller is in the kernel core code.
Signed-off-by Masami Hiramatsu <[email protected]>
---
Quoted 6 lines
Quoted 2 lines
One question, off topic.
If calling_mod is NULL, no try_module_get(), is that OK?
Good question. Currently, kprobes is called only from kernel modules, so calling_mod should be always !NULL.
However, it should be fixed, because the logic is not correct.
Thank you so much. So here is the additional bugfix patch.
 kernel/kprobes.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: 2.6.28-rc3/kernel/kprobes.c
===================================================================

kernel/kprobes.c static int __kprobes __register_kprobe(s · line 640

Quoted 8 lines

kernel/kprobes.c +1 -1

@@ -634,7 +634,7 @@ static int __kprobes __register_kprobe(s
 		 * avoid incrementing the module refcount, so as to allow
 		 * unloading of self probing modules.
 		 */
-		if (calling_mod && calling_mod != probed_mod) {
+		if (calling_mod != probed_mod) {
 			if (unlikely(!try_module_get(probed_mod))) {
 				preempt_enable();
 				return -EINVAL;

I do not understand this description "Get probed module even if the caller is in the kernel core code".

What bug is being fixed here? What is the kernel behaviour before and after the patch?

Was the bug present in 2.6.27, 2.6.26 etc? Or was it a post-2.6.28 regression?

Masami Hiramatsu

2008-11-07 02:30 UTC #11 lore raw

Re: [PATCH] kprobes: bugfix: try_module_get even if calling_mod is NULL

Quoted 5 lines · Andrew Morton

I do not understand this description "Get probed module even if the caller is in the kernel core code".
What bug is being fixed here? What is the kernel behaviour before and after the patch?

When someone called register_*probe() from kernel-core code(not from module) and that probes a kernel module, users can remove the probed module because kprobe doesn't increment reference counter of the module. (on the other hand, if the kernel-module calls register_*probe, kprobe increments refcount of the probed module.)

Currently, we have no register_*probe() calling from kernel-core(except smoke-test, but the smoke-test doesn't probe module), so there is no real bugs. But the logic is wrong(or not fair) and it can causes a problem when someone might want to probe module from kernel.

After this patch is applied, even if someone put register_*probe() call in the kernel-core code, it increments the reference counter of the probed module, and it prevents user to remove the module until stopping probing it.

Quoted 2 lines

Was the bug present in 2.6.27, 2.6.26 etc? Or was it a post-2.6.28 regression?

Hmm, it might be an enhancement, because currently the kernel doesn't have real bugs.

Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Andrew Morton

2008-11-07 02:55 UTC #12 lore raw

Re: [PATCH] kprobes: bugfix: try_module_get even if calling_mod is NULL

Quoted 23 lines · On Fri, 07 Nov 2008 11:28:02 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 5 lines · Andrew Morton
I do not understand this description "Get probed module even if the caller is in the kernel core code".
What bug is being fixed here? What is the kernel behaviour before and after the patch?
When someone called register_*probe() from kernel-core code(not from module) and that probes a kernel module, users can remove the probed module because kprobe doesn't increment reference counter of the module. (on the other hand, if the kernel-module calls register_*probe, kprobe increments refcount of the probed module.)
Currently, we have no register_*probe() calling from kernel-core(except smoke-test, but the smoke-test doesn't probe module), so there is no real bugs. But the logic is wrong(or not fair) and it can causes a problem when someone might want to probe module from kernel.
After this patch is applied, even if someone put register_*probe() call in the kernel-core code, it increments the reference counter of the probed module, and it prevents user to remove the module until stopping probing it.

Quoted 6 lines

Quoted 2 lines
Was the bug present in 2.6.27, 2.6.26 etc? Or was it a post-2.6.28 regression?
Hmm, it might be an enhancement, because currently the kernel doesn't have real bugs.

OK, thanks, so I scheduled this for 2.6.29.

Also, I decided that kprobes-disable-preempt-for-module_text_address-and-kernel_text_address.patch is needed in 2.6.28. Please let me know if that was incorrect. Please also let me know if you think that patch is needed in 2.6.27.x or earlier.

Ananth N Mavinakayanahalli

2008-11-07 04:47 UTC #13 lore raw

Re: [PATCH] kprobes: bugfix: try_module_get even if calling_mod is NULL

Quoted 32 lines · Andrew Morton

Quoted 23 lines · On Fri, 07 Nov 2008 11:28:02 +0900 Masami Hiramatsu <[email protected]> wrote:
Quoted 5 lines · Andrew Morton
I do not understand this description "Get probed module even if the caller is in the kernel core code".
What bug is being fixed here? What is the kernel behaviour before and after the patch?
When someone called register_*probe() from kernel-core code(not from module) and that probes a kernel module, users can remove the probed module because kprobe doesn't increment reference counter of the module. (on the other hand, if the kernel-module calls register_*probe, kprobe increments refcount of the probed module.)
Currently, we have no register_*probe() calling from kernel-core(except smoke-test, but the smoke-test doesn't probe module), so there is no real bugs. But the logic is wrong(or not fair) and it can causes a problem when someone might want to probe module from kernel.
After this patch is applied, even if someone put register_*probe() call in the kernel-core code, it increments the reference counter of the probed module, and it prevents user to remove the module until stopping probing it.
Quoted 6 lines
Quoted 2 lines
Was the bug present in 2.6.27, 2.6.26 etc? Or was it a post-2.6.28 regression?
Hmm, it might be an enhancement, because currently the kernel doesn't have real bugs.

Quoted 7 lines

OK, thanks, so I scheduled this for 2.6.29.
Also, I decided that kprobes-disable-preempt-for-module_text_address-and-kernel_text_address.patch is needed in 2.6.28. Please let me know if that was incorrect. Please also let me know if you think that patch is needed in 2.6.27.x or earlier.

That is correct and this doesn't warrant inclusion in 2.6.27.x.

Ananth

Lai Jiangshan

author

2008-11-06 01:09 UTC #14 lore raw

In reply to Masami Hiramatsu

Quoted 6 lines · Masami Hiramatsu

Quoted 3 lines · Lai Jiangshan
actually, calling __module_text_address() in __register_kprobe() is better after my fix applied. but I found that a line have exceed 80 characters, so I don't use __module_text_address().
I don't think that coding style is a good reason not to fix it...:(

in my patch, module_text_address() had fixed problems.
the meaning of what I said is that: since I have called preempt_disable(), calling __module_text_address() in __register_kprobe() is little better. actually, calling any one of this two is OK since we disabled preempt.

As I remember, In the previous mail, you want to fix module_text_address(). I wanted to say that: using __module_text_address() instead of module_text_address(), rather than fixing module_text_address().

Quoted 13 lines

Anyway, I think the issue that you pointed must be fixed. I found there were same kind of issues in kprobes and updated your patch. This includes fixes which Hiroshi pointed out.
Thanks a lot! :)
__register_kprobe() can be preempted after checking probing address but before try_module_get() or module_put(), and in this interval the module can be unloaded. In that case, try_module_get(probed_mod) or module_put(mod) will access to invalid address, or kprobe will probe invalid address.

Quoted 6 lines

this patch uses preempt_disable() to protect it and use __module_text_address() and __kernel_text_address().
Signed-off-by Lai Jiangshan <[email protected]>
Signed-off-by Masami Hiramatsu <[email protected]>
---

there is a bad fix in this patch.

Quoted 51 lines

 kernel/kprobes.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Index: 2.6.28-rc3/kernel/kprobes.c
===================================================================

kernel/kprobes.c +12 -5

@@ -613,30 +613,37 @@ static int __kprobes __register_kprobe(s
 		return -EINVAL;
 	p->addr = addr;
 
-	if (!kernel_text_address((unsigned long) p->addr) ||
-	    in_kprobes_functions((unsigned long) p->addr))
+	preempt_disable();
+	if (!__kernel_text_address((unsigned long) p->addr) ||
+	    in_kprobes_functions((unsigned long) p->addr)) {
+		preempt_enable();
 		return -EINVAL;
+	}
 
 	p->mod_refcounted = 0;
 
 	/*
 	 * Check if are we probing a module.
 	 */
-	probed_mod = module_text_address((unsigned long) p->addr);
+	probed_mod = __module_text_address((unsigned long) p->addr);
 	if (probed_mod) {
-		struct module *calling_mod = module_text_address(called_from);
+		struct module *calling_mod;
+		calling_mod = __module_text_address(called_from);
 		/*
 		 * 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 (unlikely(!try_module_get(probed_mod))) {
+				preempt_enable();
 				return -EINVAL;
+			}
 			p->mod_refcounted = 1;
 		} else
 			probed_mod = NULL;
 	}
+	preempt_enable();
 
 	p->nmissed = 0;
 	INIT_LIST_HEAD(&p->list);

kernel/kprobes.c static void __kprobes __unregister_kprob · line 732

Quoted 9 lines

kernel/kprobes.c +3 -1

@@ -718,9 +725,11 @@ static void __kprobes __unregister_kprob
 	struct kprobe *old_p;
 
 	if (p->mod_refcounted) {
-		mod = module_text_address((unsigned long)p->addr);
+		preempt_disable();
+		mod = __module_text_address((unsigned long)p->addr);
 		if (mod)
 			module_put(mod);
+		preempt_enable();

this is bad fix, we have had a reference to mod. we don't need preempt_disable() before module_put(mod).

Quoted 5 lines

kernel/kprobes.c +0 -0

 	}
 
 	if (list_empty(&p->list) || list_is_singular(&p->list)) {

Masami Hiramatsu

2008-11-06 15:39 UTC #15 lore raw

PATCH 1/1 kprobes: disable preempt for module_text_address() and kernel_text_address()

__register_kprobe() can be preempted after checking probing address but before module_text_address() or try_module_get(), and in this interval the module can be unloaded. In that case, try_module_get(probed_mod) will access to invalid address, or kprobe will probe invalid address.

this patch uses preempt_disable() to protect it and use __module_text_address() and __kernel_text_address().

Signed-off-by Lai Jiangshan <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---

Quoted 13 lines

Review static void __kprobes __unregister_kprob · line 732
Quoted 10 lines
(quoted diff) +3 -1
@@ -718,9 +725,11 @@ static void __kprobes __unregister_kprob
 	struct kprobe *old_p;
 
 	if (p->mod_refcounted) {
-		mod = module_text_address((unsigned long)p->addr);
+		preempt_disable();
+		mod = __module_text_address((unsigned long)p->addr);
 		if (mod)
 			module_put(mod);
+		preempt_enable();
this is bad fix, we have had a reference to mod. we don't need preempt_disable() before module_put(mod).

Hmm, Indeed.
So let's add a comment there.

 kernel/kprobes.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

kernel/kprobes.c +16 -5

Index: 2.6.28-rc3/kernel/kprobes.c
===================================================================
@@ -613,30 +613,37 @@ static int __kprobes __register_kprobe(s
 		return -EINVAL;
 	p->addr = addr;
 
-	if (!kernel_text_address((unsigned long) p->addr) ||
-	    in_kprobes_functions((unsigned long) p->addr))
+	preempt_disable();
+	if (!__kernel_text_address((unsigned long) p->addr) ||
+	    in_kprobes_functions((unsigned long) p->addr)) {
+		preempt_enable();
 		return -EINVAL;
+	}
 
 	p->mod_refcounted = 0;
 
 	/*
 	 * Check if are we probing a module.
 	 */
-	probed_mod = module_text_address((unsigned long) p->addr);
+	probed_mod = __module_text_address((unsigned long) p->addr);
 	if (probed_mod) {
-		struct module *calling_mod = module_text_address(called_from);
+		struct module *calling_mod;
+		calling_mod = __module_text_address(called_from);
 		/*
 		 * 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 (unlikely(!try_module_get(probed_mod))) {
+				preempt_enable();
 				return -EINVAL;
+			}
 			p->mod_refcounted = 1;
 		} else
 			probed_mod = NULL;
 	}
+	preempt_enable();
 
 	p->nmissed = 0;
 	INIT_LIST_HEAD(&p->list);

@@ -718,6 +725,10 @@ static void __kprobes __unregister_kprob
 	struct kprobe *old_p;
 
 	if (p->mod_refcounted) {
+		/*
+		 * Since we've already incremented refcount,
+		 * we don't need to disable preemption.
+		 */
 		mod = module_text_address((unsigned long)p->addr);
 		if (mod)
 			module_put(mod);

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Lai Jiangshan

author

2008-11-07 00:36 UTC #16 lore raw

Re: [PATCH] kprobes: disable preempt for module_text_address() and kernel_text_address()

Quoted 8 lines · Masami Hiramatsu

__register_kprobe() can be preempted after checking probing address but before module_text_address() or try_module_get(), and in this interval the module can be unloaded. In that case, try_module_get(probed_mod) will access to invalid address, or kprobe will probe invalid address.
this patch uses preempt_disable() to protect it and use __module_text_address() and __kernel_text_address().

It's good, Thanks.

Quoted 83 lines

Signed-off-by Lai Jiangshan <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---

Quoted 12 lines

kernel/kprobes.c static void __kprobes __unregister_kprob · line 732
Quoted 10 lines
kernel/kprobes.c +3 -1
@@ -718,9 +725,11 @@ static void __kprobes __unregister_kprob
 	struct kprobe *old_p;
 
 	if (p->mod_refcounted) {
-		mod = module_text_address((unsigned long)p->addr);
+		preempt_disable();
+		mod = __module_text_address((unsigned long)p->addr);
 		if (mod)
 			module_put(mod);
+		preempt_enable();
this is bad fix, we have had a reference to mod. we don't need preempt_disable() before module_put(mod).

Hmm, Indeed.
So let's add a comment there.

 kernel/kprobes.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

Index: 2.6.28-rc3/kernel/kprobes.c
===================================================================

kernel/kprobes.c +16 -5

@@ -613,30 +613,37 @@ static int __kprobes __register_kprobe(s
 		return -EINVAL;
 	p->addr = addr;
 
-	if (!kernel_text_address((unsigned long) p->addr) ||
-	    in_kprobes_functions((unsigned long) p->addr))
+	preempt_disable();
+	if (!__kernel_text_address((unsigned long) p->addr) ||
+	    in_kprobes_functions((unsigned long) p->addr)) {
+		preempt_enable();
 		return -EINVAL;
+	}
 
 	p->mod_refcounted = 0;
 
 	/*
 	 * Check if are we probing a module.
 	 */
-	probed_mod = module_text_address((unsigned long) p->addr);
+	probed_mod = __module_text_address((unsigned long) p->addr);
 	if (probed_mod) {
-		struct module *calling_mod = module_text_address(called_from);
+		struct module *calling_mod;
+		calling_mod = __module_text_address(called_from);
 		/*
 		 * 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 (unlikely(!try_module_get(probed_mod))) {
+				preempt_enable();
 				return -EINVAL;
+			}
 			p->mod_refcounted = 1;
 		} else
 			probed_mod = NULL;
 	}
+	preempt_enable();
 
 	p->nmissed = 0;
 	INIT_LIST_HEAD(&p->list);

@@ -718,6 +725,10 @@ static void __kprobes __unregister_kprob
 	struct kprobe *old_p;
 
 	if (p->mod_refcounted) {
+		/*
+		 * Since we've already incremented refcount,
+		 * we don't need to disable preemption.
+		 */
 		mod = module_text_address((unsigned long)p->addr);
 		if (mod)
 			module_put(mod);