Re: [PATCH v1 7/10] Uprobes Implementation

[Masami Hiramatsu <mhiramat@redhat.com>]

Peter Zijlstra wrote:
> On Tue, 2010-03-23 at 10:20 -0400, Masami Hiramatsu wrote:
> 
>>> And yes, all processes that share that DSO, consumers can install
>>> filters.
>>
>> Hmm, for low-level interface, it will be good. If we provide
>> a user interface(trace_uprobe.c), we'd better add pid filter
>> for it.
> 
> ftrace already has pid filters.

Indeed.

> 
>>>>> Also, like we discussed in person, I think we can do away with the
>>>>> handler_in_interrupt thing by letting the handler have an error return
>>>>> value and doing something like:
>>>>>
>>>>> do_int3:
>>>>>
>>>>>   uprobe = find_probe_point(addr);
>>>>>
>>>>>   pagefault_disable();
>>>>>   err = uprobe->handler(uprobe, regs);
>>>>>   pagefault_enable();
>>>>>
>>>>>   if (err == -EFAULT) {
>>>>>     /* set TIF flag and call the handler again from
>>>>>        task context */
>>>>>   }
>>>>>
>>>>> This should allow the handler to optimistically access memory from the
>>>>> trap handler, but in case it does need to fault pages in we'll call it
>>>>> from task context.
>>>>
>>>> Okay but what if the handler is coded to sleep.
>>>
>>> Don't do that ;-)
>>>
>>> What reason would you have to sleep from a int3 anyway? You want to log
>>> bits and get on with life, right? The only interesting case is faulting
>>> when some memory references you want are not currently available, and
>>> that can be done as suggested.
>>
>> Out of curiously, what does the task-context mean? ('current' is probed
>> task in int3, isn't it?). I think, uprobe handler can cause page fault
>> (and should sleep) if the page is swapped out.
> 
> Task context means the regular kernel task stack where we can schedule,
> int3 has its own exception stack and we cannot schedule from that.
> 
> And yes, the fault thing is the one case where sleeping makes sense and
> is dealt with in my proposal, you don't need two handlers for that, just
> call it from trap context with pagefault_disable() and when it fails
> with -EFAULT set a TIF flag to deal with it later when we're back in
> task context.

Ah, I see. so it will be done later. Actually, since int3 handler will
disable irq, it is reasonable.

> There is a very good probability that the memory you want to reference
> is mapped (because typically the program itself will want to access it
> as well) so doing the optimistic access with pagefault_disabled() will
> work most of the times and you only end up taking the slow path when it
> does indeed fault.

hm, similar technique can be applied to kprobe-tracer too (for getting
__user arguments). :)


>>>>> Everybody else simply places callbacks in kernel/fork.c and
>>>>> kernel/exit.c, but as it is I don't think you want per-task state like
>>>>> this.
>>>>>
>>>>> One thing I would like to see is a slot per task, that has a number of
>>>>> advantages over the current patch-set in that it doesn't have one page
>>>>> limit in number of probe sites, nor do you need to insert vmas into each
>>>>> and every address space that happens to have your DSO mapped.
>>>>>
>>>>
>>>> where are the per task slots stored?
>>>> or Are you looking at a XOL vma area per DSO?
>>>
>>> The per task slot (note the singular, each task needs only ever have a
>>> single slot since a task can only ever hit one trap at a time) would
>>> live in the task TLS or task stack.
>>
>> Hmm, I just worried about whether TLS/task stack can be executable
>> (no one set NX bit).
> 
> You can remove the NX bit from that one page I guess.

OK.

>>>>> Also, I would simply kill the user_bkpt stuff and merge it into uprobes,
>>>>> we don't have a kernel_bkpt thing either, only kprobes.
>>>>>
>>>>
>>>> We had uprobes as one single layer. However it was suggested that
>>>> breaking it up into two layers was useful because it would help code
>>>> reuse. Esp it was felt that a generic user_bkpt layer would be far more
>>>> useful than being used for just uprobes.
>>>> Here are links where these discussion happened.
>>>
>>> I'm so not going to read ancient emails on a funky list. What re-use?
>>> uprobe should be the only interface to this, there's no second interface
>>> to kprobes either is there?
>>
>> It will be good when we start working on 'ptrace2' :)
>> Anyway, the patch order looks a bit odd, because user_bkpt uses XOL
>> but XOL patch is introduced after user_bkpt patch...
> 
> But why would ptrace2 use a different interface? Also, why introduce
> some abstraction layer now without having a user for it, you could
> always restructure things and or add interfaces later when you have a
> clear idea what it is you need.

Because 'ptrace' doesn't have any breakpoint insertion helper.
Programs which uses ptrace must setup single-stepping buffer and
modify target code by themselves. This causes problems when
multiple debuggers/tracers attach to the same process and
try to modify same address. First program can see the original
instruction, but next one will see int3! I think we'd better
provide some abstraction interface for breakpoint setting in
next generation ptrace (of course, we also need to provide
memory peek interface which returns original instructions).

But anyway, I agree with you, we don't need it *now*, but someday.

Thank you,

-- 
Masami Hiramatsu
e-mail: mhiramat@redhat.com