Re: debuginfoless static user probes

[fche at redhat dot com (Frank Ch. Eigler)]

Stan Cox <scox@redhat.com> writes:

> This patch implements adding info to the .probes section to access
> probe arguments and since the probe address is also in the .probes
> section this means no debuginfo is required for a
> process("proc").mark("mark") probe.

Thanks.

> The .probes section format is now:
> %rcx %rsi %rdi 
> seven__test
> UPB1
> pointer to seven__test
> count of probe arguments
> probe address
> pointer to argument asm string

You will earn reap manifold benefits if each and every permutation of
the .probes struct is available in sys/sdt.h as a full fledged struct
declaration.  It may be helpful to also include the capability to
generate all corresponding STAP_PROBE_DATA() variants, e.g. for use by
the testsuite.

It would also be good to document clearly the run-time interface ABI
at the probe invocation moment, for each of these probes.  i.e.,
kprobe1 style assumes kprobe in sys_geteuid, arg1 being blah, arg2
being blah2, blah blah blah.

It's nice to have the arg-count in there finally; this should improve
translation-time error handling.


> --- a/tapset/i386/registers.stp
> +++ b/tapset/i386/registers.stp
> @@ -12,12 +12,16 @@ function _stp_register_regs() {
>  
>  	/* Same order as pt_regs */
>  	_reg_offsets["ebx"] =  0		_reg_offsets["bx"] =  0
> +			      			_reg_offsets["bl"] =  0
>  	_reg_offsets["ecx"] =  4		_reg_offsets["cx"] =  4
> +			      			_reg_offsets["cl"] =  4
> [...]

Ah an interesting solution: map the register names in the assembly
code to strings supplied to the tapset function.  This would make
these tapset functions more heavily used than usual.  Some
complications: more testing re. valid and unexpected inputs, treatment
of kernel-space vs user-space register sets (see the PR10601 stuff for
uprobes).  Consider that the expressions / register names would
henceforth arrive from untrustworthy user-space binaries!


>    sdt_var_expanding_visitor(string & process_name, string & probe_name,
> -			    int arg_count, bool have_reg_args):
> +			    string & arg_string,
> +			    int arg_count, bool have_kprobe):

(Do those string& args need to be string& as opposed to const string& ?
Are those additional outputs?)


> [...]
> +	  for (unsigned i = 0; i < tok.length(); i++)
> +	    // Recognize: %R | (%R) | N(%R)
> +	    switch (tok[i])
> +	      {
> +	      case '-':
> +	      case '1' ... '9':
> +		{
> +		  disp_str = tok.substr(i,tok.find('(',i)-i);
> +		  i += disp_str.length();
> +		  disp = lex_cast<int>(disp_str);
> +		  arg_in_memory = true;
> +		  break;
> +		}
> +	      case '(':
> +		arg_in_memory = true;
> +		break;
> +	      case ')':
> +		break;
> +	      case '%':
> +		{
> +		  reg = tok.substr(i+1,tok.find(')',i)-i-1);
> +		  i += reg.length();
> +		  break;
> +		}
> +	      default:
> +		bad_asm_op = true;
> +	      }

It would seem less fragile to use a mechanism such as regular
expressions to parse the thing.  (Remember, they are not trustworthy
from a security point of view.)


> [...]
> +	      literal_number* num = new literal_number(3);
> [...]
> +	      literal_number* inc = new literal_number((argno - 1) * 8);

There are a couple of similar magic numbers (3, -1, 8) in there that
should be better explained.


> +	  if (have_kprobe || arg_in_memory)
> +	    provide(fc);
> +	  else
> +	    provide(be);

Just confirming, you are not removing the translator's compatibility
with the old style probes, right?  It may make the code best if all
the alternatives are handled in completely separate code paths in the
translator, without intermingling the code with flags like
"have_kprobe || arg_in_memory".  If there are too many conditionals,
the code probably could be improved.  This is C++, we can subtype etc.
if needed.

> @@ -4109,10 +4178,12 @@ sdt_query::handle_query_module()
>  	  clog << "matched probe_name " << probe_name << " probe_type ";
>  	  switch (probe_type)
>  	    {
> -	    case uprobe_type:
> -	      clog << "uprobe at 0x" << hex << probe_arg << dec << endl;
> +	    case uprobe1_type:
> +	    case uprobe2_type:
> +	      clog << "uprobe at 0x" << hex << pc << dec << endl;
>  	      break;
> -	    case kprobe_type:
> +	    case kprobe1_type:
> +	    case kprobe2_type:
>  	      clog << "kprobe" << endl;
>  	      break;
>  	    }

We lose diagnostic capabilities if uprobe1/uprobe2 are shown
indistinguisably here.


> +	  Dwarf_Addr bias;
> +	  Elf* elf = (dwarf_getelf (dwfl_module_getdwarf (q.dw.mod_info->mod, &bias))
> +		      ?: dwfl_module_getelf (q.dw.mod_info->mod, &bias));
> +
> +	  GElf_Ehdr ehdr_mem;
> +	  GElf_Ehdr* em = gelf_getehdr (elf, &ehdr_mem);
> +	  string section;
> +	  if ((em->e_type == ET_EXEC))
> +	    section = ".absolute";
> +	  else if (em->e_type == ET_DYN)
> +	    section = ".dynamic";
> +	  uprobe_derived_probe* p =
> +	    new uprobe_derived_probe ("", "", 0, q.module_val, section /*".absolute"*/,
> +				     q.statement_num_val, q.statement_num_val,
> +				     q, 0);
> +	  results.push_back (p);

This looks OK, but will need the sdt.exp style permutation testing
with various flavours of executables / shared libraries (pie/prelink).

> +	  sess.unwindsym_modules.insert ("kernel");

(Why?)


> +      if (probe_type == uprobe1_type || probe_type == kprobe1_type)
> +	{
> +	  struct probe_entry
> +	  {
> +	    __uint64_t name;
> +	    __uint64_t arg;
> +	  }  *pbe;
> +
> +	  probe_scn_offset += sizeof(__uint32_t);
> +	  probe_scn_offset += probe_scn_offset % sizeof(__uint64_t);
> +	  pbe = (struct probe_entry*) ((char*)pdata->d_buf + probe_scn_offset);
> +	  if (pbe->name == 0)
> +	    return false;
> +	  probe_name = (char*)((char*)pdata->d_buf + pbe->name - (char*)probe_scn_addr);

All this pointer goo could go away if sdt.h contained usable
declarations for the .probe structs.


Thanks, good progress.


- FChE