Re: pre-compiled modules

Re: pre-compiled modules

[Frank Ch. Eigler]

Hi -

dsmith wrote:

> I've been looking at pre-compiled modules, and in general they should
> work.  I do have a few questions:
> 
> 1) One thing I've been worried about is differing options between
> compile time and run time.  I believe that most of them should be OK,
> with the exception of the '-b' relayfs option.  If it was specified at
> compile time and not at run time (or vice versa), I'm not sure what will
> happen (since that flag appears to change behavior both at compile time
> and run time).
> 
> Got any ideas here on how to prevent this?

One possibility would be to remove the run-time meaning of -b: don't
pass it to stpd any more.  Perhaps the value could flow by/within the
module to the runtime to stpd at run time.  Perhaps the
stp_check/mount check could be deferred until late in the module
initialization sequence.  Martin?


> 2) Pre-compiled systemtap modules work, but there are a couple of things
> we could/should theoretically check for:
> 
> 2a) Correct kernel version.  Right now there is nothing to prevent a
> user from compiling a systemtap script into a module against kernel X,
> rebooting his system, then trying to run the module against kernel Y.
> 
> From what I've been looking at, there really isn't a good way to check
> this, except for calling "modprobe --dry-run".
> 
> Do you know of a better way?  Do you think this check is necessary?

Blatant modversions mismatches will be detected during the actual
insertion attempt - IMO there is no need to make a dry run first.  We
will need more self-protective code though for purposes of verifying
module addresses.  Specifically, we need to find a kernel API routine
that, given a module name, gives its loaded base address.


> 2b) Actually a systemtap module.  Currently there is nothing preventing
> a user from telling systemtap to run a pre-compiled module that is a
> valid kernel module, but isn't a systemtap module.  I'm unsure of what
> might happen here.
> 
> A way to check for this would to somehow check for the existence of a
> systemtap specific symbol, such as systemtap_module_init, before running
> the module.
> 
> Got any ideas on a good way of doing that?  Is it necessary?

At this point, only a privileged user can run stap, so the risks are
already unlimited in a sense, though mitigated by the fact that
harmful loadable .ko's are rarely found lying idly around.


- FChE

Re: pre-compiled modules

[Hien Nguyen]

How about save the meta data (stap compile options, kernel version, 
location for .ko etc...) at compile time then load the pre-compiled 
module via 'stap metadata.file'

Hien.

Frank Ch. Eigler wrote:
> Hi -
>
> dsmith wrote:
>
>   
>> I've been looking at pre-compiled modules, and in general they should
>> work.  I do have a few questions:
>>
>> 1) One thing I've been worried about is differing options between
>> compile time and run time.  I believe that most of them should be OK,
>> with the exception of the '-b' relayfs option.  If it was specified at
>> compile time and not at run time (or vice versa), I'm not sure what will
>> happen (since that flag appears to change behavior both at compile time
>> and run time).
>>
>> Got any ideas here on how to prevent this?
>>     
>
> One possibility would be to remove the run-time meaning of -b: don't
> pass it to stpd any more.  Perhaps the value could flow by/within the
> module to the runtime to stpd at run time.  Perhaps the
> stp_check/mount check could be deferred until late in the module
> initialization sequence.  Martin?
>
>
>   
>> 2) Pre-compiled systemtap modules work, but there are a couple of things
>> we could/should theoretically check for:
>>
>> 2a) Correct kernel version.  Right now there is nothing to prevent a
>> user from compiling a systemtap script into a module against kernel X,
>> rebooting his system, then trying to run the module against kernel Y.
>>
>> From what I've been looking at, there really isn't a good way to check
>> this, except for calling "modprobe --dry-run".
>>
>> Do you know of a better way?  Do you think this check is necessary?
>>     
>
> Blatant modversions mismatches will be detected during the actual
> insertion attempt - IMO there is no need to make a dry run first.  We
> will need more self-protective code though for purposes of verifying
> module addresses.  Specifically, we need to find a kernel API routine
> that, given a module name, gives its loaded base address.
>
>
>   
>> 2b) Actually a systemtap module.  Currently there is nothing preventing
>> a user from telling systemtap to run a pre-compiled module that is a
>> valid kernel module, but isn't a systemtap module.  I'm unsure of what
>> might happen here.
>>
>> A way to check for this would to somehow check for the existence of a
>> systemtap specific symbol, such as systemtap_module_init, before running
>> the module.
>>
>> Got any ideas on a good way of doing that?  Is it necessary?
>>     
>
> At this point, only a privileged user can run stap, so the risks are
> already unlimited in a sense, though mitigated by the fact that
> harmful loadable .ko's are rarely found lying idly around.
>
>
> - FChE
>   

