This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]