[PATCH] module interface improvement for kprobes

[PATCH] module interface improvement for kprobes

[David Smith]

When inserting a kprobes probe into a loadable module, there isn't a way
for the kprobes module to get a module reference (in order to find the
base address of the module among perhaps other things).

A kprobes probe needs the base address of the module in order to
"relocate" the addresses of probe points (since the load address of the
module can change from run to run of the kprobe).

I've added a new function, module_get_byname(), that looks up a module
by name and returns the module reference.  Note that module_get_byname()
also increments the module reference count.  It does this so that the
module won't be unloaded between the time that module_get_byname() is
called and register_kprobe() is called.

Here's an example of how it would be used from a kprobe:

====
static struct module *mod = NULL;

int init_module(void)
{
	/* grab the module, making sure it won't get unloaded until
	 * we're done */
	const char *mod_name = "joydev";
	if (module_get_byname(mod_name, &mod) != 0)
		return 1;

	/* Specify the address/offset where you want to insert
	 * probe.  If this were a real kprobe module, we'd "relocate"
	 * our probe address based on the load address of the module
	 * we're interested in. */
	kp.addr = (kprobe_opcode_t *) mod->module_core + 0;

	/* All set to register with Kprobes */
        register_kprobe(&kp);
	return 0;
}

void cleanup_module(void)
{
	if (kp.addr != NULL)
		unregister_kprobe(&kp);

	/* allow the module to get unloaded, if needed */
	if (mod != NULL)
		module_put(mod);
}
====

I've included a patch that implements the new function. 

Signed-off-by: David Smith <dsmith@redhat.com>
---

diff --git a/include/linux/module.h b/include/linux/module.h
index 0dfb794..7464e29 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -374,6 +374,8 @@ extern void __module_put_and_exit(struct
 	__attribute__((noreturn));
 #define module_put_and_exit(code) __module_put_and_exit(THIS_MODULE,
code);
 
+long module_get_byname(const char *mod_name, struct module **mod);
+
 #ifdef CONFIG_MODULE_UNLOAD
 unsigned int module_refcount(struct module *mod);
 void __symbol_put(const char *symbol);
@@ -549,6 +551,11 @@ static inline int is_exported(const char
 	return 0;
 }
 
+static inline long module_get_byname(const char *mod_name, struct
module **mod)
+{
+	return 1;
+}
+
 static inline int register_module_notifier(struct notifier_block * nb)
 {
 	/* no events will happen anyway, so this can always succeed */
diff --git a/kernel/module.c b/kernel/module.c
index 2a19cd4..30449ce 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -291,7 +291,27 @@ static struct module *find_module(const 
 	}
 	return NULL;
 }
+  
+long module_get_byname(const char *mod_name, struct module **mod)
+{
+	*mod = NULL;
 
+	/* We must hold module_mutex before calling find_module(). */
+	if (mutex_lock_interruptible(&module_mutex) != 0)
+		return -EINTR;
+
+	*mod = find_module(mod_name);
+	mutex_unlock(&module_mutex);
+	if (*mod) {
+		if (! strong_try_module_get(*mod)) {
+			*mod = NULL;
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
+EXPORT_SYMBOL(module_get_byname);
+ 
 #ifdef CONFIG_SMP
 /* Number of blocks used and allocated. */
 static unsigned int pcpu_num_used, pcpu_num_allocated;

Re: [PATCH] module interface improvement for kprobes

[Christoph Hellwig]

> {
> 	/* grab the module, making sure it won't get unloaded until
> 	 * we're done */
> 	const char *mod_name = "joydev";
> 	if (module_get_byname(mod_name, &mod) != 0)
> 		return 1;
> 
> 	/* Specify the address/offset where you want to insert
> 	 * probe.  If this were a real kprobe module, we'd "relocate"
> 	 * our probe address based on the load address of the module
> 	 * we're interested in. */
> 	kp.addr = (kprobe_opcode_t *) mod->module_core + 0;
> 
> 	/* All set to register with Kprobes */
>         register_kprobe(&kp);
> 	return 0;
> }

This interface is horrible.  You actual patch looks good to me, but it
I can't see why you would need it.  kallsyms_lookup_name deals with modules
transparently and you shouldn't put a probe at a relative offset into a
module but only at a symbol you could find with kallsys.

That beeing said we should probably change the kprobes interface to
automatically do the kallsysms name lookup for the caller.  It would simplify
the kprobes interface and allow us to get rid of the kallsyms_lookup_name
export that doesn't have a valid use except for kprobes.  With
that change the example kprobe would look like:

static struct kprobe kp = {
	.pre_handler	= handler_pre,
	.post_handler	= handler_post,
	.fault_handler	= handler_fault,
	.symbol_name	= "do_fork",
};

static int __init probe_example_init(void)
{
	return register_kprobe(&kp);
}

(and btw, init_module is gone, so both your example and the one in
Documentation/kprobes.txt can't compile anymore - care to send a patch
to update the latter?)

Re: [PATCH] module interface improvement for kprobes

[David Smith]

Christoph,

Thanks for thinking about this.  See comments below.

On Fri, 2006-08-04 at 16:57 +0100, Christoph Hellwig wrote:
> > {
> > 	/* grab the module, making sure it won't get unloaded until
> > 	 * we're done */
> > 	const char *mod_name = "joydev";
> > 	if (module_get_byname(mod_name, &mod) != 0)
> > 		return 1;
> > 
> > 	/* Specify the address/offset where you want to insert
> > 	 * probe.  If this were a real kprobe module, we'd "relocate"
> > 	 * our probe address based on the load address of the module
> > 	 * we're interested in. */
> > 	kp.addr = (kprobe_opcode_t *) mod->module_core + 0;
> > 
> > 	/* All set to register with Kprobes */
> >         register_kprobe(&kp);
> > 	return 0;
> > }
> 
> This interface is horrible.  You actual patch looks good to me, but it
> I can't see why you would need it.  kallsyms_lookup_name deals with modules
> transparently and you shouldn't put a probe at a relative offset into a
> module but only at a symbol you could find with kallsys.

Why shouldn't I put a probe into a module other than at a symbol I can
find with kallsyms?  For example, I'm interested when a particular
module hits an error condition that occurs.  I don't want to probe how
many times the function gets called - just when the error condition
occurs.

With the existing interface, if I use kallsysms to find the value of a
symbol, the module can be unloaded between the time I use kallsyms and
register the kprobe.  The patch I included fixes that race condition by
incrementing the module reference count.

> That beeing said we should probably change the kprobes interface to
> automatically do the kallsysms name lookup for the caller.  It would simplify
> the kprobes interface and allow us to get rid of the kallsyms_lookup_name
> export that doesn't have a valid use except for kprobes.  With
> that change the example kprobe would look like:
> 
> static struct kprobe kp = {
> 	.pre_handler	= handler_pre,
> 	.post_handler	= handler_post,
> 	.fault_handler	= handler_fault,
> 	.symbol_name	= "do_fork",
> };
> 
> static int __init probe_example_init(void)
> {
> 	return register_kprobe(&kp);
> }

Your example works for a very small number of symbols, but with a large
number it could take a long time to register the kprobes.  Plus, that
would need to be done every time the kprobe was registered.  With my
patch, the symbol lookup can be done once, then all those symbols can be
turned into offsets from the base address of the module.

-- 
David Smith
dsmith@redhat.com
Red Hat, Inc.
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Re: [PATCH] module interface improvement for kprobes

[Christoph Hellwig]

On Fri, Aug 04, 2006 at 01:30:39PM -0500, David Smith wrote:
> Why shouldn't I put a probe into a module other than at a symbol I can
> find with kallsyms?  For example, I'm interested when a particular
> module hits an error condition that occurs.  I don't want to probe how
> many times the function gets called - just when the error condition
> occurs.

How do you find that offset?  You'll probably mention the S-Word but
we really want something that works with the latest kernel, not just
the vendor trees.

> With the existing interface, if I use kallsysms to find the value of a
> symbol, the module can be unloaded between the time I use kallsyms and
> register the kprobe.  The patch I included fixes that race condition by
> incrementing the module reference count.

Yes, and that's a good thing.  But the interface for doing it is wrong.
You don't really want the users to do all that by itself.  For the typical
case of putting a probe at the usual points you want an interface that
puts in the probe given a name and does the right thing for you.  For example
the interface I proposed in my last mail.  Adding another field to struct
kprobe to specify an offset into the symbol would be the logical extension
of that.

> Your example works for a very small number of symbols, but with a large
> number it could take a long time to register the kprobes.  Plus, that
> would need to be done every time the kprobe was registered.  With my
> patch, the symbol lookup can be done once, then all those symbols can be
> turned into offsets from the base address of the module.

Registering a kprobe is everything but a fastpath, and you definitly should
not have a lot of probes anyway.  It's far more worthwhile to have a sane
interface that the user can't get wrong then a small speedup in something
that's not a fastpath.  I think Rusty even has a paper or talk about why
this is absolutely nessecary :)

Re: [PATCH] module interface improvement for kprobes

[Frank Ch. Eigler]

Christoph Hellwig <hch@infradead.org> writes:

> [...]
> > Why shouldn't I put a probe into a module other than at a symbol I can
> > find with kallsyms?  For example, I'm interested when a particular
> > module hits an error condition that occurs.  [...]
>
> How do you find that offset?  

The same way one would find an address now: by a mixture of
online/offline processing of symbol tables, debugging data, and/or
disassembly.

> You'll probably mention the S-Word but we really want something that
> works with the latest kernel, not just the vendor trees.

Why are you under the impression that systemtap doesn't work with any
particular "latest kernel"?

> [...] Adding another field to struct kprobe to specify an offset
> into the symbol would be the logical extension of that.

At the top you ask rhetorically about how an offset would be found (as
if it were difficult) ...  and here you assert that it is a logical
extension to put it into the API?  I'm confused - which is it?

> > Your example works for a very small number of symbols, but with a large
> > number it could take a long time to register the kprobes.  [...]
> Registering a kprobe is everything but a fastpath, and you definitly
> should not have a lot of probes anyway.  [...]

It is not difficult to imagine situations where one wants to have
hundreds or thousands of active probes: one is function entry/exit
tracing; another is assertion checking.  (If only there was a system
for conveniently expressing these ... oh never mind.)

The idea behind module_get_byname() was to avoid changes in kprobes
etc., and keeping in mind that the same sort of module-name lookup is
already available to userspace via sysfs.  If folks insist that
instead, kprobes be extended to do this step of the probe address
calculations internally, I guess we could use that too.  It would have
to punt if !CONFIG_KALLSYMS, which is too bad, since systemtap and
kprobes work fine without kallsyms now.

- FChE

Re: [PATCH] module interface improvement for kprobes

[Ananth N Mavinakayanahalli]

On Fri, Aug 04, 2006 at 04:57:11PM +0100, Christoph Hellwig wrote:
> > {
> > 	/* grab the module, making sure it won't get unloaded until
> > 	 * we're done */
> > 	const char *mod_name = "joydev";
> > 	if (module_get_byname(mod_name, &mod) != 0)
> > 		return 1;
> > 
> > 	/* Specify the address/offset where you want to insert
> > 	 * probe.  If this were a real kprobe module, we'd "relocate"
> > 	 * our probe address based on the load address of the module
> > 	 * we're interested in. */
> > 	kp.addr = (kprobe_opcode_t *) mod->module_core + 0;
> > 
> > 	/* All set to register with Kprobes */
> >         register_kprobe(&kp);
> > 	return 0;
> > }
> 
> This interface is horrible.  You actual patch looks good to me, but it
> I can't see why you would need it.  kallsyms_lookup_name deals with modules
> transparently and you shouldn't put a probe at a relative offset into a
> module but only at a symbol you could find with kallsys.
> 
> That beeing said we should probably change the kprobes interface to
> automatically do the kallsysms name lookup for the caller.  It would simplify
> the kprobes interface and allow us to get rid of the kallsyms_lookup_name
> export that doesn't have a valid use except for kprobes.  With
> that change the example kprobe would look like:

This sounds like a good idea. How about we still allow .addr atleast for
users who know what they are doing and would want to just specify a text
addr?

> static struct kprobe kp = {
	.addr		= <addr>

> 	.pre_handler	= handler_pre,
> 	.post_handler	= handler_post,
> 	.fault_handler	= handler_fault,
> 	.symbol_name	= "do_fork",
> };

The symbol_name lookup can then be done when only when .addr is non-NULL.

That said, I have a working patch I was planning to post today that
introduces the KPROBE_ADDR macro that abstracts out the architecture-specific
artefacts of getting the actual text address to probe, so kprobe modules
can be made more portable. I was envisaging this to be used by the module
writer, but with your idea, this could live in-kernel itself.

I'll cook up a patch for this in a short while.

Ananth

Re: [PATCH] module interface improvement for kprobes

[Ananth N Mavinakayanahalli]

On Mon, Aug 07, 2006 at 10:22:13AM +0530, Ananth N Mavinakayanahalli wrote:
> On Fri, Aug 04, 2006 at 04:57:11PM +0100, Christoph Hellwig wrote:
> > > {
> > > 	/* grab the module, making sure it won't get unloaded until
> > > 	 * we're done */
> > > 	const char *mod_name = "joydev";
> > > 	if (module_get_byname(mod_name, &mod) != 0)
> > > 		return 1;
> > > 
> > > 	/* Specify the address/offset where you want to insert
> > > 	 * probe.  If this were a real kprobe module, we'd "relocate"
> > > 	 * our probe address based on the load address of the module
> > > 	 * we're interested in. */
> > > 	kp.addr = (kprobe_opcode_t *) mod->module_core + 0;
> > > 
> > > 	/* All set to register with Kprobes */
> > >         register_kprobe(&kp);
> > > 	return 0;
> > > }
> > 
> > This interface is horrible.  You actual patch looks good to me, but it
> > I can't see why you would need it.  kallsyms_lookup_name deals with modules
> > transparently and you shouldn't put a probe at a relative offset into a
> > module but only at a symbol you could find with kallsys.
> > 
> > That beeing said we should probably change the kprobes interface to
> > automatically do the kallsysms name lookup for the caller.  It would simplify
> > the kprobes interface and allow us to get rid of the kallsyms_lookup_name
> > export that doesn't have a valid use except for kprobes.  With
> > that change the example kprobe would look like:
> 
> This sounds like a good idea. How about we still allow .addr atleast for
> users who know what they are doing and would want to just specify a text
> addr?
> 
> > static struct kprobe kp = {
> 	.addr		= <addr>
> 
> > 	.pre_handler	= handler_pre,
> > 	.post_handler	= handler_post,
> > 	.fault_handler	= handler_fault,
> > 	.symbol_name	= "do_fork",
> > };
> 
> The symbol_name lookup can then be done when only when .addr is non-NULL.

Duh! I meant to say lookup when .addr is NULL.

Ananth

Re: [PATCH] module interface improvement for kprobes

[David Smith]

On Mon, 2006-08-07 at 10:22 +0530, Ananth N Mavinakayanahalli wrote:
> On Fri, Aug 04, 2006 at 04:57:11PM +0100, Christoph Hellwig wrote:

... stuff deleted ...

> > That beeing said we should probably change the kprobes interface to
> > automatically do the kallsysms name lookup for the caller.  It would simplify
> > the kprobes interface and allow us to get rid of the kallsyms_lookup_name
> > export that doesn't have a valid use except for kprobes.  With
> > that change the example kprobe would look like:
> 
> This sounds like a good idea. How about we still allow .addr atleast for
> users who know what they are doing and would want to just specify a text
> addr?
> 
> > static struct kprobe kp = {
> 	.addr		= <addr>
> 
> > 	.pre_handler	= handler_pre,
> > 	.post_handler	= handler_post,
> > 	.fault_handler	= handler_fault,
> > 	.symbol_name	= "do_fork",
> > };
> 
> The symbol_name lookup can then be done when only when .addr is non-NULL.
> 
> That said, I have a working patch I was planning to post today that
> introduces the KPROBE_ADDR macro that abstracts out the architecture-specific
> artefacts of getting the actual text address to probe, so kprobe modules
> can be made more portable. I was envisaging this to be used by the module
> writer, but with your idea, this could live in-kernel itself.
> 
> I'll cook up a patch for this in a short while.
> 
> Ananth

This does seem reasonable, so I'll abandon my patch and wait for the new
kprobes interface.

Ananth, thanks for helping out.

-- 
David Smith
dsmith@redhat.com
Red Hat, Inc.
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Re: [PATCH] module interface improvement for kprobes

[Randy.Dunlap]

On Fri, 04 Aug 2006 10:17:32 -0500 David Smith wrote:

> When inserting a kprobes probe into a loadable module, there isn't a way
> for the kprobes module to get a module reference (in order to find the
> base address of the module among perhaps other things).
> 
> A kprobes probe needs the base address of the module in order to
> "relocate" the addresses of probe points (since the load address of the
> module can change from run to run of the kprobe).
> 
> I've added a new function, module_get_byname(), that looks up a module
> by name and returns the module reference.  Note that module_get_byname()
> also increments the module reference count.  It does this so that the
> module won't be unloaded between the time that module_get_byname() is
> called and register_kprobe() is called.
> 
> Here's an example of how it would be used from a kprobe:
> 
> ====
> static struct module *mod = NULL;
> 
> int init_module(void)
> {
> 	/* grab the module, making sure it won't get unloaded until
> 	 * we're done */
> 	const char *mod_name = "joydev";
> 	if (module_get_byname(mod_name, &mod) != 0)
> 		return 1;
> 
> 	/* Specify the address/offset where you want to insert
> 	 * probe.  If this were a real kprobe module, we'd "relocate"
> 	 * our probe address based on the load address of the module
> 	 * we're interested in. */
> 	kp.addr = (kprobe_opcode_t *) mod->module_core + 0;
> 
> 	/* All set to register with Kprobes */
>         register_kprobe(&kp);
> 	return 0;
> }
> 
> void cleanup_module(void)
> {
> 	if (kp.addr != NULL)
> 		unregister_kprobe(&kp);
> 
> 	/* allow the module to get unloaded, if needed */
> 	if (mod != NULL)
> 		module_put(mod);
> }
> ====
> 
> I've included a patch that implements the new function. 
> 
> Signed-off-by: David Smith <dsmith@redhat.com>
> ---
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 0dfb794..7464e29 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -374,6 +374,8 @@ extern void __module_put_and_exit(struct
>  	__attribute__((noreturn));
>  #define module_put_and_exit(code) __module_put_and_exit(THIS_MODULE,
> code);
>  
> +long module_get_byname(const char *mod_name, struct module **mod);

Why long instead of int?

> +
>  #ifdef CONFIG_MODULE_UNLOAD
>  unsigned int module_refcount(struct module *mod);
>  void __symbol_put(const char *symbol);
> @@ -549,6 +551,11 @@ static inline int is_exported(const char
>  	return 0;
>  }
>  
> +static inline long module_get_byname(const char *mod_name, struct
> module **mod)
> +{
> +	return 1;
> +}
> +
>  static inline int register_module_notifier(struct notifier_block * nb)
>  {
>  	/* no events will happen anyway, so this can always succeed */
> diff --git a/kernel/module.c b/kernel/module.c
> index 2a19cd4..30449ce 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -291,7 +291,27 @@ static struct module *find_module(const 
>  	}
>  	return NULL;
>  }
> +  
> +long module_get_byname(const char *mod_name, struct module **mod)
> +{
> +	*mod = NULL;
>  
> +	/* We must hold module_mutex before calling find_module(). */
> +	if (mutex_lock_interruptible(&module_mutex) != 0)
> +		return -EINTR;
> +
> +	*mod = find_module(mod_name);
> +	mutex_unlock(&module_mutex);
> +	if (*mod) {
> +		if (! strong_try_module_get(*mod)) {

no space following the '!'.

> +			*mod = NULL;
> +			return -EINVAL;
> +		}
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(module_get_byname);
> + 
>  #ifdef CONFIG_SMP
>  /* Number of blocks used and allocated. */
>  static unsigned int pcpu_num_used, pcpu_num_allocated;


---
~Randy

Re: [PATCH] module interface improvement for kprobes

[David Smith]

Randy,

Thanks for looking at this.  I implemented your suggestions and also
changed EXPORT_SYMBOL to EXPORT_SYMBOL_GPL.

On Fri, 2006-08-04 at 09:00 -0700, Randy.Dunlap wrote:
> On Fri, 04 Aug 2006 10:17:32 -0500 David Smith wrote:
> 
> > When inserting a kprobes probe into a loadable module, there isn't a way
> > for the kprobes module to get a module reference (in order to find the
> > base address of the module among perhaps other things).
> > 
> > A kprobes probe needs the base address of the module in order to
> > "relocate" the addresses of probe points (since the load address of the
> > module can change from run to run of the kprobe).
> > 
> > I've added a new function, module_get_byname(), that looks up a module
> > by name and returns the module reference.  Note that module_get_byname()
> > also increments the module reference count.  It does this so that the
> > module won't be unloaded between the time that module_get_byname() is
> > called and register_kprobe() is called.

...

Signed off by David Smith <dsmith@redhat.com>

----
diff --git a/include/linux/module.h b/include/linux/module.h
index 0dfb794..9953a38 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -374,6 +374,8 @@ extern void __module_put_and_exit(struct
 	__attribute__((noreturn));
 #define module_put_and_exit(code) __module_put_and_exit(THIS_MODULE,
code);
 
+int module_get_byname(const char *mod_name, struct module **mod);
+
 #ifdef CONFIG_MODULE_UNLOAD
 unsigned int module_refcount(struct module *mod);
 void __symbol_put(const char *symbol);
@@ -549,6 +551,11 @@ static inline int is_exported(const char
 	return 0;
 }
 
+static inline int module_get_byname(const char *mod_name, struct module
**mod)
+{
+	return 1;
+}
+
 static inline int register_module_notifier(struct notifier_block * nb)
 {
 	/* no events will happen anyway, so this can always succeed */
diff --git a/kernel/module.c b/kernel/module.c
index 2a19cd4..473cc0b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -291,7 +291,27 @@ static struct module *find_module(const 
 	}
 	return NULL;
 }
+  
+int module_get_byname(const char *mod_name, struct module **mod)
+{
+	*mod = NULL;
 
+	/* We must hold module_mutex before calling find_module(). */
+	if (mutex_lock_interruptible(&module_mutex) != 0)
+		return -EINTR;
+
+	*mod = find_module(mod_name);
+	mutex_unlock(&module_mutex);
+	if (*mod) {
+		if (!strong_try_module_get(*mod)) {
+			*mod = NULL;
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(module_get_byname);
+ 
 #ifdef CONFIG_SMP
 /* Number of blocks used and allocated. */
 static unsigned int pcpu_num_used, pcpu_num_allocated;

Re: [PATCH] module interface improvement for kprobes

[Rusty Russell]

On Fri, 2006-08-04 at 10:17 -0500, David Smith wrote:
> When inserting a kprobes probe into a loadable module, there isn't a way
> for the kprobes module to get a module reference (in order to find the
> base address of the module among perhaps other things).

OK, there's nothing fundamentally wrong with the idea of a new lookup
fn, but does kprobes really have a module name and an offset into the
load address of the module?  Or a symbol name?  Or an offset relative to
a specific section?

It seems to me that the last two options are best.  Both require
kallsyms, but I don't think that's unreasonable...

> +static inline long module_get_byname(const char *mod_name, struct module **mod)
> +{
> +	return 1;
> +}
...
> +long module_get_byname(const char *mod_name, struct module **mod)
> +{
> +	*mod = NULL;
>  
> +	/* We must hold module_mutex before calling find_module(). */
> +	if (mutex_lock_interruptible(&module_mutex) != 0)
> +		return -EINTR;
> +
> +	*mod = find_module(mod_name);
> +	mutex_unlock(&module_mutex);
> +	if (*mod) {
> +		if (! strong_try_module_get(*mod)) {
> +			*mod = NULL;
> +			return -EINVAL;
> +		}
> +	}
> +	return 0;
> +}

Your return values here are confused. Please just return struct module
*.  Also, there doesn't seem to be any reason for this function to exist
in the CONFIG_MODULES=n case.

Cheers!
Rusty,
-- 
Help! Save Australia from the worst of the DMCA: http://linux.org.au/law

Re: [PATCH] module interface improvement for kprobes

[Chuck Ebbert]

In-Reply-To: <1154721061.15967.31.camel@dhcp-2.hsv.redhat.com>

On Fri, 04 Aug 2006 14:51:01 -0500, David Smith wrote:

> > > I've added a new function, module_get_byname(), that looks up a module
> > > by name and returns the module reference.  Note that module_get_byname()
> > > also increments the module reference count.  It does this so that the
> > > module won't be unloaded between the time that module_get_byname() is
> > > called and register_kprobe() is called.

Your patch is word-wrapped.

Also:

> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -291,7 +291,27 @@ static struct module *find_module(const 
>       }
>       return NULL;
>  }
> +  
> +int module_get_byname(const char *mod_name, struct module **mod)
> +{
> +     *mod = NULL;
>  
> +     /* We must hold module_mutex before calling find_module(). */
> +     if (mutex_lock_interruptible(&module_mutex) != 0)
> +             return -EINTR;
> +
> +     *mod = find_module(mod_name);
> +     mutex_unlock(&module_mutex);
> +     if (*mod) {
> +             if (!strong_try_module_get(*mod)) {

What keeps the module from going away between the mutex_unlock() and
strong_try_module_get()? It needs to be something like this:

        *mod = find_module(mod_name);
        if (*mod)
                ret = strong_try_module_get(*mod);
        mutex_unlock(&module_mutex);
        if (!ret) {
                *mod = NULL;
                return -EINVAL;
        }

> +                     *mod = NULL;
> +                     return -EINVAL;
> +             }
> +     }
> +     return 0;
> +}
> +EXPORT_SYMBOL_GPL(module_get_byname);
> + 
>  #ifdef CONFIG_SMP
>  /* Number of blocks used and allocated. */
>  static unsigned int pcpu_num_used, pcpu_num_allocated;

-- 
Chuck