Re: pre-compiled modules

[Martin Hunt]

On Mon, 2006-06-19 at 12:40 -0400, Frank Ch. Eigler wrote:
> Hi -
> 
> dsmith wrote:
> 
> > I've been looking at pre-compiled modules, and in general they should
> > work.  I do have a few questions:
> > 
> > 1) One thing I've been worried about is differing options between
> > compile time and run time.  I believe that most of them should be OK,
> > with the exception of the '-b' relayfs option.  If it was specified at
> > compile time and not at run time (or vice versa), I'm not sure what will
> > happen (since that flag appears to change behavior both at compile time
> > and run time).
> > 
> > Got any ideas here on how to prevent this?

I designed stpd from the beginning to get all its parameters from the
module it is loading. Some, like buffer size, can be overridden at the
command line. However you cannot force the transport; it will use
whatever the module uses.

 
> One possibility would be to remove the run-time meaning of -b: don't
> pass it to stpd any more.  

Stpd doesn't get "-b".  It does recognize "-r", which is a hack. Without
it, stpd runs "stp_check" which used to do all kinds of things including
compiling and loading relayfs if necessary. Now it just
mounts /mnt/relay.  

Anyway, "-r" can probably be safely eliminated. I will make a note to
try it ASAP. 


> Perhaps the value could flow by/within the
> module to the runtime to stpd at run time.  

It does.

> Perhaps the
> stp_check/mount check could be deferred until late in the module
> initialization sequence.  

Probably. That's what I plan to try.

Martin

Re: pre-compiled modules

[David Smith]

[-- Attachment #1: Type: text/plain, Size: 1875 bytes --]

See stuff below.

On Mon, 2006-06-19 at 12:40 -0400, Frank Ch. Eigler wrote:
> Hi -
> 
> dsmith wrote:
> 
> > I've been looking at pre-compiled modules, and in general they should
> > work.  I do have a few questions:
> > 

... stuff deleted ...


> Blatant modversions mismatches will be detected during the actual
> insertion attempt - IMO there is no need to make a dry run first.  We
> will need more self-protective code though for purposes of verifying
> module addresses.  Specifically, we need to find a kernel API routine
> that, given a module name, gives its loaded base address.

Hmm, I hadn't considered that.  Will this code go in the module itself
or go in the runtime?


To help move this along a bit, I've attached a patch that modifies the
systemtap front end to take 2 new options:

   -S         Save the compiled module in the current directory
   -P PRE_COMPILED_MODULE
              Run pre-compiled module

Note that the patch was generated with diff ignoring whitespace changes,
since a large part of main.cxx was re-indented so that passes 1-4 will
get skipped when '-P' is specified.

Here's how it works.  The '-S' option isn't strictly necessary since you
can accomplish the same thing with '-k' then a manual copy, but it is
nice.  To save a module, you do the following:

  # stap -S foo.stp 
  ... output from running stap_27691.ko ...
  Module saved as stap_27691.ko

It makes more sense to name the module something reasonable (and to not
run the module, just compile it).  So, you would do the following:

  # stap -m foo -p4 -S foo.stp
  Module saved as foo.ko

Now that you've got a module, you can run it like this:

  # stap -P foo.ko
  ... output from foo.ko ...

Comments on the option names and code itself appreciated.

-- 
David Smith
dsmith@redhat.com
Red Hat, Inc.
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)


