PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Mark Wielaard]

Hi Frank,

On Fri, 2009-03-27 at 15:55 +0000, fche@sourceware.org wrote:
> commit 432f054fc20511d487d18234b6408b5df89a8c4d
> Author: Frank Ch. Eigler <fche@elastic.org>
> Date:   Fri Mar 27 11:54:42 2009 -0400
> 
>     PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data
>     
>     * translate.cxx (dump_unwindsyms): Also emit STT_OBJECT symbols,
>       therefore .data etc. sections into stap-symbols.h.

Allowing STT_OBJECTS also allowed in some odd creatures that don't seem
to be real symbols. In my case in libc.so the following things got
really malformed (negative) addresses leading to the infamous "error:
integer constant is too large for ‘long’ type" in stap-symbols.h:

  { 0xffffffffffeda000, "__evoke_link_warning_fdetach" },
  { 0xffffffffffeda040, "__evoke_link_warning_getwd" },

This is turning in a wake-the-mole game, but I am now also filtering out
symbols with apparently bogus (before base) addresses:

        commit dc31eb39ab70d9d6b81d1ab02fd49795a4d8f2d0
        Author: Mark Wielaard <mjw@redhat.com>
        Date:   Wed Apr 8 12:35:01 2009 +0200
        
        Omit symbols that have suspicious addresses (before base) from
        symbol table.
            
        * translate.cxx (dump_unwindsyms): Filter out sym.st_value <
        base values.

The only "symbols" this gets rid of (in my local libc.so) are the
__evoke_link_warning_* symbols. Which indeed seem like they aren't
actually real but just hints to the linker.

If there is a better way to recognize these (they have a "normal"
st_shndx as far as I can tell) please let me know.

Cheers,

Mark

diff --git a/translate.cxx b/translate.cxx
index c42097b..3442703 100644
--- a/translate.cxx
+++ b/translate.cxx
@@ -4576,10 +4576,13 @@ dump_unwindsyms (Dwfl_Module *m,
           // PC's, so we omit undefined or "fake" absolute addresses.
           // These fake absolute addresses occur in some older i386
           // kernels to indicate they are vDSO symbols, not real
-          // functions in the kernel.
+          // functions in the kernel. We also omit symbols that have
+          // suspicious addresses (before base).
           if ((GELF_ST_TYPE (sym.st_info) == STT_FUNC ||
                GELF_ST_TYPE (sym.st_info) == STT_OBJECT) // PR10000: also need .data
-               && !(sym.st_shndx == SHN_UNDEF || sym.st_shndx == SHN_ABS))
+               && !(sym.st_shndx == SHN_UNDEF
+                   || sym.st_shndx == SHN_ABS
+                   || sym.st_value < base))
             {
               Dwarf_Addr sym_addr = sym.st_value;
               const char *secname = NULL;

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Roland McGrath]

> Allowing STT_OBJECTS also allowed in some odd creatures that don't seem
> to be real symbols. In my case in libc.so the following things got
> really malformed (negative) addresses leading to the infamous "error:
> integer constant is too large for ?long? type" in stap-symbols.h:
> 
>   { 0xffffffffffeda000, "__evoke_link_warning_fdetach" },
>   { 0xffffffffffeda040, "__evoke_link_warning_getwd" },

I don't know where those crazy values come from.  These symbols are local,
have actual value of zero, and are in non-SHF_ALLOC sections.

> If there is a better way to recognize these (they have a "normal"
> st_shndx as far as I can tell) please let me know.

Any symbol in a non-SHF_ALLOC section should be of no concern to you.
All sorts of symbols in non-allocated sections (e.g. in .debug_*
sections) might sometimes be in an unstripped .symtab, not just these
(which are in link-time magic sections).


Thanks,
Roland

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Mark Wielaard]

Hi Roland,

On Wed, 2009-04-08 at 05:40 -0700, Roland McGrath wrote:
> > Allowing STT_OBJECTS also allowed in some odd creatures that don't seem
> > to be real symbols. In my case in libc.so the following things got
> > really malformed (negative) addresses leading to the infamous "error:
> > integer constant is too large for ?long? type" in stap-symbols.h:
> > 
> >   { 0xffffffffffeda000, "__evoke_link_warning_fdetach" },
> >   { 0xffffffffffeda040, "__evoke_link_warning_getwd" },
> 
> I don't know where those crazy values come from.  These symbols are local,
> have actual value of zero, and are in non-SHF_ALLOC sections.

The original values are not all zero (the second one actually is 0x40).
The crazy values come from then transforming the value with
dwfl_module_relocate_address().

> > If there is a better way to recognize these (they have a "normal"
> > st_shndx as far as I can tell) please let me know.
> 
> Any symbol in a non-SHF_ALLOC section should be of no concern to you.
> All sorts of symbols in non-allocated sections (e.g. in .debug_*
> sections) might sometimes be in an unstripped .symtab, not just these
> (which are in link-time magic sections).

Aha. OK. Thanks.

Basically we are just doing the following from a dwfl_getmodules()
callback:

int syments = dwfl_module_getsymtab(m);
for (int i = 0; i < syments; ++i)
  {
      GElf_Sym sym;
      const char *name = dwfl_module_getsym(m, i, &sym, NULL);
      if ((GELF_ST_TYPE (sym.st_info) == STT_FUNC ||
           GELF_ST_TYPE (sym.st_info) == STT_OBJECT)
          && !(sym.st_shndx == SHN_UNDEF
               || sym.st_shndx == SHN_ABS))
        {
           Dwarf_Addr sym_addr = sym.st_value;
           const char *secname;

           int ki = dwfl_module_relocate_address (m, &sym_addr);
           secname = dwfl_module_relocation_info (m, ki, NULL);

          // ...
        }
  }

So, we need to extend the check to include the section flags of the
given sym.st_shndx. Now I see dwfl_module_getsym already (through
__libdwfl_relocate_value) gets the section header and even compares it
to SHF_ALLOC. But I don't see a way for the caller to get at this value
to use in a situation as the above. Do I need to get at the elf file
myself and extract with elf_getscn by hand, or is there something
smarter to do? I seem to remember that calling dwfl_module_getelf() is
considered expensive.

Thanks,

Mark

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Roland McGrath]

> > >   { 0xffffffffffeda000, "__evoke_link_warning_fdetach" },
> > >   { 0xffffffffffeda040, "__evoke_link_warning_getwd" },
> > 
> > I don't know where those crazy values come from.  These symbols are local,
> > have actual value of zero, and are in non-SHF_ALLOC sections.
> 
> The original values are not all zero (the second one actually is 0x40).

Ah, yes.  For some reason the .gnu.warning.getwd section is doubled, with
its first and second parts pointed to by two different local symbols called
__evoke_link_warning_getwd (i.e. must have been from different .o files in
the libc build).  Well, that is neither here nor there.  The st_values are
quite meaningless to your context, being symbols in non-SHF_ALLOC sections.

> The crazy values come from then transforming the value with
> dwfl_module_relocate_address().

I don't follow.  You mean you got some value from dwfl_module_getsym, right?

I think it's giving you st_value + main.bias, basically.  (That's a bug.)
Then this bogus address happens to lie somewhere very early in the text, so
that it's actually an address in the ELF header or something.  Then
dwfl_module_relocate_address tells you what that is relative to the
earliest proper text section (so it can be negative), or something like that.

> Basically we are just doing the following from a dwfl_getmodules()
> callback:
[...]
>       const char *name = dwfl_module_getsym(m, i, &sym, NULL);
s/NULL/&shndx/
>           && !(sym.st_shndx == SHN_UNDEF
>                || sym.st_shndx == SHN_ABS))

You might not want to exclude SHN_ABS, really.  
That is things like _edata and _end.

> So, we need to extend the check to include the section flags of the
> given sym.st_shndx. 

For anal reasons, the &shndx filled by dwfl_module_getsym is what you use
for the actual symbol, not st_shndx--but st_shndx is what you test for the
SHN_* constants.

> I seem to remember that calling dwfl_module_getelf() is considered
> expensive.

Right, because it causes eager relocation of all the sections.  (This was
added for the benefit of parsing the old markers table sections in .ko
files, and I don't think anything actually wants that relocation done now
that stap just uses the Module.markers file.  But now that's the libdwfl API.)

> Do I need to get at the elf file myself and extract with
> elf_getscn by hand, or is there something smarter to do? 

It's worse than that.  In the general case, the symtab might come from
either the main file or the separate debug file.  You have to know which it
is to make sense of a section index.  The indices of the SHF_ALLOC sections
should always match those in the main file, but the main file's symtab
won't have the non-allocated sections' headers at all.  It probably works
to just see the shndx is >= main's e_shnum, but that feels real fragile.

A first thought was to add a variant dwfl_module_getsymscn that yields an
Elf_Scn * instead of the index.  But, for general-purpose use where you
might look at sh_addr, that leaves you still having to figure out what bias
(or relocation) to apply to the sh_addr value.

So a second thought was to make it dwfl_module_getsymshdr instead, just
filling in a GElf_Shdr for you (with sh_addr adjusted).  But that too is
getting rather clunky.

Another thought is to say that dwfl_module_getsym is intended mainly for
symbols that translate to an actual address (i.e. in SHF_ALLOC sections or
SHN_ABS).  Then all one is really looking for is an indicator whether the
symndx you asked about has an st_value meaningful as an address or not.
I've done that on the roland/getsym branch in elfutils git, if you want to
take a look.


Thanks,
Roland

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Mark Wielaard]

