Re: [RFC] Design + prototype: Multiple kprobes at an address -take 2

[Jim Keniston <jkenisto at us dot ibm dot com>]

On Wed, 2005-04-06 at 07:48, Ananth N Mavinakayanahalli wrote:

I missed take 1.  Sorry if these comments have already been addressed,
but here goes...

...
> +
> +static void copy_kprobe(struct kprobe *src, struct kprobe *dst)
> +{
> +	dst->opcode = src->opcode;
> +	memcpy(&dst->ainsn, &src->ainsn, sizeof(struct arch_specific_insn));
> +}

x86_64 stores the copy of the instruction on a different (executable)
page.  (See arch_prepare_kprobe().)  It's not clear to me that you take
this into account.

> +
> +struct aggr_probe *register_aggr_probe(struct kprobe *old_p)
> +{
> +	struct aggr_probe *ap;
> +
> +	ap = kcalloc(1, sizeof(struct aggr_probe), GFP_ATOMIC);

I understand why you have to use GFP_ATOMIC here (lock held), but
consider that there may be many probepoints where there are multiple
kprobes.  (E.g., Hien is thinking of re-implementing retprobes using
your multiple-kprobes feature, to allow register/unregister of a
retprobe to be independent of the entry probe.  Think about those two
probes at the entry for every system call.)  Anyway, it's probably best
to avoid eating up GFP_ATOMIC memory.

Instead, why not pre-allocate this memory early in register_kprobe(),
before you acquire the lock?  If you don't need it (the usual case), you
can just free it.

...
>  int register_kprobe(struct kprobe *p)
>  {
>  	int ret = 0;
>  	unsigned long flags = 0;
> +	struct aggr_probe *ap;
> +	struct kprobe *old_p;
>  
>  	if ((ret = arch_prepare_kprobe(p)) != 0) {
>  		goto rm_kprobe;
>  	}
> +	
>  	spin_lock_irqsave(&kprobe_lock, flags);
>  	INIT_HLIST_NODE(&p->hlist);
> -	if (get_kprobe(p->addr)) {
> -		ret = -EEXIST;
> +	old_p = get_kprobe(p->addr);
> +	if (old_p) {
> +		if (old_p->break_handler) {

Seems like you could change this to
		if (old_p->break_handler || p->break_handler)
and remove the two checks below.

> +			/* jprobe present, can't register another probe */
> +			ret = -EEXIST;
> +			goto out;
> +		}
> +		if (old_p->pre_handler == aggr_pre_handler) {
> +			/* 
> +			 * Check if registration is for a jprobe
> +			 * Fail if so till we figure out how a jprobe 
> +			 * and a kprobe can coexist at the same addr
> +			 */
> +			if (p->break_handler) {
> +				ret = -EEXIST;
> +				goto out;
> +			}
> +			/* addr is already initialized */
> +			copy_kprobe(old_p, p);
> +			ap = container_of(old_p, struct aggr_probe, kp);
> +			list_add(&p->list, &ap->kprobes);
> +		} else {
> +			/* check for same reason as above */
> +			if (p->break_handler) {
> +				ret = -EEXIST;
> +				goto out;
> +			}
...
>  
>  void unregister_kprobe(struct kprobe *p)
>  {
>  	unsigned long flags;
> -	arch_remove_kprobe(p);
> +	struct kprobe *kp;
> +	struct aggr_probe *ap;
> +	
>  	spin_lock_irqsave(&kprobe_lock, flags);
> +	kp = get_kprobe(p->addr);
> +	if (kp && (kp->pre_handler == aggr_pre_handler)) {
> +
> +		/* this is one of the possibly many kprobes at the address */
> +		ap = container_of(kp, struct aggr_probe, kp);
> +		list_del(&p->list);
> +
> +		/* 
> +		 * if we just unregistered the last probe at the address,
> +		 * its time to cleanup
> +		 */ 
> +		if (list_empty(&ap->kprobes)) {
> +			arch_remove_kprobe(p);

Unfortunately, on x86_64, arch_remove_kprobe() can sleep, so you can't
call it holding a lock.

...

Jim