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

[Masami Hiramatsu <hiramatu@sdl.hitachi.co.jp>]

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.

> +            }
> +        }
> +        mutex_unlock(&kprobe_mutex);
> +    }
>  }


And the next part is not comfortable to me.

> And I think booster can be enabled even
> when preempt is disabled.

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.


-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp