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

[Jim Keniston <jkenisto at us dot ibm dot com>]

On Tue, 2007-05-08 at 20:50 -0400, Ernie Petrides wrote:
> 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.

Yeah, that's an important consideration.

> 
> 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.

Yeah, I'll bite the bullet and make the change from uprobe + uprobe_kimg
(where uprobe_kimg really a probepoint) to uprobe + uprobe_kimg +
uprobe_probept.

> 
> > > 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.

Again, uprobe_kimg and uprobe_task are used both in kernel/urobes.c and
the architecture-specific uprobes.c.  So is uprobe_ssol_slot.  But I
should be able to move some other struct decls to uprobes.c.

...
>
>
> > Thanks very much for your review.
> 
> No problem.  Good luck with this stuff.
> 
> Cheers.  -ernie

Jim