Re: [RFC] Multiple kprobes at an address redux (take3)

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

Suparna Bhattacharya wrote:

Hello Suparna,

> It would be nice to summarize all the requirements upfront, before
> moving on to the design.
>
> One observation I have is that a register_single_kprobe equivalent 
> is still useful for those who might be setting up a kprobe for debugging
> or fault injection purposes --  or rather maybe something like
> register_exclusive_kprobe, because one might not want to be surprised by
> having multiple handlers clicking away in such a situation.

Yes, this is something we have to be congnizant of. So, here is what I
propose:

We have a "coexist" field in struct kprobe which is default 0. This
means that the kprobe cannot tolerate other kprobes at the same address.
A non zero value would indicate the kprobe can be one of many at the
address.

Then:

- In case we get a registration request for a location that already has
a kprobe with "coexist" = 0, we fail the new one with -EUSERS (or any
other appropriate error code).
- In case we already have a kprobe at the location with "coexist" = 1
and the new request is with "coexist" = 0, we fail that too. (basically
a sort of first-come-first-serve algorithm).
- In case we have a kprobe with "coexist" = 1 and the new one is also
with "coexist" = 1, then we go ahead and register the probe.

I think with this solution, we can safely say that people who would
not want to be surprised by multiple handlers can still continue using
kprobes as they currently do. Only those who are aware of and would not
mind multiple probes would then set the "coexist" flag to a non-zero
value. I also think that this will then make the requirement of a new
set of interfaces moot.

Attached is a patch that applies on top of the one I posted yesterday
and provides this functionality.

> That brings me to the other question -- typically whenever we have
> a call-chain of this sort, policy isn't very far behind ... quite
> soon we find ourselves wanting priorities between handlers or
> to decide whether to continue calling rest of the handlers or
> short circuit through depending on the result of an earlier handler
> etc. Sort of like notifiers ...

If I understand correctly, the (current) users that require multiple
kprobes at an address are not particular about the order of invocation
of the handlers. IMHO, we should not be supporting priorities wrt
handlers when multiple kprobes are registered.

As for short-circuiting handlers, that is a tricky one. We wouldn't want
to do that until there is a safe way to co-ordinate the pre and post
handler runs, ie., in case we have 3 kprobes at an address, we wouldn't
want all three post_handlers to run if the first pre_handler would have
returned a code that would've prevented other pre_handlers from running.

> Perhaps that isn't something that would be very relevant for
> probes which are just used for tracing purposes by various subsystems,
> which is where I guess the multiple handler requirement arose.
>
> But in the more general case, is the design flexible enough to
> allow policy to be built _over_ this infrastructure (again of course
> without cluttering the core kprobes logic) ?
>
> Nomenclature wise - both aggr_probe and multi-probe are names that
> sound more like a set of different probes, not multiple handlers
> for the same probe as is really intended. Ideas for better names ?

"manager kprobe"? Don't know what would be a good name, really :)

Thanks,
Ananth

This patch applies on the "take3" patch

--- temp/linux-2.6.12-rc2/kernel/kprobes.c	2005-04-12 14:27:00.000000000 -0400
+++ linux-2.6.12-rc2/kernel/kprobes.c	2005-04-12 14:15:58.000000000 -0400
@@ -121,14 +121,18 @@
 {
 	int ret = 0;
 	unsigned long flags = 0;
+	struct kprobe *kp;
 
 	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;
+	kp = get_kprobe(p->addr);
+	if (kp) {
+		ret = -EUSERS;
+		if (kp->coexist)
+			ret = -EEXIST;
 		goto out;
 	}
 	arch_copy_kprobe(p);
@@ -143,7 +147,7 @@
 out:
 	spin_unlock_irqrestore(&kprobe_lock, flags);
 rm_kprobe:
-	if (ret == -EEXIST)
+	if (ret == -EEXIST || ret == -EUSERS)
 		arch_remove_kprobe(p);
 	return ret;
 }
@@ -170,6 +174,7 @@
 	ap->pre_handler = aggr_pre_handler;
 	ap->post_handler = aggr_post_handler;
 	ap->fault_handler = aggr_fault_handler;
+	ap->coexist = 1;
 
 	INIT_LIST_HEAD(&ap->list);
 	list_add(&p->list, &ap->list);
@@ -252,7 +257,7 @@
 	int ret = 0;
 
 	ret = register_kprobe_single(p);
-	if (ret == -EEXIST) 
+	if ((ret == -EEXIST) && (p->coexist)) 
 		ret = register_aggr_kprobe(p);
 
 	return ret;
--- temp/linux-2.6.12-rc2/include/linux/kprobes.h	2005-04-12 14:27:00.000000000 -0400
+++ linux-2.6.12-rc2/include/linux/kprobes.h	2005-04-12 12:02:19.000000000 -0400
@@ -46,6 +46,12 @@
 	/* list of kprobes for multi-handler support */
 	struct list_head list;
 
+	/*
+	 * set this to a non-zero value if the kprobe can coexist with
+	 * other kprobes at the same address. Default 0 (can't coexist)
+	 */
+	unsigned int coexist;
+
 	/* location of the probe point */
 	kprobe_opcode_t *addr;