Re: [PATCH][RFC] kprobes: Add user entry-handler in kretprobes

It'd be helpful to see others (especially kprobes maintainers) chime in
on this. In particular, if doing kmalloc/kfree of GFP_ATOMIC data at
kretprobe-hit time is OK, as in Abhishek's approach, then we could also
use GFP_ATOMIC (or at least GFP_NOWAIT) allocations to make up the
difference when we run low on kretprobe_instances.

More comments below.

On Fri, 2007-11-16 at 23:20 +0530, Abhishek Sagar wrote:
On Nov 16, 2007 2:46 AM, Jim Keniston <jkenisto@xxxxxxxxxx> wrote:
...

There are three problems in the "data pouch" approach, which is a
generalized case of Srinivasa's timestamp case:

1. It bloats the size of each return instance to a run-time defined
value. I'm certain that quite a few memory starved ARM kprobes users
would certainly be wishing they could install more probes on their
system without taking away too much from the precious memory pools
which can impact their system performance. This is not a deal breaker
though, just an annoyance.

entry_info is, by default, a zero-length array, which adds nothing to
the size of a uretprobe_instance -- at least on the 3 architectures I've
tested on (i386, x86_64, powerpc).

Strange, because from what I could gather, the data pouch patch had
the following in the kretprobe registration routine:


for (i = 0; i < rp->maxactive; i++) {
- inst = kmalloc(sizeof(struct kretprobe_instance), GFP_KERNEL);
+ inst = kmalloc((sizeof(struct kretprobe_instance)
+ + rp->entry_info_sz), GFP_KERNEL);


rp->entry_info_sz is presumably the size of the private data structure
of the registering module.

... which is zero for kretprobes that don't use the data pouch.

This is the bloat I was referring to. But
this difference should've showed up in your tests...?

What bloat? On my 32-bit system, the pouch to hold struct prof_data in
your test_module example would be 20 bytes. (For comparison,
sizeof(struct kretprobe_instance) = 28, btw.) Except for functions like
schedule(), where a lot of tasks can be sleeping at the same time, an
rp->maxactive value of 5 or 10 is typically plenty. That's 100-200
bytes of "bloat" spent at registration time (GFP_KERNEL), at least some
of which will be saved at probe-hit time (GFP_ATOMIC). (And if somebody
says, "I always use a much higher value of rp->maxactive," then he/she's
probably not really worried about bloat.)



2. Forces user entry/return handlers to use ri->entry_info (see
Kevin's patch) even if their design only wanted such private data to
be used in certain instances.

No, it doesn't. Providing a feature isn't the same as forcing people to
use the feature.

From the portion of the patch quoted above, it seems that there is
private data allocation at registration time per-instance in one go.
First of all, not all maxactive instances get used frequently. Even if
they do, the fact that this private data would be used by the user
handlers for a particular instance is also an assumption. So I guess
it is better to leave allocation to the handlers and provide them with
a data pointer in ri.

But as noted in my review of your sample module, hand-crafted, per-hit
allocation is more expensive both in terms of execution time and code
size/complexity.


...

3. It's redundant. 'ri' can uniquely identify any entry-return handler
pair. This itself solves your problem #2. It only moves the onus of
private data allocation to user handlers.

Having ri available at function entry time is definitely a win, but
maintaining separate data structures and using ri to map to the right
one is no trivial task.
...

True, some kind of data pointer/pouch is essential.

Yes. If the pouch idea is too weird, then the data pointer is a good
compromise.

With the above reservations, your enclosed patch looks OK.

You should provide a patch #2 that updates Documentation/kprobes.txt.
Maybe that will yield a little more review from other folks.


I suggest you show us a probe module that captures data at function
entry and reports it upon return, exploiting your proposed patch.
Profiling would be a reasonable example, but something where multiple
values are captured might be more relevant. (Your example below doesn't
count because it doesn't work.) Then I'll code up the same example
using your enhancement + the data pouch enhancement, and we can compare.

I'll send a sample module which uses data pointer provided in ri in my
next response.

Got it. Thanks. I've already commented.

Jim


Of course, the data pouch enhancement would build nicely atop your
enhancement, so it could be proposed and considered separately.

Ok.


Review of Abhishek's patch:
...
Revising the patch with the suggested change:

---
Signed-off-by: Abhishek Sagar <sagar.abhishek@xxxxxxxxx>

diff -upNr linux-2.6.24-rc2/include/linux/kprobes.h
linux-2.6.24-rc2_kp/include/linux/kprobes.h
--- linux-2.6.24-rc2/include/linux/kprobes.h 2007-11-07 03:27:46.000000000 +0530
+++ linux-2.6.24-rc2_kp/include/linux/kprobes.h 2007-11-16
22:50:24.000000000 +0530
@@ -152,6 +152,7 @@ static inline int arch_trampoline_kprobe
struct kretprobe {
struct kprobe kp;
kretprobe_handler_t handler;
+ kretprobe_handler_t entry_handler;
int maxactive;
int nmissed;
struct hlist_head free_instances;
@@ -164,6 +165,7 @@ struct kretprobe_instance {
struct kretprobe *rp;
kprobe_opcode_t *ret_addr;
struct task_struct *task;
+ void *data;
};

struct kretprobe_blackpoint {
diff -upNr linux-2.6.24-rc2/kernel/kprobes.c
linux-2.6.24-rc2_kp/kernel/kprobes.c
--- linux-2.6.24-rc2/kernel/kprobes.c 2007-11-07 03:27:46.000000000 +0530
+++ linux-2.6.24-rc2_kp/kernel/kprobes.c 2007-11-16 22:53:46.000000000 +0530
@@ -699,6 +699,14 @@ static int __kprobes pre_handler_kretpro
struct kretprobe_instance, uflist);
ri->rp = rp;
ri->task = current;
+ ri->data = NULL;
+
+ if (rp->entry_handler) {
+ if (rp->entry_handler(ri, regs)) {
+ spin_unlock_irqrestore(&kretprobe_lock, flags);
+ return 0; /* voluntary miss */
+ }
+ }
arch_prepare_kretprobe(ri, regs);

/* XXX(hch): why is there no hlist_move_head? */

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages