[PATCH] kprobes: disable preempt for module_text_address()

[PATCH] kprobes: disable preempt for module_text_address()

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

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 8b57a25..8238ec5 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -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);

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

[Ananth N Mavinakayanahalli]

On Tue, Nov 04, 2008 at 01:56:21PM +0800, 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.

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

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

[Lai Jiangshan]

Ananth N Mavinakayanahalli wrote:
> On Tue, Nov 04, 2008 at 01:56:21PM +0800, 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.
> 
> 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().

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

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

[Masami Hiramatsu]

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,

-- 
Masami Hiramatsu

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

e-mail: mhiramat@redhat.com

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

[Lai Jiangshan]

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.

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

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

[Hiroshi Shimamoto]

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

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

[Masami Hiramatsu]

Lai Jiangshan wrote:
> 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 <laijs@cn.fujitsu.com>
Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
---
 kernel/kprobes.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Index: 2.6.28-rc3/kernel/kprobes.c
===================================================================
--- 2.6.28-rc3.orig/kernel/kprobes.c
+++ 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)) {


-- 
Masami Hiramatsu

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

e-mail: mhiramat@redhat.com

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

[Hiroshi Shimamoto]

Masami Hiramatsu wrote:
> Lai Jiangshan wrote:
>> 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 <laijs@cn.fujitsu.com>
> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
> ---
>  kernel/kprobes.c |   21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> Index: 2.6.28-rc3/kernel/kprobes.c
> ===================================================================
> --- 2.6.28-rc3.orig/kernel/kprobes.c
> +++ 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) {

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

thanks,
Hiroshi Shimamoto

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

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

[Masami Hiramatsu]

Hiroshi Shimamoto wrote:
>>  	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?

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,

-- 
Masami Hiramatsu

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

e-mail: mhiramat@redhat.com

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

[Masami Hiramatsu]

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

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
---

>> 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
===================================================================
--- 2.6.28-rc3.orig/kernel/kprobes.c
+++ 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;

-- 
Masami Hiramatsu

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

e-mail: mhiramat@redhat.com

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

[Andrew Morton]

On Wed, 05 Nov 2008 19:06:57 -0500
Masami Hiramatsu <mhiramat@redhat.com> wrote:

> Get probed module even if the caller is in the kernel core code.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
> ---
> 
> >> 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
> ===================================================================
> --- 2.6.28-rc3.orig/kernel/kprobes.c
> +++ 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;
> 

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?

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

[Masami Hiramatsu]

Andrew Morton wrote:
> 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.

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

-- 
Masami Hiramatsu

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

e-mail: mhiramat@redhat.com

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

[Andrew Morton]

On Fri, 07 Nov 2008 11:28:02 +0900 Masami Hiramatsu <mhiramat@redhat.com> wrote:

> Andrew Morton wrote:
> > 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.
> 
> > 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.

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

[Ananth N Mavinakayanahalli]

On Thu, Nov 06, 2008 at 06:54:56PM -0800, Andrew Morton wrote:
> On Fri, 07 Nov 2008 11:28:02 +0900 Masami Hiramatsu <mhiramat@redhat.com> wrote:
> 
> > Andrew Morton wrote:
> > > 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.
> > 
> > > 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.

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

Ananth

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

[Lai Jiangshan]

Masami Hiramatsu wrote:
> Lai Jiangshan wrote:
>> 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().

> 
> 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 <laijs@cn.fujitsu.com>
> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
> ---

there is a bad fix in this patch.

>  kernel/kprobes.c |   21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> Index: 2.6.28-rc3/kernel/kprobes.c
> ===================================================================
> --- 2.6.28-rc3.orig/kernel/kprobes.c
> +++ 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();

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

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

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

[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().

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
---

>> @@ -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
===================================================================
--- 2.6.28-rc3.orig/kernel/kprobes.c
+++ 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);

-- 
Masami Hiramatsu

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

e-mail: mhiramat@redhat.com

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

[Lai Jiangshan]

Masami Hiramatsu wrote:
> __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.

> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
> ---
> 
>>> @@ -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
> ===================================================================
> --- 2.6.28-rc3.orig/kernel/kprobes.c
> +++ 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);
>