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

[Namhyung Kim]

Hi Oleg,

On Wed, 6 Nov 2013 17:28:06 +0100, Oleg Nesterov wrote:
> On 11/06, Namhyung Kim wrote:
>>
>> On Tue, 5 Nov 2013 18:45:35 +0100, Oleg Nesterov wrote:
>> > On 11/05, Namhyung Kim wrote:
>> >>
>> >> This is what I have for now:
>> >>
>> >> static void __user *get_user_vaddr(struct pt_regs *regs, unsigned long addr,
>> >> 				   struct trace_uprobe *tu)
>> >> {
>> >> 	unsigned long base_addr;
>> >> 	unsigned long vaddr;
>> >>
>> >> 	base_addr = instruction_pointer(regs) - tu->offset;
>> >> 	vaddr = base_addr + addr;
>> >>
>> >> 	return (void __force __user *) vaddr;
>> >> }
>> >>
>> >> When I tested it, it was able to fetch global and bss data from both of
>> >> executable and library properly.
>> >
>> > Heh ;) I didn't expect you will agree with this suggestion. But if you
>> > think it can work - great!
>>
>> It seems to work for me well except the cross-fetch.
>
> Yes, but cross-fetching needs something different anyway, so I think we
> should discuss this separately.

Okay.

>
>> But I'm not sure it'll work for every cases.
>
> I think "ip - tu->offset + vaddr" trick should always work, just we need
> to calculate this "vaddr" passed as an argument correctly.

Right.

>
> Except: user-space can create another executable mapping and call the
> probed function via another address, but I think we can ignore this.
> And I think we can do nothing in this case, because in this case we
> can't even rely on tu->inode.

Agreed.


>> > 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?
>>
>> Hmm.. it makes sense too. :)
>
> I am no longer sure ;)
>
> This way the "@" argument will look more confusing, it will depend on the
> address/offset of the probed insn. But again, I do not know, this is up
> to you.

That said, I'd prefer the original "-= -tu->offset" approach.  It'll
make debugging easier IMHO.

>
>> >> But it still doesn't work for uretprobes
>> >> as you said before.
>> >
>> > This looks simple,
>> >
>> > 	+	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);
>> >
>> > although not pretty.
>>
>> So for normal non-uretprobes, func == instruction_pointer(), right?
>
> No, for normal non-uretprobes func == 0 (actually, undefined).

Ah, okay.

>
>> If so, just passing func as you suggested looks better than this.
>
> Not sure I understand... OK, we can change uprobe_trace_func() and
> uprobe_perf_func()
>
> 		if (!is_ret_probe(tu))
> 	-		uprobe_trace_print(tu, 0, regs);
> 	+		uprobe_trace_print(tu, instruction_pointer(regs), regs);
> 		return 0;
>
> but why?
>
> We need the "saved_ip" ugly hack above only if is_ret_probe() == T and
> thus instruction_pointer() doesn't match the address of the probed function.
> And there is no way to pass some additional info to call_fetch/etc from
> uprobe_*_print().

Ah, I was confused that the 'func' is somewhat available in trace_uprobe
and it can make to avoid passing regs to get_user_vaddr().

Sorry for the noise.


> See also another email...

Will do.

Thanks,
Namhyung
--
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/