Re: [PATCHSET 00/13] tracing/uprobes: Add support for more fetch methods (v6)

[Namhyung Kim <namhyung@kernel.org>]

On Wed, 6 Nov 2013 18:37:54 +0100, Oleg Nesterov wrote:
> 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.

Okay, but as I said before, subtracting tu->offset part can be removed.

>
> 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().

Okay, I agree with you that adding one more fetch method will make
things simpler.

>
> 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);
> 	}

Looks good.

>
> 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.

Yeah, and I want to hear from Masami.

>
> 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

I got it.

>
> 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;
> 		break;
> 		
> branch.

Looks good.

>
>> > 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.

Okay.

>
> 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.

Thank you very much for providing good review, suggestion and pseudo
code. :)  I indeed like this approach too.

I'll change the code this way in next version.

Thanks,
Namhyung