Re: [PATCH 2/4] MIPS Kprobes: Deny probes on ll/sc instructions

[Victor Kamensky]

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