This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: kprobe fault handling
On Mon, 2006-02-06 at 16:50, Jim Keniston wrote:
> Hi, Martin. This is the subject of bugzilla #1303, which is assigned to
> Prasanna. I believe that Prasanna is also the author of the original
> kprobe_fault_handler code in kprobes, so he may be able to provide
> insight that I don't have.
>
> However, I've thought about this some, so here goes...
After rethinking this, I believe that no change of kprobes is
necessary. If the pre- or post-handler does user-space accesses using
functions that include fixups, then it should be sufficient for the
corresponding fault-handler to call fixup_exception(). Details below.
>
> On Mon, 2006-02-06 at 11:50, Martin Hunt wrote:
> > I've been trying to understand how kprobes fault handling is supposed to
> > work and why it isn't doing what I thought it did.
> >
[Snipped some analysis from Martin, which I still believe to be
correct.]
>
> > Question: What
> > do we imagine a probe-specific page fault handler would do? Why is it
> > useful?
>
> If an attempted memory access failed, the fault handler might be able to
> clean up what the pre_handler or post_handler was trying to do.
>
> For example, consider user-space return probes (which is currently hung
> up on a couple of issues, including user-space access). When we enter
> the probed function, the uretprobe version of arch_prepare_kretprobe()
> has to do the following:
> 1. Reserve a kretprobe_instance.
> 2. Save a copy of the return address (a read from user space).
> 3. Replace the return address with the uretprobe trampoline address (a
> write to user space).
> 4. Hash the kretprobe_instance.
> If the page containing the return address is not in memory (a very
> unlikely scenario, but one which I believe we must handle) then we'll
> get a page fault on #2 or #3.
OK so far.
> The corresponding fault handler could
> free up the kretprobe_instance reserved in step #1.
No. The fault handler should look something like this:
int fault_handler(struct kprobe *p, struct pt_regs *regs, int trapnr)
{
if (trapnr == 14) { /* Fix this -- i386-specific */
/* page fault */
return fixup_exception(regs);
}
return 0;
}
fixup_exceptions will adjust regs->eip to the code in the
user-read/write function that returns -EFAULT. Returning normally from
the page fault will leave us right where we want to be.
So in steps 2-3 from arch_prepare_uretprobe above, on each call to the
appropriate user-space access function (perhaps __get_user() for step
2), I'd just check to see if it returns -EFAULT.
[Snipped some stuff that I now believe to be bogus.]
>
> I understand what fixup_exceptions() does, but it's not clear to me how
> we can use it here.
It's clearer to me now, I think. :-}
> I guess if all user-space access by kprobes
> handlers used the same user-read and user-write functions, then we'd
> have a fixup for the user-read function and one for the user-write, and
> each of these could vector into the aforementioned longjmp.
Well, no, the fixups would vector to code that returns -EFAULT, as most
such fixups do.
>
> Do you envision that all user-space accesses would be via functions like
> *_from_user() in the SystemTap runtime?
We should reconsider using user-space access functions that return
-EFAULT when the page isn't resident.
>
> >
> > Then there is this code, which I don't understand
> > if (kcb->kprobe_status & KPROBE_HIT_SS) {
> > resume_execution(cur, regs, kcb);
> > regs->eflags |= kcb->kprobe_old_eflags;
> >
> > reset_current_kprobe();
> > preempt_enable_no_resched();
> > }
> >
>
> I think KPROBE_HIT_SS means that the fault happened while the probed
> instruction was being single-stepped.
Still correct, I think.
> Beyond that, I'm not sure what's
> going on.
I think I do now.
>
> > And that's it. kprobe_fault_handler returns 0. No call to
> > fixup_exceptions()! So do_page_fault() will have to do the fixups, but
> > first it will print nasty might_sleep warnings and maybe actually sleep!
You have the right idea of running fixup_exceptions() in the fault
handler, to prevent do_page_fault() from attempting its demand-paging
stuff. I just think it's OK to have the user's fault handler, rather
than kprobe_fault_handler, run fixup_exceptions.
> >
> > I could have sworn this was not the case previously but it has been a
> > very long time since I have looked at the code at this level. Anyway,
> > this MUST be fixed.
> >
> > Martin
>
> I agree that we need to resolve this. Thanks for getting this
> discussion rolling again.
>
> Jim
Jim again