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

[Ernie Petrides <petrides at redhat dot com>]

On Monday, 7-May-2007 at 11:42 PDT, Jim Keniston wrote:

> On Fri, 2007-05-04 at 21:07 -0400, Ernie Petrides wrote:
>
> > [...]
> > 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.
>
> Yeah, you're not the first person to suggest that.  That was the
> original intent of struct uprobe_kimg, but that struct quickly morphed
> into something that would be better called struct uprobe_probepoint
> (which can be associated with multiple uprobes).  uprobe_kimg needs
> to be split into uprobe_kimg + uprobe_probepoint.  The main reason
> I haven't already done it is inertia (e.g., over 200 refs to "uk"
> in kernel/uprobes.c).
>
> I need to reconsider based on your point about binary compatibility.

The ideal scenario would be to have a "user"-interface structure that you
can guarantee will never need to be changed "for all eternity".  Maybe an
easier path forward would be to create a newly named struct for that,
which could contain the opaque pointer to the "uprobe" that you already
have, the latter then becoming an internal-only (hidden) struct.

One thing I should clarify about exported symbol versioning is that every
recursively relevant C identifier depended on by a versioned symbol is used
to generate the checksum suffix.  Thus, if you were to change one little
detail in the "uprobe_kimg", which is pointed to by a "uprobe", which
is used as an arg to register_uprobe(), which is an exported symbol
needed by a 3rd-party uprobe "user" module, then the checksum suffix
on register_uprobe() would change.  And then, the 3rd-party module could
no longer be loaded on a system with the new uprobe infrastructure module.

That's why it's necessary to have an opaque (void *) pointer in the
"user"-interface structure.  Only the code in kernel/uprobes.c would
need to know what the actual internal-only data structure looks like.



> > 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.
>
> arch/*/kernel/uprobes.c typically needs access to certain internal
> data structures (uprobe_kimg, uprobe_task), so I think the best we
> could do for some of those data structures is to move them to a
> separate "implementation" header.  I don't see much precedent for
> that, but I'm open to examples.

Right.  I'm suggesting moving all internal-only-use declarations out of
the one header file and *into* the only C source file that needs them.



> > [...] and that the init_module()
> > and cleanup_module() functions in the documentation example should use
> > __init/__exit.
>
> OK.  There doesn't seem to be anything close to unanimity about this in
> the kernel source, but a slight majority of init_module/cleanup_module
> uses use __init/__exit.  Do you happen to know under what circumstances
> this makes a difference for a module?

Actually, I'm a little confused about this myself.  Maybe the best model
to follow in the example is to use the module_init() and module_exit()
macros (defined in include/linux/init.h) to declare the entry and exit
functions of the example module (which could then use any old names).



> Thanks very much for your review.

No problem.  Good luck with this stuff.

Cheers.  -ernie