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

[Ananth N Mavinakayanahalli]

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