Re: [PATCHSET 00/13] tracing/uprobes: Add support for more fetchmethods (v6)
From: Oleg Nesterov
Date: Wed Nov 06 2013 - 12:36:44 EST
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;
instruction_pointer_set(temp_ip);
store_trace_args(...);
instruction_pointer_set(saved_ip);
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);
copy_from_user(uaddr);
}
Then,
> > 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
FETCH_MTD_memory_dotranslate.
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, ¶m);
f->fn = t->fetch[FETCH_MTD_memory_dotranslate];
f->data = (void *)param;
break;
branch.
> > 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.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/