Re: [PATCH] kprobe-booster: boosting multi-probe

["bibo,mao" <bibo.mao@intel.com>]

Masami Hiramatsu wrote:
> Hi, bibo
> 
> bibo,mao wrote:
>> Hi Marami,
>> I refresh kprobe boost patch as follows, in this patch post_handler is
>> seperated from break_handler.
> 
> The part of the patch against kernel/kprobes.c seems good to me.
> But I found a bug.
> 
>> @@ -537,6 +539,18 @@ valid_p:
>>          }
>>          arch_remove_kprobe(p);
>>      }
>> +    else{
>> +        mutex_lock(&kprobe_mutex);
>> +        if (p->break_handler)
>> +            old_p->break_handler = NULL;
>> +        if (p->post_handler){
>> +            list_for_each_entry_rcu(list_p, &old_p->list, list){
>> +                if (list_p->post_handler) break;
>> +                old_p->post_handler = NULL;
>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> When the head of the list does not have a post_handler, this code
> always clear the aggr_kprobe's post_handler.
I have modified it.

> And the next part is not comfortable to me.
> 
> 
> I think your patch enables booster even when preemption is
> enabled, and it may be dangerous. Some running processes can
> be preempted by another process when it is executing the codes in
> the instruction buffer. When the boosted-probe is deregistered, that
> instruction buffer is removed. Then, those preempted processes
> can't continue to run, because they can't back to the preempted address.
> So, I think, for the safety, the booster should NOT be enabled when
> preemption is enabled.
> Please fix it.
It actually is one problem, now I fix it. But actually most time kprobe 
happen in preempt enable places, such as system call entry position, 
then booster function will lose its effect.



  arch/i386/kernel/kprobes.c |   14 ++------------
  kernel/kprobes.c           |   34 ++++++++++++++++++++++++++--------
  2 files changed, 28 insertions(+), 20 deletions(-)
--- 2.6.16-rc6-mm1.org/kernel/kprobes.c	2006-03-13 12:25:18.000000000 +0800
+++ 2.6.16-rc6-mm1/kernel/kprobes.c	2006-03-14 08:56:01.000000000 +0800
@@ -368,16 +368,15 @@ static inline void copy_kprobe(struct kp
  */
  static int __kprobes add_new_kprobe(struct kprobe *old_p, struct 
kprobe *p)
  {
-        struct kprobe *kp;
-
  	if (p->break_handler) {
-		list_for_each_entry_rcu(kp, &old_p->list, list) {
-			if (kp->break_handler)
-				return -EEXIST;
-		}
+		if (old_p->break_handler)
+			return -EEXIST;
  		list_add_tail_rcu(&p->list, &old_p->list);
+		old_p->break_handler = aggr_break_handler;
  	} else
  		list_add_rcu(&p->list, &old_p->list);
+	if (p->post_handler && !old_p->post_handler)
+		old_p->post_handler = aggr_post_handler;
  	return 0;
  }

@@ -390,9 +389,12 @@ static inline void add_aggr_kprobe(struc
  	copy_kprobe(p, ap);
  	ap->addr = p->addr;
  	ap->pre_handler = aggr_pre_handler;
-	ap->post_handler = aggr_post_handler;
  	ap->fault_handler = aggr_fault_handler;
-	ap->break_handler = aggr_break_handler;
+	if (p->post_handler)
+		ap->post_handler = aggr_post_handler;
+	if (p->break_handler)
+		ap->break_handler = aggr_break_handler;
+

  	INIT_LIST_HEAD(&ap->list);
  	list_add_rcu(&p->list, &ap->list);
@@ -537,6 +539,22 @@ valid_p:
  		}
  		arch_remove_kprobe(p);
  	}
+	else{
+		mutex_lock(&kprobe_mutex);
+		if (p->break_handler)
+			old_p->break_handler = NULL;
+		if (p->post_handler){
+			list_for_each_entry_rcu(list_p, &old_p->list, list){
+				if (list_p->post_handler){
+					cleanup_p = 2;
+					 break;
+				}
+			}
+			if (cleanup_p == 0)
+				old_p->post_handler = NULL;
+		}
+		mutex_unlock(&kprobe_mutex);
+	}
  }

  static struct notifier_block kprobe_exceptions_nb = {
--- 2.6.16-rc6-mm1.org/arch/i386/kernel/kprobes.c	2006-03-13 
12:41:01.000000000 +0800
+++ 2.6.16-rc6-mm1/arch/i386/kernel/kprobes.c	2006-03-14 
09:27:56.000000000 +0800
@@ -205,9 +205,7 @@ static int __kprobes kprobe_handler(stru
  	int ret = 0;
  	kprobe_opcode_t *addr;
  	struct kprobe_ctlblk *kcb;
-#ifdef CONFIG_PREEMPT
  	unsigned pre_preempt_count = preempt_count();
-#endif /* CONFIG_PREEMPT */

  	addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));

@@ -293,22 +291,14 @@ static int __kprobes kprobe_handler(stru
  		/* handler has already set things up, so skip ss setup */
  		return 1;

-	if (p->ainsn.boostable == 1 &&
-#ifdef CONFIG_PREEMPT
-	    !(pre_preempt_count) && /*
-				       * This enables booster when the direct
-				       * execution path aren't preempted.
-				       */
-#endif /* CONFIG_PREEMPT */
-	    !p->post_handler && !p->break_handler ) {
+ss_probe:
+	if (pre_preempt_count && p->ainsn.boostable == 1 && !p->post_handler) {
  		/* Boost up -- we can execute copied instructions directly */
  		reset_current_kprobe();
  		regs->eip = (unsigned long)p->ainsn.insn;
  		preempt_enable_no_resched();
  		return 1;
  	}
-
-ss_probe:
  	prepare_singlestep(p, regs);
  	kcb->kprobe_status = KPROBE_HIT_SS;
  	return 1;