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

[Ananth N Mavinakayanahalli <amavin at redhat dot com>]

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

Hi Jim,

Thanks for the review comments!

> 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 arch_specific_insn on x86_64 is just kprobe_opcode_t *insn. So I
think we should be ok here.

> > +
> > +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.

Hmm, some cases to think of if kmalloc/kcalloc fails in this case:

- if this is the only probe at the address, the registration should
still go on.
- if this is one of many, the registration should fail.

I think this is doable.

> ...
>
> > 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.

Yes, you are right. Guess I can't get dumber :-)

....

> > 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.
>
> ...

Hmm OK. Will have to rearrange the code a bit.

Thanks,
Ananth