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

[Oleg Nesterov <oleg@redhat.com>]

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, &param);
		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.