module interface improvement for kprobes
PATCH 1/1 module interface improvement for kprobes
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.
---
include/linux/module.h +0 -0
@@ -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 */
kernel/module.c +20 -0
@@ -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;
Quoted 9 lines
{
/* 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"Quoted 8 lines
* 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?)
Christoph,
Thanks for thinking about this. See comments below.
Quoted 18 lines · Christoph Hellwig
Quoted 9 lines
{ /* 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"Quoted 8 lines
* 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; }
Quoted 4 lines
This interface is horrible. You actual patch looks good to me, but it I can't see why you would need it.
kallsyms_lookup_namedeals 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.
Quoted 13 lines
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_nameexport 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",
};
Quoted 4 lines
static int
__initprobe_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.
Signature · 6 lines
David Smith [email protected] Red Hat, Inc. http://www.redhat.com 256.217.0141 (direct) 256.837.0057 (fax)
Quoted 5 lines · David Smith
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.
Quoted 4 lines
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.
Quoted 5 lines
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 :)
Quoted 6 lines · Christoph Hellwig
[...]
Quoted 3 lines
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.
Quoted 2 lines
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"?
Quoted 2 lines
[...] 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?
Quoted 4 lines
Quoted 2 lines
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
Quoted 23 lines · Christoph Hellwig
Quoted 9 lines
{ /* 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"Quoted 8 lines
* 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_namedeals 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.
Quoted 5 lines
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_nameexport 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?
Quoted 1 line
static struct kprobe kp = {
.addr = <addr>
Quoted 5 lines
.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
Quoted 37 lines · Ananth N Mavinakayanahalli
Quoted 23 lines · Christoph Hellwig
Quoted 9 lines
{ /* 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"Quoted 8 lines
* 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_namedeals 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.Quoted 5 lines
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_nameexport 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?
Quoted 1 line
static struct kprobe kp = {
.addr = <addr>
Quoted 7 lines
Quoted 5 lines
.pre_handler =
handler_pre,
.post_handler =handler_post,
.fault_handler =handler_fault,
.symbol_name = "do_fork",
};The
symbol_namelookup can then be done when only when .addr is non-NULL.
Duh! I meant to say lookup when .addr is NULL.
Ananth
Quoted 1 line · Ananth N Mavinakayanahalli
On Fri, Aug 04, 2006 at 04:57:11PM +0100, Christoph Hellwig wrote:
... stuff deleted ...
Quoted 27 lines
Quoted 5 lines
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_nameexport 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?
Quoted 1 line
static struct kprobe kp = {
.addr = <addr>Quoted 5 lines
.pre_handler =
handler_pre,
.post_handler =handler_post,
.fault_handler =handler_fault,
.symbol_name = "do_fork",
};The
symbol_namelookup 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_ADDRmacro 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.
Quoted 3 lines
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.
Signature · 6 lines
David Smith [email protected] Red Hat, Inc. http://www.redhat.com 256.217.0141 (direct) 256.837.0057 (fax)
Quoted 59 lines · 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 thatmodule_get_byname()also increments the module reference count. It does this so that the module won't be unloaded between the time thatmodule_get_byname()is called andregister_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 <[email protected]>---
include/linux/module.h +0
-0
include/linux/module.h extern void __module_put_and_exit(struct · line 378
Quoted 5 lines
include/linux/module.h +1
-0@@ -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?
Quoted 28 lines
include/linux/module.h +1
-0+#ifdef CONFIG_MODULE_UNLOAD unsigned int module_refcount (struct module *mod);void
__symbol_put(const char *symbol);include/linux/module.h +5
-0@@ -549,6 +551,11 @@ static inline int is_exported(const charreturn 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) {include/linux/module.h +0
-0/* no events will happen anyway, so this can always succeed */ kernel/module.c +3
-0@@ -291,7 +291,27 @@ static struct module *find_module(const }return NULL ; } + +long module_get_byname (const char *mod_name,struct module **mod) +{
kernel/module.c static struct module *find_module(const · line 306
Quoted 10 lines
kernel/module.c +9
-0+ *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 '!'.
kernel/module.c static struct module *find_module(const · line 317
Quoted 11 lines
kernel/module.c +8
-0+ *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
Randy,
Thanks for looking at this. I implemented your suggestions and also changed EXPORT_SYMBOL to EXPORT_SYMBOL_GPL.
Quoted 10 lines · Randy.Dunlap
Quoted 8 lines · 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).
Quoted 5 lines
Quoted 5 lines
I've added a new function,
module_get_byname(), that looks up a module by name and returns the module reference. Note thatmodule_get_byname()also increments the module reference count. It does this so that the module won't be unloaded between the time thatmodule_get_byname()is called andregister_kprobe()is called.
...
Signed off by David Smith <[email protected]>
----
include/linux/module.h +0 -0
@@ -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 */
kernel/module.c +20 -0
@@ -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;
Quoted 3 lines · 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).
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...
Quoted 4 lines
kernel/module.c +4
-0+static inline long module_get_byname (const char *mod_name,struct module **mod) +{ +return 1 ; +}
...
Quoted 8 lines
kernel/module.c +7
-0+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 ; +
kernel/module.c static struct module *find_module(const · line 312
Quoted 10 lines
kernel/module.c +10
-0+ *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,
Signature · 1 line
Help! Save Australia from the worst of the DMCA: http://linux.org.au/law