Hi Roland,

On Wed, 2009-04-08 at 17:56 -0700, Roland McGrath wrote:
> > The crazy values come from then transforming the value with
> > dwfl_module_relocate_address().
> 
> I don't follow.  You mean you got some value from dwfl_module_getsym, right?

Yes, and the results of that (sym.st_value) put into
dwfl_module_relocate_address. Which is bogus in this case.

> I think it's giving you st_value + main.bias, basically.  (That's a bug.)
> Then this bogus address happens to lie somewhere very early in the text, so
> that it's actually an address in the ELF header or something.  Then
> dwfl_module_relocate_address tells you what that is relative to the
> earliest proper text section (so it can be negative), or something like that.

Yes, that seems precisely what is happening (except that negative
addresses don't exist, so we get something hugely large).

So what we could do at this time is make sure sym.value is between the
reported base of the module and the end of the module as returned by
dwfl_module_info() Then at least we only process "sane values" (even
though they might not be real function or data symbols).

> > Basically we are just doing the following from a dwfl_getmodules()
> > callback:
> [...]
> >       const char *name = dwfl_module_getsym(m, i, &sym, NULL);
> s/NULL/&shndx/
> >           && !(sym.st_shndx == SHN_UNDEF
> >                || sym.st_shndx == SHN_ABS))
> 
> You might not want to exclude SHN_ABS, really.  
> That is things like _edata and _end.

We exclude those since in some kernels on some architectures have vDSO
names marked as SHN_ABS && STT_FUNC (instead of STT_NOTYPE). Maybe we
should move the SHN_ABS check to only the kernel module.

> > So, we need to extend the check to include the section flags of the
> > given sym.st_shndx. 
> 
> For anal reasons, the &shndx filled by dwfl_module_getsym is what you use
> for the actual symbol, not st_shndx--but st_shndx is what you test for the
> SHN_* constants.

ehm, ok, odd.

> Another thought is to say that dwfl_module_getsym is intended mainly for
> symbols that translate to an actual address (i.e. in SHF_ALLOC sections or
> SHN_ABS).  Then all one is really looking for is an indicator whether the
> symndx you asked about has an st_value meaningful as an address or not.
> I've done that on the roland/getsym branch in elfutils git, if you want to
> take a look.

 This works like gelf_getsym except that st_value is always adjusted to
 an absolute value based on the module's location, when the symbol is in
 an SHF_ALLOC section.  If SHNDXP is non-null, it's set with the section
 index (whether from st_shndx or extended index table); in case of a
 symbol in a non-allocated section, *SHNDXP is instead set to -1.  */

Nice, that is the interface I want!
Unfortunately we also have to work with older versions.
But I guess I can already add the check for shndxp != -1 and it will
start working properly with newer versions.

Thanks,

Mark

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Roland McGrath]

> So what we could do at this time is make sure sym.value is between the
> reported base of the module and the end of the module as returned by
> dwfl_module_info() Then at least we only process "sane values" (even
> though they might not be real function or data symbols).

That seems like a reasonable heuristic.

> We exclude those since in some kernels on some architectures have vDSO
> names marked as SHN_ABS && STT_FUNC (instead of STT_NOTYPE). Maybe we
> should move the SHN_ABS check to only the kernel module.

Ah, right.  Or perhaps just use the heuristic above to cover that case.

> > For anal reasons, the &shndx filled by dwfl_module_getsym is what you use
> > for the actual symbol, not st_shndx--but st_shndx is what you test for the
> > SHN_* constants.
> 
> ehm, ok, odd.

SHN_LORESERVE is 0xff00, so to have more than 65279 sections, the indices
have to be treated disjointly from the st_shndx values that might be those.

> Nice, that is the interface I want!

Ok, I'll merge it for 0.141.

> Unfortunately we also have to work with older versions.
> But I guess I can already add the check for shndxp != -1 and it will
> start working properly with newer versions.

