[lkml]   [2013]   [Nov]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCHSET 00/13] tracing/uprobes: Add support for more fetch methods (v6)
On 11/06, Namhyung Kim wrote:
> On Tue, 5 Nov 2013 20:24:01 +0100, Oleg Nesterov wrote:
> > On 11/05, Oleg Nesterov wrote:
> >>
> >> As for "-= tu->offset"... Can't we avoid it? User-space needs to calculate
> >> the "@" argument anyway, why it can't also substruct this offset?
> >>
> >> Or perhaps we can change parse_probe_arg("@") to update "param" ? Yes,
> >> in this case it needs another argument, not sure...
> >
> > Or,
> >
> >> + if (is_ret_probe(tu)) {
> >> + saved_ip = instruction_pointer(regs);
> >> + instruction_pointer_set(func);
> >> + }
> >> store_trace_args(...);
> >> + if (is_ret_probe(tu))
> >> + instruction_pointer_set(saved_ip);
> >
> > we can put "-= tu->offset" here.
> I don't think I get the point.

I meant,

saved_ip = instruction_pointer(regs);

// pass the "ip" which was used to calculate
// the @addr argument to fetch_*() methods

temp_ip = is_ret_probe(tu) ? func : saved_ip;
temp_ip -= tu->offset;



This way we can avoid the new "void *" argument for fetch_func_t,
we do not need it to calculate the address.

But: we still need the additional "bool translate_vaddr" to solve
the problems with FETCH_MTD_deref.

We already discussed this a bit, previously I suggested the new
FETCH_MTD_memory_notranslate and

- dprm->fetch = t->fetch[FETCH_MTD_memory];
+ dprm->fetch = t->fetch[FETCH_MTD_memory_notranslate];

change in parse_probe_arg().

However, now I think it would be more clean to leave FETCH_MTD_memory
alone and add FETCH_MTD_memory_dotranslate instead.

So trace_uprobes.c should define

void FETCH_FUNC_NAME(memory, type)(addr, ...)
copy_from_user((void __user *)addr);

void FETCH_FUNC_NAME(memory_dotranslate, type)(addr, ...)
void __user *uaddr = get_user_vaddr(regs, addr);


> > Or. Perhaps we can leave "case '@'" in parse_probe_arg() and
> > FETCH_MTD_memory alone. You seem to agree that "absolute address"
> > can be useful anyway.
> Yes, but it's only meaningful to process-wide tracing sessions IMHO.

Yes, yes, sure.

I meant, we need both. Say, "perf probe "func global=@addr" means
FETCH_MTD_memory, and "perf probe "func global=*addr" means

Just in case, of course I do not care about the syntax, for example we
can use "@~addr" for translate (or not translate) or whatever.

My only point: I think we need both to

1. avoid the new argument in fetch_func_t

2. allow the dump the data from the absolute address

And just to simplify the discussion, lets assume we use "*addr" for
FETCH_MTD_memory_dotranslate and thus parse_probe_arg() gets the new

case '*':
if (is_kprobe)
return -EINVAL;

kstrtoul(arg + 1, 0, &param);
f->fn = t->fetch[FETCH_MTD_memory_dotranslate];
f->data = (void *)param;


> > Instead, perhaps we can add FETCH_MTD_memory_do_fancy_addr_translation,
> > and, say, the new "case '*'" in parse_probe_arg() should add all the
> > neccessary info as f->data (like, say, FETCH_MTD_symbol).
> Could you elaborate this more?

Yes, I was confusing sorry.

As for FETCH_MTD_memory_do_fancy_addr_translation, please see above.

As for "neccessary info as f->data". Suppose that we still have a reason
for the additional argument in FETCH_MTD_memory_dotranslate method. Even
in this case I don't think we should change the signature of fetch_func_t.

What I think we can do is something like

1. Changed parse_probe_arg() to accept "struct trace_uprobe *tu"
instead of is_kprobe. Naturally, !tu can be used instead.

2. Introduce

struct dotranslate_fetch_param {
struct trace_uprobe *tu;
fetch_func_t fetch;
fetch_func_t fetch_size;

3. Change the "case '*'" above to do

case '*':
if (!tu)
return -EINVAL;

struct dotranslate_fetch_param *xxx = kmalloc(..);

xxx->fetch = t->fetch[FETCH_MTD_memory];

// ... kstrtoul, fetch_size, etc, ...

f->fn = t->fetch[FETCH_MTD_memory_dotranslate];
f->data = (void *)xxx;

4. Update traceprobe_free_probe_arg/etc.

5. Now,

void FETCH_FUNC_NAME(memory_dotranslate, type)(addr, ...)
struct dotranslate_fetch_param *xxx = data;
void __user *uaddr = get_user_vaddr(regs, addr, tu);

xxx->fetch(regs, addr, dest);

Yes, yes, I am sure I missed something and this is not that simple,
I am new to this "fetch" code.

And even if I am right, let me repeat that I am not going to argue.
Well, at least too much ;) This looks better in my opinion, but this
is always subjective, so please free to ignore.


 \ /
  Last update: 2013-11-06 19:01    [W:0.122 / U:0.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site