[-- Attachment #2: pre_compiled.patch --]
[-- Type: text/x-patch, Size: 10676 bytes --]

Index: buildrun.cxx
===================================================================
RCS file: /cvs/systemtap/src/buildrun.cxx,v
retrieving revision 1.23
diff -u -p -u -p -w -r1.23 buildrun.cxx
--- buildrun.cxx	9 May 2006 09:33:19 -0000	1.23
+++ buildrun.cxx	27 Jun 2006 18:30:59 -0000
@@ -128,6 +128,9 @@ run_pass (systemtap_session& s)
   if (s.buffer_size)
     stpd_cmd += "-b " + stringify(s.buffer_size) + " ";
   
+  if (s.pre_compiled_mode)
+    stpd_cmd += s.module_name;
+  else  
   stpd_cmd += s.tmpdir + "/" + s.module_name + ".ko";
   
   if (s.verbose>1) clog << "Running " << stpd_cmd << endl;
Index: main.cxx
===================================================================
RCS file: /cvs/systemtap/src/main.cxx,v
retrieving revision 1.48
diff -u -p -u -p -w -r1.48 main.cxx
--- main.cxx	2 Jun 2006 15:54:26 -0000	1.48
+++ main.cxx	27 Jun 2006 18:30:59 -0000
@@ -57,6 +57,8 @@ usage (systemtap_session& s, int exitcod
     << endl
     << "   or: stap [options] -e SCRIPT    Run given script."
     << endl
+    << "   or: stap [options] -P PRE_COMPILED_MODULE  Run pre-compiled module."
+    << endl
     << endl
     << "Options:" << endl
     << "   --         no more options after this" << endl
@@ -93,6 +95,9 @@ usage (systemtap_session& s, int exitcod
     << endl
     << "   -x PID     sets target() to PID" << endl
     << "   -t         benchmarking timing information generated" << endl
+    << "   -S         Save the compiled module in the current directory" << endl
+    << "   -P PRE_COMPILED_MODULE" << endl
+    << "              Run pre-compiled module" << endl
     ;
   // -d: dump safety-related external references
 
@@ -138,6 +143,8 @@ main (int argc, char * const argv [])
   s.cmd = "";
   s.target_pid = 0;
   s.merge=true;
+  s.pre_compiled_mode = false;
+  s.save_module = false;
 
   const char* s_p = getenv ("SYSTEMTAP_TAPSET");
   if (s_p != NULL)
@@ -153,7 +160,7 @@ main (int argc, char * const argv [])
 
   while (true)
     {
-      int grc = getopt (argc, argv, "hVMvtp:I:e:o:R:r:m:kgc:x:D:bs:u");
+      int grc = getopt (argc, argv, "hVMvtp:I:e:o:R:r:m:kgc:x:D:bs:uP:S");
       if (grc < 0)
         break;
       switch (grc)
@@ -251,6 +258,15 @@ main (int argc, char * const argv [])
 	  s.macros.push_back (string (optarg));
 	  break;
 
+	case 'P':
+	  s.pre_compiled_mode = true;
+	  s.module_name = string(optarg);
+	  break;
+
+	case 'S':
+	  s.save_module = true;
+	  break;
+
         case 'h':
           usage (s, 0);
           break;
@@ -285,13 +301,80 @@ main (int argc, char * const argv [])
     }
 
   // need a user file
-  if (! have_script)
+  if (! have_script && ! s.pre_compiled_mode)
+    {
+      cerr << "A script or pre-compiled module must be specified." << endl;
+      usage(s, 1);
+    }
+
+  if (s.pre_compiled_mode)
+    {
+      // There isn't any point in specifying '-D' -with '-P' since the
+      // complation has already occurred.
+      if (s.macros.size() != 0)
+        {
+	  cerr << "You can't specify the -D and -P options together." << endl;
+	  usage(s, 1);
+	}
+      // There isn't any point in specifying '-I' -with '-P' since the
+      // complation has already occurred.
+      if (s.include_path.size() != 1)
+        {
+	  cerr << "You can't specify the -I and -P options together." << endl;
+	  usage(s, 1);
+	}
+      // There isn't any point in specifying '-g' -with '-P' since the
+      // complation has already occurred.
+      if (s.guru_mode)
+        {
+	  cerr << "You can't specify the -g and -P options together." << endl;
+	  usage(s, 1);
+	}
+      // There isn't any point in specifying '-u' -with '-P' since the
+      // complation has already occurred.
+      if (s.unoptimized)
+        {
+	  cerr << "You can't specify the -u and -P options together." << endl;
+	  usage(s, 1);
+	}
+      // There isn't any point in specifying '-k' -with '-P' since there
+      // is no temporary directory.
+      if (s.keep_tmpdir)
+        {
+	  cerr << "You can't specify the -k and -P options together." << endl;
+	  usage(s, 1);
+	}
+      // There isn't any point in specifying '-p' -with '-P' since
+      // we're automatically only doing step 5.
+      if (s.last_pass != 5)
     {
-      cerr << "A script must be specified." << endl;
+	  cerr << "You can't specify the -p and -P options together." << endl;
       usage(s, 1);
     }
+      // There isn't any point in specifying '-S' -with '-P' since there
+      // is no module to save.
+      if (s.save_module)
+        {
+	  cerr << "You can't specify the -S and -P options together." << endl;
+	  usage(s, 1);
+	}
+    }
+
+  if (s.save_module)
+    {
+      // With '-S', you must run at least pass 4 (the compilation
+      // phase) since there wouldn't be anything to save if we stopped
+      // before pass 4.
+      if (s.last_pass < 4)
+        {
+	  cerr << "When using the -S option, you must specify at least"
+	       << " pass 4 with the -p option." << endl;
+	  usage(s, 1);
+	}
+    }
 
   int rc = 0;
+  int pass4_rc = 0;
 
   // override PATH and LC_ALL
   char* path = "PATH=/bin:/sbin:/usr/bin:/usr/sbin";
@@ -309,6 +392,7 @@ main (int argc, char * const argv [])
 
   // Create a temporary directory to build within.
   // Be careful with this, as "s.tmpdir" is "rm -rf"'d at the end.
+  if (! s.pre_compiled_mode)
   {
     char* tmpdir_env = getenv("TMPDIR");
     if (! tmpdir_env)
@@ -320,7 +404,8 @@ main (int argc, char * const argv [])
     if (! tmpdir)
       {
         const char* e = strerror (errno);
-        cerr << "ERROR: cannot create temporary directory (\"" << tmpdirt << "\"): " << e << endl;
+	  cerr << "ERROR: cannot create temporary directory (\""
+	       << tmpdirt << "\"): " << e << endl;
         exit (1); // die
       }
     else
@@ -335,6 +420,11 @@ main (int argc, char * const argv [])
   struct timeval tv_before;
   gettimeofday (&tv_before, NULL);
 
+  unsigned _sc_clk_tck = sysconf (_SC_CLK_TCK);
+  struct tms tms_after;
+  struct timeval tv_after;
+  if (! s.pre_compiled_mode)
+    {
   // PASS 1a: PARSING USER SCRIPT
   // XXX: pass args vector, so parser (or lexer?) can substitute
   // $1..$NN with actual arguments
@@ -360,7 +450,8 @@ main (int argc, char * const argv [])
   version_suffixes.push_back ("/" + kvr);
   // add kernel version (2.6.NN) + arch
   string::size_type dash_index = kvr.find ('-');
-  if (dash_index > 0 && dash_index != string::npos) {
+      if (dash_index > 0 && dash_index != string::npos)
+        {
     kvr.erase(dash_index);
     version_suffixes.push_back ("/" + kvr + "/" + arch);
     version_suffixes.push_back ("/" + kvr);
@@ -368,7 +459,8 @@ main (int argc, char * const argv [])
   // add kernel family (2.6) + arch
   string::size_type dot1_index = kvr.find ('.');
   string::size_type dot2_index = kvr.rfind ('.');
-  while (dot2_index > dot1_index && dot2_index != string::npos) {
+      while (dot2_index > dot1_index && dot2_index != string::npos)
+        {
     kvr.erase(dot2_index);
     version_suffixes.push_back ("/" + kvr + "/" + arch);
     version_suffixes.push_back ("/" + kvr);
@@ -423,10 +515,7 @@ main (int argc, char * const argv [])
           }
     }
 
-  struct tms tms_after;
   times (& tms_after);
-  unsigned _sc_clk_tck = sysconf (_SC_CLK_TCK);
-  struct timeval tv_after;
   gettimeofday (&tv_after, NULL);
 
 #define TIMESPRINT \
@@ -521,7 +610,8 @@ main (int argc, char * const argv [])
   times (& tms_after);
   gettimeofday (&tv_after, NULL);
 
-  if (s.verbose) clog << "Pass 2: analyzed script: "
+      if (s.verbose)
+	clog << "Pass 2: analyzed script: "
                       << s.probes.size() << " probe(s), "
                       << s.functions.size() << " function(s), "
                       << s.globals.size() << " global(s) in "
@@ -552,7 +642,8 @@ main (int argc, char * const argv [])
   times (& tms_after);
   gettimeofday (&tv_after, NULL);
 
-  if (s.verbose) clog << "Pass 3: translated to C into \""
+      if (s.verbose)
+	clog << "Pass 3: translated to C into \""
                       << s.translated_source
                       << "\" in "
                       << TIMESPRINT
@@ -572,7 +663,8 @@ main (int argc, char * const argv [])
   times (& tms_after);
   gettimeofday (&tv_after, NULL);
 
-  if (s.verbose) clog << "Pass 4: compiled C into \""
+      if (s.verbose)
+	clog << "Pass 4: compiled C into \""
                       << s.module_name << ".ko"
                       << "\" in "
                       << TIMESPRINT
@@ -584,7 +676,9 @@ main (int argc, char * const argv [])
          << endl;
 
   // XXX: what to do if rc==0 && last_pass == 4?  dump .ko file to stdout?
+      pass4_rc = rc;
   if (rc || s.last_pass == 4) goto cleanup;
+    }
 
   // PASS 5: RUN
   times (& tms_before);
@@ -592,11 +686,13 @@ main (int argc, char * const argv [])
   // NB: this message is a judgement call.  The other passes don't emit
   // a "hello, I'm starting" message, but then the others aren't interactive
   // and don't take an indefinite amount of time.
-  if (s.verbose) clog << "Pass 5: starting run." << endl;
+  if (s.verbose)
+    clog << "Pass 5: starting run." << endl;
   rc = run_pass (s);
   times (& tms_after);
   gettimeofday (&tv_after, NULL);
-  if (s.verbose) clog << "Pass 5: run completed in "
+  if (s.verbose)
+    clog << "Pass 5: run completed in "
                       << TIMESPRINT
                       << endl;
 
@@ -608,6 +704,20 @@ main (int argc, char * const argv [])
   // if (rc) goto cleanup;
 
  cleanup:
+  // If the user requested that we save the module and pass 4 worked,
+  // save module.
+  if (s.save_module && !pass4_rc && s.last_pass >= 4)
+    {
+      string copycmd = "cp ";
+      copycmd += s.tmpdir + "/" + s.module_name + ".ko .";
+      if (s.verbose>1) clog << "Running " << copycmd << endl;
+      int status = system (copycmd.c_str());
+      if (status != 0)
+	clog << "Module save failed, status: " << status << endl;
+      else
+	clog << "Module saved as " << s.module_name << ".ko" << endl;
+    }
+
   // Clean up temporary directory.  Obviously, be careful with this.
   if (s.tmpdir == "")
     ; // do nothing
Index: session.h
===================================================================
RCS file: /cvs/systemtap/src/session.h,v
retrieving revision 1.7
diff -u -p -u -p -w -r1.7 session.h
--- session.h	9 May 2006 09:33:19 -0000	1.7
+++ session.h	27 Jun 2006 18:30:59 -0000
@@ -77,6 +77,8 @@ struct systemtap_session
   bool unoptimized;
   bool merge;
   int buffer_size;
+  bool pre_compiled_mode;
+  bool save_module;
 
   // temporary directory for module builds etc.
   // hazardous - it is "rm -rf"'d at exit

Re: pre-compiled modules

[Frank Ch. Eigler]

joshua.i.stone wrote:

> [...]  Your example usage was to simply compile it, save to the
> local directory, and immediately turn around and run it.  But in
> this model, I don't see what value has been added over just running
> it in one shot.

Good point.  We should think about the larger use scenarios, not just
the smaller steps convenient for our code.

> I think the value of pre-compiled modules is that it enables you to
> keep a library of modules that users can fire up at will.  [...]

Right, if for no other purpose than as a cache.  In this case, the
pre-compilation and reuse should be transparently automated.

Another general notion is running the script elsewhere or "elsewhen".
The elsewhere could be a different machine of the same platform (where
plain .ko reuse is sufficient) or a different platform (likely
cross-compilation), or perhaps even a group of machines.  The elsewhen
could be "from next kernel reboot" as in dtrace anonymous probes (and
our bug #2035), "once" as in now, or "periodically" as in cron.  

It is worth considering whether this spectrum of usage should be
addressed by systemtap, or else to make it an SEP and let people roll
their own.


- FChE

Re: pre-compiled modules

[Roland McGrath]

> Or, we could request some module-exported functions:

Naturally.  I was just giving advice on the current state of the kernel.

>   // already exists, just needs an EXPORT ... or abuse symbol_put_addr() ?
>   void module_put (struct module *)

This is an inline and can already be used from modules.


Thanks,
Roland

Re: pre-compiled modules

[Frank Ch. Eigler]

roland wrote:

> [...]  Indeed there are no convenient exported kernel interfaces for
> finding modules.  Short of probe-like techniques using the kernel's
> DWARF info to access unexported things, you can't do it all purely
> in the module itself.  [...]

Or, we could request some module-exported functions:

  // a composition of find_module and module_get
  struct module * module_get_byname (char *name) 

  // already exists, just needs an EXPORT ... or abuse symbol_put_addr() ?
  void module_put (struct module *)

Or perhaps a special module-load notifier could be invented that does
a callback for all currently loaded modules, not just future ones.


> Wacky as it is, what I actually recommend is this.  For each module
> of interest (containing probe targets or global variable addresses
> used by other probes), open
> "/sys/module/MODNAME/sections/.gnu.linkonce.this_module".  [...]

This may be workable for the medium term.  It would reinforce the
current requirent for user-space management for probe modules to even
start, which is unfortunate.


- FChE

RE: pre-compiled modules

[Roland McGrath]

> Sorry, I meant to say: "Will this code go in the module itself or the
> stpd daemon?"

Some of each, I think.  Indeed there are no convenient exported kernel
interfaces for finding modules.  Short of probe-like techniques using the
kernel's DWARF info to access unexported things, you can't do it all purely
in the module itself.  It's easy to do lots of these things once you have the
module handle (struct module *) for the module in question.  Every module can
find the list implicitly because it can find its own THIS_MODULE and follow
the linked list.  However, it does not have any access to the lock that
governs the list structure.  So it's not safe to access the list structure,
or any module handle you aren't sure cannot be removed.

Wacky as it is, what I actually recommend is this.  For each module of
interest (containing probe targets or global variable addresses used by other
probes), open "/sys/module/MODNAME/sections/.gnu.linkonce.this_module".  From
there you can read (0x... in ascii) the address of MODNAME's module handle.
Then, *keep the file descriptor open*.  As long as you keep the fd on a
/sys/module/MODNAME/sections/foobar file open, then MODNAME is pinned and
cannot be removed.  Keep these fd's open until the probe module has loaded
and run its initializer.  (For now, we might want to keep them open until the
probe is removed, since we are not yet prepared for modules of interest to be
removed.)  

When inserting the probe module, pass in a parameter
"module_addresses=0x123,0x234,...".  The probe code will declare:

	static struct module *modules[NMODULES_WE_CARE_ABOUT];
	module_param_array(modules, ulong, NULL, 0444);

Then its initializer can do:

	if (modules[0]->module_core != 0x123... /* module 0 base */)
	  lose;
	if (modules[1]->module_core != 0x234... /* module 1 base */)
	  lose;

etc.  Later on, this will be:

	_stp_runtime_foobar(modules[0], kprobe_list_for_0, nprobes_for_0,
			    nsections_for_0, section_offs_for_0,
			    section_names_for_0);

after defining globals like:

	const char *const sections_for_0[] = { ".text", ".data" };
	unsigned long section_offs_for_0[2];

(Probably you need to generate code to fix up each kprobe individually after
the runtime finds the section addresses.)  The translator will eventually be
generating "section_offs_for_0[0] + 0x123" for the addresses gleaned from
DWARF info instead of straight constants.

The runtime function can look at module->sect_attrs to map section names to
addresses.  This is the same data structure that
/sys/module/MODNAME/sections/SECNAME gets to via sysfs magic, but it is
probably more efficient just to consult it directly (even with linear string
searches vs dcache for the SECNAME lookup).  


What makes it robust to use an fd to pin a module of interest while a probe
module looks it up is that the module initializer runs in the context of the
process that makes the init_module system call to install it.  Until that
initializer finishes, that process can't do anything else like process
signals and die, or close an fd.  So the robustness depends on the fd being
held open by the same process that makes the syscall, and on that process not
having other threads that can close its fds.  That means one really should
replace running insmod with a specialized program that we specificially
design to keep fds open.  That is much less trouble than it sounds like.
insmod doesn't do anything.  

There is a system call, which is in libc, but helpfully not declared anywhere:

	int init_module (void *buf, size_t bytes, const char *string);

You call this with the contents and length of the whole .ko file, and a
string of "foo=abc bar=def" (space separated parameters).  That's it.
Maybe stpd can do this directly.  Or perhaps a special child program just to
do the loading, since it's the only thing that needs to be root.


Thanks,
Roland

RE: pre-compiled modules

[Roland McGrath]

> > > To help move this along a bit, I've attached a patch that modifies the
> > > systemtap front end to take 2 new options:
> > > 
> > >    -S         Save the compiled module in the current directory
> > 
> > I would prefer to see an option to specify the directory, instead of
> > assuming $PWD.  You could always use '-S .', but some people might want
> > to drop it somewhere else.  Another possibility instead of -S is to just
> > extend the -m option to allow a path.
> 
> That's easy enough to add, but it seems like that 99% of the time I'd
> want it to go in the current directory.
> 
> Anyone else got any opinions?

The output compiled module is the single .ko file, right?  That being the
case, I think what makes the most sense is an option that takes a file name.

> > Have you thought about concurrent access to a precompiled module?  If
> > you have a systemtap module foo.ko on a multi-user system, you might end
> > up with a situation where multiple people want to run it at the same
> > time.  Of course you can only insmod one at a time, if nothing else
> > because of the naming issue.  
> 
> Hmm, no I hadn't considered that.  We've also got a similar but
> different problem.  What happens when 2 users both compile two different
> scripts called 'foo.stp' into 'foo.ko', then try to run them
> concurrently?

It doesn't matter what the name of the .ko file is at load time.  The
module name is embedded in the struct module that's compiled into the file
by the Kbuild process.  So as to the latter case of unrelated modules that
users think of as having the same name, that is easily avoided by choosing
unique names at module compilation time.  

RE: pre-compiled modules

[Stone, Joshua I]

On Tuesday, June 27, 2006 12:59 PM, David Smith wrote:
>>> Hmm, I hadn't considered that.  Will this code go in the module
>>> itself or go in the runtime?
>> 
>> Is there a difference?  The runtime is still compiled into the
>> module, right?
> 
> Sorry, I meant to say: "Will this code go in the module itself or the
> stpd daemon?"

Well, it's probably difficult for stpd to determine where the systemtap module thinks that all the kernel modules are loaded.  But I also don't know of any kernel API's for looking up modules addresses.  It may have to be some ugly cross where stpd looks in /proc/modules for current addresses, and the systemtap module keeps an array of where it thinks the module bases are.  Then pass addresses over the transport layer to compare (either direction would work).  We don't need to match all modules exactly, just the ones that we want to place probes in.  It would probably be best to compare the sizes too, as a crude version check.

Hien's suggestion about some sort of metadata file could work too, and then you can do all checks in stpd.  You then just have to make sure that the metadata and ko are paired correctly...
 
>> I would prefer to see an option to specify the directory, instead of
>> assuming $PWD.  You could always use '-S .', but some people might
>> want to drop it somewhere else.  Another possibility instead of -S
>> is to just extend the -m option to allow a path.
> 
> That's easy enough to add, but it seems like that 99% of the time I'd
> want it to go in the current directory.
> 
> Anyone else got any opinions?

I'll defer to popular opinion, of course.  I think the difference in opinion is really about what the usage model looks like.  Your example usage was to simply compile it, save to the local directory, and immediately turn around and run it.  But in this model, I don't see what value has been added over just running it in one shot.

I think the value of pre-compiled modules is that it enables you to keep a library of modules that users can fire up at will.  You would probably have a directory somewhere that is your systemtap module repository, and you might not even run the module right away.  One could imagine that stpd could be augmented to support a module search path, à la modprobe.  For these cases, I think the ability to specify the output directory would be helpful.

>> Have you thought about concurrent access to a precompiled module?  If
>> you have a systemtap module foo.ko on a multi-user system, you might
>> end up with a situation where multiple people want to run it at the
>> same time.  Of course you can only insmod one at a time, if nothing
>> else because of the naming issue.
> 
> Hmm, no I hadn't considered that.  We've also got a similar but
> different problem.  What happens when 2 users both compile two
> different scripts called 'foo.stp' into 'foo.ko', then try to run them
> concurrently?
> 
> I'm unsure of what we can do in either case.

Right now we hack around this by including the stap PID in the default name.  Perhaps the default name could instead just derive from the script name (foo.stp -> foo.ko), and then at load time the module could be copied to a unique name (foo_$PID.ko, stap_foo_$PID.ko, etc.).  Your -P option could manage this before it calls stpd.

I tried to do this manually, but it seems this won't work anyway.  As it turns out, the name of the module is compiled in, so renaming the ko later has no effect.  It looks like the name is specified in stap_###.mod.c.  I think the only way we'll be able to handle renaming is to save only the stap_###.o, and finish the compile into a ko with the desired name at load time.  Or we could try to patch the name in the ko, but that's a lot uglier.

This actually exposes a small bug in stpd too -- stpd looks in /proc/systemtap/<name>/ to communicate with the module, but it derives <name> from the filename, which won't match the module's real name if someone renamed it.


Josh

RE: pre-compiled modules

[David Smith]

Thanks for the response.  See stuff below.

On Tue, 2006-06-27 at 12:47 -0700, Stone, Joshua I wrote:
> On Tuesday, June 27, 2006 11:52 AM, David Smith wrote:
> >> Blatant modversions mismatches will be detected during the actual
> >> insertion attempt - IMO there is no need to make a dry run first.  We
> >> will need more self-protective code though for purposes of verifying
> >> module addresses.  Specifically, we need to find a kernel API routine
> >> that, given a module name, gives its loaded base address.
> > 
> > Hmm, I hadn't considered that.  Will this code go in the module itself
> > or go in the runtime?
> 
> Is there a difference?  The runtime is still compiled into the module,
> right?

Sorry, I meant to say: "Will this code go in the module itself or the
stpd daemon?"

> > To help move this along a bit, I've attached a patch that modifies the
> > systemtap front end to take 2 new options:
> > 
> >    -S         Save the compiled module in the current directory
> 
> I would prefer to see an option to specify the directory, instead of
> assuming $PWD.  You could always use '-S .', but some people might want
> to drop it somewhere else.  Another possibility instead of -S is to just
> extend the -m option to allow a path.

That's easy enough to add, but it seems like that 99% of the time I'd
want it to go in the current directory.

Anyone else got any opinions?

> >    -P PRE_COMPILED_MODULE
> >               Run pre-compiled module
> 
> Looks fine.
> 
> Have you thought about concurrent access to a precompiled module?  If
> you have a systemtap module foo.ko on a multi-user system, you might end
> up with a situation where multiple people want to run it at the same
> time.  Of course you can only insmod one at a time, if nothing else
> because of the naming issue.  

Hmm, no I hadn't considered that.  We've also got a similar but
different problem.  What happens when 2 users both compile two different
scripts called 'foo.stp' into 'foo.ko', then try to run them
concurrently?

I'm unsure of what we can do in either case.

-- 
David Smith
dsmith@redhat.com
Red Hat, Inc.
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

RE: pre-compiled modules

[Stone, Joshua I]

On Tuesday, June 27, 2006 11:52 AM, David Smith wrote:
>> Blatant modversions mismatches will be detected during the actual
>> insertion attempt - IMO there is no need to make a dry run first.  We
>> will need more self-protective code though for purposes of verifying
>> module addresses.  Specifically, we need to find a kernel API routine
>> that, given a module name, gives its loaded base address.
> 
> Hmm, I hadn't considered that.  Will this code go in the module itself
> or go in the runtime?

Is there a difference?  The runtime is still compiled into the module,
right?

> To help move this along a bit, I've attached a patch that modifies the
> systemtap front end to take 2 new options:
> 
>    -S         Save the compiled module in the current directory

I would prefer to see an option to specify the directory, instead of
assuming $PWD.  You could always use '-S .', but some people might want
to drop it somewhere else.  Another possibility instead of -S is to just
extend the -m option to allow a path.

>    -P PRE_COMPILED_MODULE
>               Run pre-compiled module

Looks fine.

Have you thought about concurrent access to a precompiled module?  If
you have a systemtap module foo.ko on a multi-user system, you might end
up with a situation where multiple people want to run it at the same
time.  Of course you can only insmod one at a time, if nothing else
because of the naming issue.  


Josh