Yes.  And you might use an _ELFUTILS_PREREQ check around the heuristic you
apply to filter out the uninteresting symbols.  Or, you could use getelf
and getscn and check for SHF_ALLOC--a real, proper check, not a heuristic
guess.  You'd definitely want that code only under _ELFUTILS_PREREQ, since
it introduces undesireable performance effects.


Thanks,
Roland

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Mark Wielaard]

Hi Roland,

On Mon, 2009-04-13 at 11:32 -0700, Roland McGrath wrote:
> > So what we could do at this time is make sure sym.value is between the
> > reported base of the module and the end of the module as returned by
> > dwfl_module_info() Then at least we only process "sane values" (even
> > though they might not be real function or data symbols).
> 
> That seems like a reasonable heuristic.

Something I thought of later. Does that also hold for kernel modules,
which are ET_REL? If I understood correctly in theory the separate
sections could be allocated completely separately from the module base
and end. Does libdwfl in offline mode guarantee that all ET_REL sections
have a base themselves that falls inside the module start/end address?
Or did I misunderstand how ET_REL section addresses are allocated?

> Or, you could use getelf
> and getscn and check for SHF_ALLOC--a real, proper check, not a heuristic
> guess.  You'd definitely want that code only under _ELFUTILS_PREREQ, since
> it introduces undesireable performance effects.

So, for a different reason, since I needed the address range a ET_REL
section covers for which we build a symbol table, I am now using
dwfl_module_address_section () and then gelf_getshdr () on the returned
Elf_Scn to get at the details of the section. Does this also introduce
undesirable performance effects?

Also, this interface could probably be slightly friendlier.
What I am doing is:
- Loop through symbols with dwfl_module_getsymtab () and
  dwfl_module_getsym() (use 
- Save the original symbol address for later use (see below).
- Transform the symbol address to a relative one with
  dwfl_module_relocate_address().
- Get the section name with dwfl_module_relocation_info(), which also
  can give me the elf section index (same as dwfl_module_getsym()
  already returned).
- Use the saved symbol address to call dwfl_module_address_section () to
  get the Elf_Scn (again the address is relocated and again I get the
  section relocation base).
- Call gelf_getshdr () to get at the section header which contains the
  section size.

Is there a way to shorten this call chain?

Cheers,

Mark

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Roland McGrath]

> Something I thought of later. Does that also hold for kernel modules,
> which are ET_REL? If I understood correctly in theory the separate
> sections could be allocated completely separately from the module base
> and end. 

When the ET_REL module is actually loaded at runtime, any placement of each
section is possible (from the libdwfl perspective, the general case).  In
actual fact, the kernel lays out everything in a module close together.

> Does libdwfl in offline mode guarantee that all ET_REL sections
> have a base themselves that falls inside the module start/end address?

In offline mode, libdwfl lays out the sections all next to each other so
that there is a single contiguous module start..end.

> So, for a different reason, since I needed the address range a ET_REL
> section covers for which we build a symbol table, 

I am not clear on why you are doing this.  Why do you need this range?
I'd expected you'd just use dwfl_module_relocate_address and be done with it.

> I am now using
> dwfl_module_address_section () and then gelf_getshdr () on the returned
> Elf_Scn to get at the details of the section. Does this also introduce
> undesirable performance effects?

Yes, it applies relocs to the section contents before returning.

> Also, this interface could probably be slightly friendlier.

That might not get much better that soon.  My intent is to get a clear
handle on all your usage patterns and make that be easier and more natural
in the eventual proper C++ interfaces.  (Having those gel so stap could
convert to using them is a few months away.)

> What I am doing is:
> - Loop through symbols with dwfl_module_getsymtab () and
>   dwfl_module_getsym() (use 

For what again?  Are you storing the whole symtab for runtime lookups or
something?


Thanks,
Roland

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Mark Wielaard]

Hi Roland,

On Thu, 2009-04-16 at 13:53 -0700, Roland McGrath wrote:
> > Does libdwfl in offline mode guarantee that all ET_REL sections
> > have a base themselves that falls inside the module start/end address?
> 
> In offline mode, libdwfl lays out the sections all next to each other so
> that there is a single contiguous module start..end.

That is a nice feature to have in offline mode.

> > So, for a different reason, since I needed the address range a ET_REL
> > section covers for which we build a symbol table, 
> 
> I am not clear on why you are doing this.  Why do you need this range?
> I'd expected you'd just use dwfl_module_relocate_address and be done with it.
[... rearranged ...]
> > What I am doing is:
> > - Loop through symbols with dwfl_module_getsymtab () and
> >   dwfl_module_getsym() (use 
> 
> For what again?  Are you storing the whole symtab for runtime lookups or
> something?

Yes, we build a table of all function symbols, sorted by (relative)
address, per relocation section, so at runtime we can easily lookup a
symbol name given an address (keep track of where the sections are
placed at runtime, then do a binary search through that table). We also
need the size of the relocation sections to lookup other info for an
address at runtime we associate with a module, like the unwind tables.

> > I am now using
> > dwfl_module_address_section () and then gelf_getshdr () on the returned
> > Elf_Scn to get at the details of the section. Does this also introduce
> > undesirable performance effects?
>
> Yes, it applies relocs to the section contents before returning.

That is a pity, but this is only for a relocation section that contains
an address of a symbol we are interested in (which admittedly is a
larger number now that we also collect non-function symbol addresses).
But I don't see any other way to get the individual relocation section
sizes (in the ET_EXEC and ET_DYN cases it is easy, since then the
relocation section size equals the module size for which we can get the
start and end addresses through dwfl_module_info).

Cheers,

Mark

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Roland McGrath]

> We also need the size of the relocation sections to lookup other info for
> an address at runtime we associate with a module, like the unwind tables.

I don't entirely follow this part.  (You mean "relocated sections", right?
A "relocation section" is the SHT_REL/SHT_RELA section.)

You are packing the unwind info taken from a .ko's .debug_frame for use at
runtime.  All the address literals therein will need to be relocated at run
time.  For each one, you need to emit the symbol+offset or section+offset
information to use at runtime.  dwfl_relocate_address when you pack gives
you section+offset.  i.e., this is the same as loc2c-emitted addresses,
probe addresses, etc.  None of that procedure needs to know the size of a
section, neither at translation time nor at runtime.

So what's the size for?


Thanks,
Roland

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Mark Wielaard]

Hi Roland,

On Fri, 2009-04-17 at 14:17 -0700, Roland McGrath wrote:
> > We also need the size of the relocation sections to lookup other info for
> > an address at runtime we associate with a module, like the unwind tables.
> 
> I don't entirely follow this part.  (You mean "relocated sections", right?
> A "relocation section" is the SHT_REL/SHT_RELA section.)

Yes, I mean the relocated sections.

> You are packing the unwind info taken from a .ko's .debug_frame for use at
> runtime.  All the address literals therein will need to be relocated at run
> time.  For each one, you need to emit the symbol+offset or section+offset
> information to use at runtime.  dwfl_relocate_address when you pack gives
> you section+offset.  i.e., this is the same as loc2c-emitted addresses,
> probe addresses, etc.  None of that procedure needs to know the size of a
> section, neither at translation time nor at runtime.

None of this needs the size of the section indeed. Although I have to
check we do the right thing at runtime now that you described it again.

> So what's the size for?

So at runtime we have an address and we want to know which
module/section it is part of so we can lookup the correct symbol or
unwind table we need to use. We do that by looking through our
module/section list and check that the address we have falls between the
start of the module/section (at runtime) and the end (for which we need
to know the size of the section).

Cheers,

Mark

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Roland McGrath]

> So at runtime we have an address and we want to know which
> module/section it is part of so we can lookup the correct symbol or
> unwind table we need to use. We do that by looking through our
> module/section list and check that the address we have falls between the
> start of the module/section (at runtime) and the end (for which we need
> to know the size of the section).

You need to know the runtime set of addresses that fall within each module.
For that you don't need section sizes per se.  You need an address->module
mapping at runtime that works for what runtime modules really are.

For non-ET_REL, modules are contiguous.  (Even if we really supported
discontiguous modules, you just need to know the PT_LOAD set, which is easy
to find at runtime and was so found already in doing the loading.  i.e.,
it is the same as the file mapping tracking you used to decide this was the
module to probe, etc.)

For ET_REL, there isn't a universal meaningful kind of thing, so we are
just talking about the specifics of the runtime ET_REL loader you are
working with, i.e. Linux kernel modules.  Linux kernel modules are loaded
in two chunks, whose bounds are handy in the 'struct module': one at
module_core for core_size bytes; the other at module_init for init_size
bytes (which are cleared after the load-time initializer has run).


Thanks,
Roland

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Mark Wielaard]

Hi Roland,

On Fri, 2009-04-17 at 15:27 -0700, Roland McGrath wrote:
> You need to know the runtime set of addresses that fall within each module.
> For that you don't need section sizes per se.  You need an address->module
> mapping at runtime that works for what runtime modules really are.

Yes.

