Re: [RFC][PATCH 1/2] uprobes: utrace-based user-space probes

[Ernie Petrides <petrides@redhat.com>]

On Friday, 20-Apr-2007 at 15:08 PDT, Jim Keniston wrote:

> Here's the latest rev of our i386 implementation of user-space
> probes (uprobes), based on Roland's utrace infrastructure.  Uprobes
> supplements utrace and kprobes, enabling a kernel module to probe
> user-space applications in much the same way that a kprobes-based
> module probes the kernel.  See Documentation/uprobes.txt in the patch
> for details.
>
> This patch should apply to any recent -mm kernel (i.e., any kernel
> that includes utrace).  Comments welcome.


Hello, Jim.  I've been asked to look over your uprobes patches by
Frank and Linda.  While I don't have the detailed arch-specific or
utrace background for a thorough review, I do have a couple of higher
level comments.

Firstly, you have my compliments on a great idea and sound implementation.
The value of this as a fantastic debugging facility (with systemtap) is
obvious, but I also envision this getting used for applying temporary
fixes or work-arounds to applications in the field.

Because of this latter possibility, consider that a 3rd-party uprobe
module has been deployed at a customer site, but then a later update
to the uprobe infrastructure becomes desirable.  It's possible that a
change to the "uprobe" structure would be needed for whatever reason,
but that could break compatibility with the 3rd-party module (i.e.,
the uprobes facility consumer).

Thus, I'd recommend finding a way to isolate the consumer/infrastructure
interface from the uprobes-internal data.  The best way would be to make
the "uprobe" contain only the "pid", "vaddr", and "handler" fields and
then add an opaque (void *) to point to a 2nd internal-use-only struct.
Alternatives would be to put a flags field, version number, or struct
size field at the beginning of the struct, or else to require a dynamic
allocation of the struct.

Secondly, I'd recommend that everything except the consumer/infrastructure
interface be moved from include/linux/uprobes.h to kernel/uprobes.c in
order to prevent a consumer module from using/modifying/depending on
anything that it shouldn't.  This is simply basic "information hiding"
to prevent future incompatibility issues.

Lastly, I think that the register_uprobe() and unregister_uprobe()
declarations in uprobes.h should use "extern" and that the init_module()
and cleanup_module() functions in the documentation example should use
__init/__exit.

I'll reply to your 2nd patch with one final comment.

(Note that I'm not on the systemtap@sources.redhat.com list.)

Cheers.  -ernie