Hi Prasanna,
Here's a patch that handles gracefully attempts to unregister
unregistered kprobes (ie. a warning message instead of an oops).
Patch is against 2.6.12-rc3
Signed-off-by: Frederik Deweerdt <[email protected]>
Regards,
Frederik
--
o----------------------------------------------o
| http://open-news.net : l'info alternative |
| Tech - Sciences - Politique - International |
o----------------------------------------------o
> @@ -107,6 +107,13 @@ rm_kprobe:
> void unregister_kprobe(struct kprobe *p)
> {
> unsigned long flags;
> +
> + if (!get_kprobe(p)) {
> + printk(KERN_WARNING "Warning: Attempt to unregister "
> + "unknown kprobe (addr:0x%lx)\n",
> + (unsigned long) p);
> + return;
> + }
This is wrong. You should call get_kprobe() with spin_lock().
-Prasanna
--
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<[email protected]>
On Tue, Apr 26, 2005 at 06:22:03PM +0200, Frederik Deweerdt wrote:
> Here's a patch that handles gracefully attempts to unregister
> unregistered kprobes (ie. a warning message instead of an oops).
> Patch is against 2.6.12-rc3
Why not -EINVAL instead of the printk?
Le 26/04/05 21:57 +0530, Prasanna S Panchamukhi ?crivit:
> This is wrong. You should call get_kprobe() with spin_lock().
>
Right, corrected patch attached. It also sets flags to zero.
Signed-off-by: Frederik Deweerdt <[email protected]>
Regards,
Frederik
--
o----------------------------------------------o
| http://open-news.net : l'info alternative |
| Tech - Sciences - Politique - International |
o----------------------------------------------o
On Tue, 26 Apr 2005, Frederik Deweerdt wrote:
> Le 26/04/05 21:57 +0530, Prasanna S Panchamukhi ?crivit:
> > This is wrong. You should call get_kprobe() with spin_lock().
> >
> Right, corrected patch attached. It also sets flags to zero.
> Signed-off-by: Frederik Deweerdt <[email protected]>
> --- linux-2.6.12-rc3/kernel/kprobes.c 2005-04-26 16:35:22.000000000 +0200
> +++ linux-2.6.12-rc3-devel/kernel/kprobes.c 2005-04-26 23:18:47.000000000 +0200
> @@ -106,13 +106,22 @@ rm_kprobe:
>
> void unregister_kprobe(struct kprobe *p)
> {
> - unsigned long flags;
> + unsigned long flags = 0;
> +
> + spin_lock_irqsave(&kprobe_lock, flags);
^^^
+one...
> + if (!get_kprobe(p)) {
> + printk(KERN_WARNING "Warning: Attempt to unregister "
> + "unknown kprobe (addr:0x%lx)\n",
> + (unsigned long) p);
> + goto out;
> + }
> arch_remove_kprobe(p);
> spin_lock_irqsave(&kprobe_lock, flags);
^^^
+two...
> *p->addr = p->opcode;
> hlist_del(&p->hlist);
> flush_icache_range((unsigned long) p->addr,
> (unsigned long) p->addr + sizeof(kprobe_opcode_t));
> +out:
> spin_unlock_irqrestore(&kprobe_lock, flags);
^^^
-one...
> }
Seems to me this will end up calling spin_lock_irqsave() twice, but only
spin_unlock_irqrestore once in the non-failing case... hmm..
Also, as Chris Wedgwood asked, why not simply return -EINVAL; instead of
the printk()? Does the user really care that we tried to unregister a
nonexisting kprobe? and if you really think someone would like to know
then I'd personally say that KERN_DEBUG should be sufficient.
I'd suggest something like this :
Signed-off-by: Jesper Juhl <[email protected]>
--- linux-2.6.12-rc2-mm3-orig/kernel/kprobes.c 2005-04-11 21:20:56.000000000 +0200
+++ linux-2.6.12-rc2-mm3/kernel/kprobes.c 2005-04-27 00:04:23.000000000 +0200
@@ -108,16 +108,24 @@ rm_kprobe:
return ret;
}
-void unregister_kprobe(struct kprobe *p)
+int unregister_kprobe(struct kprobe *p)
{
- unsigned long flags;
- arch_remove_kprobe(p);
+ unsigned long flags = 0;
+ int ret = 0;
+
spin_lock_irqsave(&kprobe_lock, flags);
+ if (!get_kprobe(p)) {
+ ret = -EINVAL;
+ goto out;
+ }
+ arch_remove_kprobe(p);
*p->addr = p->opcode;
hlist_del(&p->hlist);
flush_icache_range((unsigned long) p->addr,
(unsigned long) p->addr + sizeof(kprobe_opcode_t));
+out:
spin_unlock_irqrestore(&kprobe_lock, flags);
+ return ret;
}
And if you really want that printk in there, then this should make you
happy - but personally I don't see the big need :
+ if (!get_kprobe(p)) {
+ printk(KERN_DEBUG "Warning: Attempt to unregister "
+ "unknown kprobe (addr:0x%lx)\n",
+ (unsigned long) p);
+ ret = -EINVAL;
+ goto out;
+ }
Ohh and btw, would you mind posting patches inline? Having to save the
patch, add quotes to it and then read it back into the reply mail to
comment on it is a pain...
And don't trim the CC: list when replying to mails on LKML, please.
--
Jesper Juhl
Le 27/04/05 00:09 +0200, Jesper Juhl ?crivit:
> On Tue, 26 Apr 2005, Frederik Deweerdt wrote:
> Seems to me this will end up calling spin_lock_irqsave() twice, but only
> spin_unlock_irqrestore once in the non-failing case... hmm..
Indeed, I made an obvious mistake there, sorry.
>
> Also, as Chris Wedgwood asked, why not simply return -EINVAL; instead of
> the printk()? Does the user really care that we tried to unregister a
> nonexisting kprobe? and if you really think someone would like to know
> then I'd personally say that KERN_DEBUG should be sufficient.
I wanted to make the patch minimal, but it does make sense.
> I'd suggest something like this :
> [ ... ]
You should also change the prototype in include/kernel/kprobes.h:
--- linux-2.6.12-rc3/include/linux/kprobes.h 2005-03-02 08:37:50.000000000 +0100
+++ linux-2.6.12-rc3-devel/include/linux/kprobes.h 2005-04-27 00:23:09.000000000 +0200
@@ -103,7 +103,7 @@ extern void show_registers(struct pt_reg
struct kprobe *get_kprobe(void *addr);
int register_kprobe(struct kprobe *p);
-void unregister_kprobe(struct kprobe *p);
+int unregister_kprobe(struct kprobe *p);
int setjmp_pre_handler(struct kprobe *, struct pt_regs *);
int longjmp_break_handler(struct kprobe *, struct pt_regs *);
int register_jprobe(struct jprobe *p);
Regards,
Frederik
PS: I've fixed my mailing habits, sorry for inconvenience :)
--
o----------------------------------------------o
| http://open-news.net : l'info alternative |
| Tech - Sciences - Politique - International |
o----------------------------------------------o
Chris,
On Tue, Apr 26, 2005 at 12:48:41PM -0700, Chris Wedgwood wrote:
> On Tue, Apr 26, 2005 at 06:22:03PM +0200, Frederik Deweerdt wrote:
>
> > Here's a patch that handles gracefully attempts to unregister
> > unregistered kprobes (ie. a warning message instead of an oops).
> > Patch is against 2.6.12-rc3
>
> Why not -EINVAL instead of the printk?
This problem has been already fixed, pls see the URL below.
http://lkml.org/lkml/2005/4/11/112
Currently this patch is in -mm tree. But this patch does not have the
prink reporting failure.
Thanks
Prasanna
--
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<[email protected]>