> For non-ET_REL, modules are contiguous.  (Even if we really supported
> discontiguous modules, you just need to know the PT_LOAD set, which is easy
> to find at runtime and was so found already in doing the loading.  i.e.,
> it is the same as the file mapping tracking you used to decide this was the
> module to probe, etc.)

Using the tracker is a nice trick. But in that case we already know the
size since dwfl_module_info() gives us start and end.

> For ET_REL, there isn't a universal meaningful kind of thing, so we are
> just talking about the specifics of the runtime ET_REL loader you are
> working with, i.e. Linux kernel modules.  Linux kernel modules are loaded
> in two chunks, whose bounds are handy in the 'struct module': one at
> module_core for core_size bytes; the other at module_init for init_size
> bytes (which are cleared after the load-time initializer has run).

OK, that is simpler than I thought. I had expected the sections to be
split up more (data read-only/write vs code, etc sections).
Unfortunately we don't seem to have access to the struct module data
since we are a kernel module and the interfaces aren't exported. So we
use the /sys/module/<modname>/sections/<secname> files, which only gives
us the section start addresses.

This is the documentation of runtime/staprun/staprun.c
(send_a_relocation):

/* Send a variety of relocation-related data to the kernel: for the
   kernel proper, just the "_stext" symbol address; for all loaded
   modules, a variety of symbol base addresses.

   We do this under protest.  The kernel ought expose this data to
   modules such as ourselves, but instead the upstream community
   continually shrinks its module-facing interfaces, including this
   stuff, even when users exist.
*/

Cheers,

Mark

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-0.9-238-g432f054)

[Roland McGrath]

> OK, that is simpler than I thought. I had expected the sections to be
> split up more (data read-only/write vs code, etc sections).
> Unfortunately we don't seem to have access to the struct module data
> since we are a kernel module and the interfaces aren't exported.

struct module is defined in <linux/module.h>.  __module_address is
exported.  That does address->module mapping, which is the part of the
problem we are talking about.

Also, find_module is exported, for looking up by name.
Also, the struct module * is passed to register_module_notifier callbacks.
So you have it right at hand when tracking a module being loaded/unloaded.

Every module can access its own struct module * with THIS_MODULE.
So a stap-generated module can do this.  You can start with your own,
and iterate over the list that links them all together, even though
the head of the list is private to module.c ("static LIST_HEAD(modules);").

struct module_sec_attrs is indeed private to module.c so that you can't get
the section info easily inside the module, and have to get that via /sys.


Thanks,
Roland

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data

[Frank Ch. Eigler]

Roland McGrath <roland@redhat.com> writes:

>>[...]
> Also, find_module is exported, for looking up by name.

... but that is designed to run with module_mutex held, which was only
only recently module-exported, so using it generally incurs a race
condition.  (Many moons ago we requested a get_module_byname() to
solve this but lkml people said no.)

> Every module can access its own struct module * with THIS_MODULE.
> So a stap-generated module can do this.  You can start with your
> own, and iterate over the list that links them all together [...]

I seem to recall some other problem (portability? locking?) with this
approach.

- FChE

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data

[Roland McGrath]

> ... but that is designed to run with module_mutex held, which was only
> only recently module-exported, so using it generally incurs a race
> condition.

If "generally" means on older kernels, sure.  This stuff will always be a
moving target.

> > Every module can access its own struct module * with THIS_MODULE.
> > So a stap-generated module can do this.  You can start with your
> > own, and iterate over the list that links them all together [...]
> 
> I seem to recall some other problem (portability? locking?) with this
> approach.

You do have to worry about locking for that as for find_module et al, sure.
There are some unlocked uses, however.  So you should be safe if you follow
those models.  Those use list_for_each_entry_rcu with preempt disabled,
e.g. __module_address.

There is also the /proc/modules list, which shows ->module_core and ->core_size.

There is also /sys/module/sections/.gnu.linkonce.this_module, which has the
kernel address of the magic section that maps to THIS_MODULE.

There are many ways to skin this cat at run-time (in kernel or in staprun).
I don't think that determining individual section sizes is necessary for
any good choice.


Thanks,
Roland

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data

[Mark Wielaard]

Hi Roland,

On Mon, 2009-04-20 at 12:13 -0700, Roland McGrath wrote:
> I don't think that determining individual section sizes is necessary for
> any good choice.

Why do you think that? We need the size (start/end) of the mapped
section at runtime to know if an address falls inside it or not.
What am I missing?

Thanks,

Mark

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data

[Roland McGrath]

