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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Maneesh Soni<manesoni@xxxxxxxxx>
> > ---
> > 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
