Messages in this thread |  | | Date | Wed, 6 Nov 2013 18:37:54 +0100 | From | Oleg Nesterov <> | Subject | Re: [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; 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.
|  |