> On Mon, 2009-04-20 at 12:13 -0700, Roland McGrath wrote:
> > I don't think that determining individual section sizes is necessary for
> > any good choice.
> 
> Why do you think that? We need the size (start/end) of the mapped
> section at runtime to know if an address falls inside it or not.
> What am I missing?

I guess you're missing that I said "individual section".  I am using
"section" in the technical ELF sense.  As we've been saying, the mapped
regions of a kernel module are easily available at runtime.  There are
either one or two of these total.

In normal ELF parlance, we'd call these the "segments".  In normal ELF
parlance, the mapping of sections to segments would have been assumed to
take place statically so that each "segment" is described by a PT_LOAD
header.  In the kernel module case, all that is really different is that
this mapping is chosen at load time.  By actual "run time", i.e. after the
physical loading, there are two "segments" (or only one, if no initializer
code--and always only one actually left in memory after initializers run).


Thanks,
Roland

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data

[Mark Wielaard]

Hi Roland,

On Tue, 2009-04-21 at 13:55 -0700, Roland McGrath wrote:
> > On Mon, 2009-04-20 at 12:13 -0700, Roland McGrath wrote:
> > > I don't think that determining individual section sizes is necessary for
> > > any good choice.
> > 
> > Why do you think that? We need the size (start/end) of the mapped
> > section at runtime to know if an address falls inside it or not.
> > What am I missing?
> 
> I guess you're missing that I said "individual section".  I am using
> "section" in the technical ELF sense.  As we've been saying, the mapped
> regions of a kernel module are easily available at runtime.  There are
> either one or two of these total.
> 
> In normal ELF parlance, we'd call these the "segments".  In normal ELF
> parlance, the mapping of sections to segments would have been assumed to
> take place statically so that each "segment" is described by a PT_LOAD
> header.  In the kernel module case, all that is really different is that
> this mapping is chosen at load time.  By actual "run time", i.e. after the
> physical loading, there are two "segments" (or only one, if no initializer
> code--and always only one actually left in memory after initializers run).

OK, I see. Now reading back in the thread I think what you are saying is
to use /proc/modules instead of /sys/module/sections since /proc/modules
provides segment size and start address during runtime of the ET_REL
kernel module. Then we can treat these just like ET_DYN shared
libraries. The only trick would be to track the init segments (assuming
we support probing those at the moment). Is it possible to find out
during offline mode which sections will go into which kernel segment
(core/init)? That would probably simplify some of our code.

Thanks,

Mark

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data

[Roland McGrath]

> OK, I see. Now reading back in the thread I think what you are saying is
> to use /proc/modules instead of /sys/module/sections since /proc/modules

I tried to cite all the places you could get the runtime module segment
bounds.  I'm not saying exactly which of those to use.  

I am saying that runtime module segment bounds is all you need and that you
should not be trying to do anything for which you would care about
individual sections' bounds.

> Then we can treat these just like ET_DYN shared libraries.

Almost.  The kernel modules have two "floating" segments (core and init),
i.e. the offset between core and init addresses is not static.  So there
are two separate contiguous chunks to consider at runtime.

In ET_DYN objects, there might be several segments in theory (not just
magically two), but there is a static offset between them.  So there is
only one contiguous address range to consider.

> The only trick would be to track the init segments (assuming we support
> probing those at the moment).

I don't know what the status of init probes is.  The init segment exists in
memory only during the window where the initializer code runs.  i.e.,
between MODULE_STATE_COMING notifier and MODULE_STATE_LIVE notifier
(or MODULE_STATE_GOING instead if the initializer fails).

