Messages in this thread | | | Date | Tue, 8 Nov 2011 15:26:42 -0800 (PST) | From | Victor Kamensky <> | Subject | Re: [PATCH 2/4] MIPS Kprobes: Deny probes on ll/sc instructions |
| |
Hi David,
Thank you for your feedback! Please see response inline.
On Tue, 8 Nov 2011, David Daney wrote:
> On 11/08/2011 09:05 AM, Maneesh Soni wrote: > > > > From: Maneesh Soni<manesoni@cisco.com> > > > > Deny probes on ll/sc instructions for MIPS kprobes > > > > As ll/sc instruction are for atomic read-modify-write operations, allowing > > probes on top of these insturctions is a bad idea. > > > > s/insturctions/instructions/ > > Not only is it a bad idea, it will probably make them fail 100% of the time. > > It is also an equally bad idea to place a probe between any LL and SC > instructions. How do you prevent that?
As per below code comment we don't prevent that. There is no way to do that.
> If you cannot prevent probes between LL and SC, why bother with this at all?
We just trying to be a bit practical here. It is better than nothing, right? Breakpoint on top of ll/sc simply won't work and that is the fact. Breakpoint between related pair of ll/sc won't work too, but nothing we can do about that.
We run into this situation with SystemTap function wildcard based tracing, as per attached unit test note. Basically SystemTap wildcard probe picked inline assembler function which had first 'll' instruction and as result it was spinning there till SystemTap module reached threshold and shut itself off so code proceeded after that. Note attached unit test presents simplified version of real issue we run into, so it may look a bit artificial. Note it is highly unlikely that SystemTap wildcard tracing would pick up anything between related pair of ll/sc. In order to have breakpoint between ll/sc user had use 'statement' SystemTap directive and it would be specifically targeting given address and therefore could be removed easily. In case of wildcard tracing there is no easy workaround for user to drop functions that start with 'll'.
Ideally we would want to push this check into SystemTap compile time, along with check for branch delay slot instruction, but currently in SystemTap there is no infrastructure that would check instruction opcode at compile time. I believe that disallowed instruction check in SystemTap compiler is missing for any CPU. Adding it would be small feature that we did not have time to pursue.
Thanks, Victor
> David Daney > > > Signed-off-by: Victor Kamensky<kamensky@cisco.com> > > Signed-off-by: Maneesh Soni<manesoni@cisco.com> > > --- > > arch/mips/kernel/kprobes.c | 31 +++++++++++++++++++++++++++++++ > > 1 files changed, 31 insertions(+), 0 deletions(-) > > > > diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c > > index 9fb1876..0ab1a5f 100644 > > --- a/arch/mips/kernel/kprobes.c > > +++ b/arch/mips/kernel/kprobes.c > > @@ -113,6 +113,30 @@ insn_ok: > > return 0; > > } > > > > +/* > > + * insn_has_ll_or_sc function checks whether instruction is ll or sc > > + * one; putting breakpoint on top of atomic ll/sc pair is bad idea; > > + * so we need to prevent it and refuse kprobes insertion for such > > + * instructions; cannot do much about breakpoint in the middle of > > + * ll/sc pair; it is upto user to avoid those places > > + */ > > +static int __kprobes insn_has_ll_or_sc(union mips_instruction insn) > > +{ > > + int ret = 0; > > + > > + switch (insn.i_format.opcode) { > > + case ll_op: > > + case lld_op: > > + case sc_op: > > + case scd_op: > > + ret = 1; > > + break; > > + default: > > + break; > > + } > > + return ret; > > +} > > + > > int __kprobes arch_prepare_kprobe(struct kprobe *p) > > { > > union mips_instruction insn; > > @@ -121,6 +145,13 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p) > > > > insn = p->addr[0]; > > > > + if (insn_has_ll_or_sc(insn)) { > > + pr_notice("Kprobes for ll and sc instructions are not" > > + "supported\n"); > > + ret = -EINVAL; > > + goto out; > > + } > > + > > if (insn_has_delayslot(insn)) { > > pr_notice("Kprobes for branch and jump instructions are not" > > "supported\n"); > >Kernel module source --------------------
sjc-lds-154$ cat Makefile obj-m := hellom.o
hellom-objs := hello.o hello1.o
sjc-lds-154$ cat hello.c #include <linux/sched.h> #include <linux/pid.h> #include <linux/err.h> #include <linux/module.h> #include <linux/errno.h> #include <linux/kthread.h>
MODULE_DESCRIPTION("simple hello world module"); MODULE_LICENSE("GPL");
static struct task_struct *my_kthread; int count_down = 100;
void print_value (int i) { printk("print_value received %d\n", i); }
unsigned int bar;
void do_atomic_things (unsigned int i, unsigned int *p);
void foo (int mask) { do_atomic_things(1, &bar);
if (mask & 0x1) { print_value(5); } else if (mask & 0x2) { print_value(4); } else if (mask & 0x4) { print_value(3); } else if (mask & (0x8 | 0x10)) { print_value(2); } }
int hellom_start_func1 (int p) { printk("hellom: %s called, p = %d\n", __FUNCTION__, p); foo(p); return p * p; }
int hellom_start_func2 (int p) { printk("hellom: %s called, p = %d\n", __FUNCTION__, p); return hellom_start_func1(p); }
int hellom_start_func3 (int p) { printk("hellom: %s called, p = %d\n", __FUNCTION__, p); return hellom_start_func2(p); }
static int mythread(void *arg) { while (!kthread_should_stop()) { printk("mythread wakeup: count_doun = %d\n", count_down); schedule_timeout_interruptible(3 * HZ); count_down--; hellom_start_func3(count_down); } return 0; }
void create_mythread (void) { my_kthread = kthread_run(mythread, NULL, "hello"); }
static int __init init_hello(void) { printk("hello module loaded\n"); create_mythread(); hellom_start_func3(5); return 0; }
int hellom_end_func1 (int p) { printk("hellom: %s called, p = %d\n", __FUNCTION__, p); return p * p; }
int hellom_end_func2 (int p) { printk("hellom: %s called, p = %d\n", __FUNCTION__, p); return hellom_end_func1(p); }
int hellom_end_func3 (int p) { printk("hellom: %s called, p = %d\n", __FUNCTION__, p); return hellom_end_func2(p); }
static void __exit exit_hello(void) { hellom_end_func3(6); printk("hello module removed\n"); }
module_init(init_hello); module_exit(exit_hello); sjc-lds-154$ cat hello1.c
static __inline__ void my_atomic_sub(unsigned int *p, unsigned int v) { unsigned int temp;
__asm__ __volatile__ ( ".set push\n\t" ".set noreorder\n\t" "1:\tll %0, %3\n\t" /* load old value */ "subu %0, %0, %2\n\t" /* calculate new value */ "sc %0, %1\n\t" /* attempt to store */ "beqz %0, 1b\n\t" /* spin if failed */ "nop\n\t" ".set pop\n\t" : "=&r" (temp), "=m" (*p) : "r" (v), "m" (*p) : "memory"); }
int k1; int k2; int l1; int l2;
void do_atomic_things (unsigned int i, unsigned int *p) { k1 += 1; k2 += 2; my_atomic_sub(p, i); l2 -= 2; l1 -= 3; }
Tracing Script -------------- sjc-lds-154$ cat atomicm_foo.stp probe module("hellom").function("*").call { printf ("%s -> %s\n", thread_indent(1), probefunc()) }
probe module("hellom").function("*").return { printf ("%s <- %s\n", thread_indent(-1), probefunc()) }
probe module("hellom").function("*").inline { printf ("%s => %s\n", thread_indent(1), probefunc()) thread_indent(-1) }
Run logs --------
When script activated without fixes it keeps printing my_atomic_sub forever:
[my6300:~]$ staprun atomicm_foo.ko 0 hello(3807): -> hellom_start_func3 17 hello(3807): -> hellom_start_func2 25 hello(3807): -> hellom_start_func1 33 hello(3807): -> foo 39 hello(3807): -> do_atomic_things 44 hello(3807): => my_atomic_sub 51 hello(3807): => my_atomic_sub 58 hello(3807): => my_atomic_sub 65 hello(3807): => my_atomic_sub 72 hello(3807): => my_atomic_sub 79 hello(3807): => my_atomic_sub 86 hello(3807): => my_atomic_sub 93 hello(3807): => my_atomic_sub 100 hello(3807): => my_atomic_sub 107 hello(3807): => my_atomic_sub 114 hello(3807): => my_atomic_sub 121 hello(3807): => my_atomic_sub 128 hello(3807): => my_atomic_sub ...
After the fix:
[my6300:~]$ staprun atomicm_foo.ko WARNING: probe module("hellom").function("my_atomic_sub@/ws/kamensky-sjc/nova/atomicm/hello1.c:3").inline (address 0xffffffffc0212408) registration error (rc -22) 0 hello(5605): -> hellom_start_func3 24 hello(5605): -> hellom_start_func2 33 hello(5605): -> hellom_start_func1 42 hello(5605): -> foo 48 hello(5605): -> do_atomic_things 54 hello(5605): <- do_atomic_things 60 hello(5605): => print_value 68 hello(5605): => print_value 77 hello(5605): <- foo 82 hello(5605): <- hellom_start_func1 86 hello(5605): <- hellom_start_func2 90 hello(5605): <- hellom_start_func3 0 hello(5605): -> hellom_start_func3 23 hello(5605): -> hellom_start_func2 32 hello(5605): -> hellom_start_func1 41 hello(5605): -> foo 46 hello(5605): -> do_atomic_things 53 hello(5605): <- do_atomic_things 59 hello(5605): => print_value 66 hello(5605): => print_value 75 hello(5605): <- foo 80 hello(5605): <- hellom_start_func1 84 hello(5605): <- hellom_start_func2 89 hello(5605): <- hellom_start_func3
from dmegs
atomicm_foo: systemtap: 1.4/0.152, base: ffffffffc0218000, memory: 23data/28text/58ctx/13net/262alloc kb, probes: 27 Kprobes for ll and sc instructions are not supported
| |