Just using the notifier, it would be simple enough to track which modules
are in the middle of init and hold their struct module pointers.  (There
won't often be more than one at a time, but there could be.)  From there,
the ->module_init and ->init_size fields are handy.

> Is it possible to find out during offline mode which sections will go
> into which kernel segment (core/init)? That would probably simplify some
> of our code.

There is nothing that libdwfl or anything generic to ELF stuff knows about
this.  So again you would just encode specific knowledge about the Linux
kernel module loader in code specific to Linux kernel modules.  It just
segregates them by which sections have names beginning with ".init".

But I don't see why you need to know that at all.  At runtime, you need to
know which module a PC belongs to, end of story.

At stap module load time, you need to apply the correct adjustment to each
address value, no different from probe PCs.  At translation time, you need
to know what to tell that load time logic to do, i.e. the libdwfl
relocation base info, no different from probe PCs.

The only reason I can see for knowing whether an address is in the init
segment is for probe insertion.  There you need to know which probe
addresses can no longer be touched after the module initializer has run.

I guess for unwind data you could segregate your data by segment, and then
free the data about init-segment addresses when the init segment itself has
been freed.  But I am guessing you do not pick apart and re-pack the unwind
data at translation time enough to do that.


Thanks,
Roland

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data

[Ananth N Mavinakayanahalli]

On Wed, Apr 22, 2009 at 01:07:57PM -0700, Roland McGrath wrote:

...
 
> > The only trick would be to track the init segments (assuming we support
> > probing those at the moment).
> 
> I don't know what the status of init probes is.  The init segment exists in
> memory only during the window where the initializer code runs.  i.e.,
> between MODULE_STATE_COMING notifier and MODULE_STATE_LIVE notifier
> (or MODULE_STATE_GOING instead if the initializer fails).
> 
> Just using the notifier, it would be simple enough to track which modules
> are in the middle of init and hold their struct module pointers.  (There
> won't often be more than one at a time, but there could be.)  From there,
> the ->module_init and ->init_size fields are handy.

Kprobes already has the intelligence to handle probes in module
init/exit sections. Masami's work recently on kprobes_module_callback
added this feature.

Ananth

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data

[Mark Wielaard]

On Wed, 2009-04-22 at 13:07 -0700, Roland McGrath wrote:
> I am saying that runtime module segment bounds is all you need and that you
> should not be trying to do anything for which you would care about
> individual sections' bounds.

OK, we will figure something out to get those. One way or another.

> > Then we can treat these just like ET_DYN shared libraries.
> 
> Almost.  The kernel modules have two "floating" segments (core and init),
> i.e. the offset between core and init addresses is not static.  So there
> are two separate contiguous chunks to consider at runtime.
> 
> In ET_DYN objects, there might be several segments in theory (not just
> magically two), but there is a static offset between them.  So there is
> only one contiguous address range to consider.

Right. For our purposes, there is just one giant segment for a mapped in
ET_DYN object, that might span several sub-segments, but together those
cover one continuous address space, for which we carry one symbol and
unwind table.

> > Is it possible to find out during offline mode which sections will go
> > into which kernel segment (core/init)? That would probably simplify some
> > of our code.
> 
> There is nothing that libdwfl or anything generic to ELF stuff knows about
> this.  So again you would just encode specific knowledge about the Linux
> kernel module loader in code specific to Linux kernel modules.  It just
> segregates them by which sections have names beginning with ".init".
> 
> But I don't see why you need to know that at all.  At runtime, you need to
> know which module a PC belongs to, end of story.
> 
> At stap module load time, you need to apply the correct adjustment to each
> address value, no different from probe PCs.  At translation time, you need
> to know what to tell that load time logic to do, i.e. the libdwfl
> relocation base info, no different from probe PCs.

OK, it seems I have to study how the probe insertion code tracks these
addresses then.

The main problem I have is that we keep symbol data for functions and
data per "relocation section". So for ET_EXEC and ET_DYN that is just
one table with addresses relative to the load address. We just need to
keep track of where these are loaded at runtime (and there we can easily
get the size of the segment mapped in). With ET_REL we have multiple
"relocation sections", so we have multiple symbol tables, one for each
index as given by dwfl_module_relocate_address (or more accurately per
section name returned by dwfl_module_relocation_info() for that index),
up to dwfl_module_relocations(). All these tables are relative to the
base address those sections will be mapped to in memory. So if we aren't
going to track where these sections are mapped into kernel memory, we we
need to figure out how these sections map into the kernel segments for
the module at runtime, and/or we need to keep the tables relative to the
segment start. I am still slightly confused how the "relocation
sections" map to the "runtime segments" when mapped into the kernel at
runtime.

Cheers,

Mark

Re: PR10000: emit _stp_relocate* calculations correctly for kernel/module global $data

[Roland McGrath]

> So if we aren't going to track where these sections are mapped into
> kernel memory, 

This is what you are already doing to compute probe locations.
I imagine you must use /sys/module/foo/sections/bar.
The address of each section is easy.
It's the size of the section that you don't need.

Once again, you need to do two things:

1. adjust used relocation bases at load time
   You already do exactly this for probe insertion in modules.
   It requires the runtime address of each section of interest,
   which is readily available at runtime and already sampled by staprun.
   All you need is to reuse the same machinery.

2. map PC to module at runtime: segment bounds from struct module have it
   (You can even get the core segment's bounds from /proc/modules
   in userland, if you are only handling post-init.)

These two things are not really related and what you need to do the one
does not equate to what you need to do the other.  Where's the mystery?


Thanks,
Roland