On Fri, Jun 05, 2009 at 03:11:58PM +1000, David Gibson wrote:
> On Wed, Jun 03, 2009 at 10:05:11PM +0530, K.Prasad wrote:
Hi David,
Sorry for the delay in response below. In the meanwhile, I
discovered an issue in detecting stray exceptions that affected
user-space handling of breakpoints. I've made some changes to correct
that behaviour which will be included in version VI of the patchset.
> > Introduce PPC64 implementation for the generic hardware breakpoint
> > interfaces
> > defined in kernel/hw_breakpoint.c. Enable the HAVE_HW_BREAKPOINT flag and
> > the
> > Makefile.
>
>
> [snip]
> > +/*
> > + * Install the debug register values for just the kernel, no thread.
>
> This comment does seem to quite match the function below.
>
Thanks for pointing out. Will change it to read thus:
/*
* Clear the DABR which contains the thread-specific breakpoint address
*/
> > + */
> > +void arch_uninstall_thread_hw_breakpoint()
> > +{
> > + set_dabr(0);
> > +}
> > +
> > +/*
> > + * Store a breakpoint's encoded address, length, and type.
> > + */
> > +int arch_store_info(struct hw_breakpoint *bp, struct task_struct *tsk)
> > +{
> > + /*
> > + * User-space requests will always have the address field populated
> > + * Symbol names from user-space are rejected
> > + */
> > + if (tsk && bp->info.name)
> > + return -EINVAL;
> > + /*
> > + * User-space requests will always have the address field populated
> > + * For kernel-addresses, either the address or symbol name can be
> > + * specified.
> > + */
> > + if (bp->info.name)
> > + bp->info.address = (unsigned long)
> > + kallsyms_lookup_name(bp->info.name);
>
> Archs don't have to implement this name lookup stuff, but it looks
> like most of them would - so it looks like there ought to be a helper
> function in generic code that will do the check / name lookup stuff.
>
>
It doesn't turn out to be very generic. The IO breakpoints in x86, the
address-range (only) breakpoints in S390 and perhaps 4xx powerpc
processors were what made me think that this should remain in
arch-specific code. In these cases, we might have to deal only with
breakpoint addresses and not names.
> > + if (bp->info.address)
> > + return 0;
>
> Hrm.. you realise there's no theoretical reason a userspace program
> couldn't put a breakpoint at address 0...?
>
I agree. I think there must be parts of code that works based on this
assumption. Will check and remove them.
> > + return -EINVAL;
> > +}
> > +
> > +/*
> > + * Validate the arch-specific HW Breakpoint register settings
> > + */
> > +int arch_validate_hwbkpt_settings(struct hw_breakpoint *bp,
> > + struct task_struct *tsk)
> > +{
> > + int is_kernel, ret = -EINVAL;
> > +
> > + if (!bp)
> > + return ret;
> > +
> > + switch (bp->info.type) {
> > + case HW_BREAKPOINT_READ:
> > + case HW_BREAKPOINT_WRITE:
> > + case HW_BREAKPOINT_RW:
> > + break;
> > + default:
> > + return ret;
> > + }
> > +
> > + if (bp->triggered)
> > + ret = arch_store_info(bp, tsk);
> > +
> > + is_kernel = is_kernel_addr(bp->info.address);
> > + if ((tsk && is_kernel) || (!tsk && !is_kernel))
> > + return -EINVAL;
> > +
> > + return ret;
> > +}
> > +
> > +void arch_update_user_hw_breakpoint(int pos, struct task_struct *tsk)
> > +{
> > + struct thread_struct *thread = &(tsk->thread);
> > + struct hw_breakpoint *bp = thread->hbp[0];
> > +
> > + if (bp)
> > + thread->dabr = (bp->info.address & ~HW_BREAKPOINT_ALIGN) |
> > + bp->info.type | DABR_TRANSLATION;
> > + else
> > + thread->dabr = 0;
> > +}
> > +
> > +void arch_flush_thread_hw_breakpoint(struct task_struct *tsk)
> > +{
> > + struct thread_struct *thread = &(tsk->thread);
> > +
> > + thread->dabr = 0;
> > +}
> > +
> > +/*
> > + * Handle debug exception notifications.
> > + */
> > +int __kprobes hw_breakpoint_handler(struct die_args *args)
> > +{
> > + int rc = NOTIFY_STOP;
> > + struct hw_breakpoint *bp;
> > + struct pt_regs *regs = args->regs;
> > + unsigned long dar = regs->dar;
> > + int cpu, is_one_shot, stepped = 1;
> > +
> > + /* Disable breakpoints during exception handling */
> > + set_dabr(0);
> > +
> > + cpu = get_cpu();
> > + /* Determine whether kernel- or user-space address is the trigger */
> > + bp = (hbp_kernel_pos == HBP_NUM) ? current->thread.hbp[0] :
> > + per_cpu(this_hbp_kernel[0], cpu);
> > + /*
> > + * bp can be NULL due to lazy debug register switching
> > + * or due to the delay between updates of hbp_kernel_pos
> > + * and this_hbp_kernel.
> > + */
> > + if (!bp)
> > + goto out;
> > +
> > + if (dar == bp->info.address)
> > + per_cpu(dabr_data, cpu) = (hbp_kernel_pos == HBP_NUM) ?
> > + current->thread.dabr : kdabr;
> > + else {
> > + /*
> > + * This exception is triggered not because of a memory access on
> > + * the monitored variable but in the double-word address range
> > + * in which it is contained. We will consume this exception,
> > + * considering it as 'noise'.
> > + */
> > + rc = NOTIFY_STOP;
> > + goto out;
> > + }
> > + is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0;
>
> Ouch, explicitly special-casing ptrace_triggered is pretty nasty.
> Since the bp_info is already arch specific, maybe it should include a
> flag to indicate whether the breakpoint is one-shot or not.
>
The reason to check for ptrace_triggered is to contain the one-shot
behaviour only to ptrace (thus retaining the semantics) and not to extend
them to all user-space requests through register_user_hw_breakpoint().
A one-shot behaviour for all user-space requests would create more work
for the user-space programs (such as re-registration) and will leave open
a small window of opportunity for debug register grabbing by kernel-space
requests.
So, in effect a request through register_user_hw_breakpoint() interface
will behave as under:
- Single-step over the causative instruction that triggered the
breakpoint exception handler.
- Deliver the SIGTRAP signal to user-space after executing the causative
instruction.
This behaviour is in consonance with that of kernel-space requests and
those on x86 processors, and helps define a consistent behaviour across
architectures for user-space.
Let me know what you think on the same.
> > + (bp->triggered)(bp, regs);
> > + /*
> > + * Ptrace expects the HW Breakpoints to be one-shot. We will return
> > + * NOTIFY_DONE without restoring DABR with the breakpoint address. The
> > + * downstream code will generate SIGTRAP to the process
> > + */
> > + if (is_one_shot) {
> > + rc = NOTIFY_DONE;
> > + goto out;
>
> Don't you need to clear dabr_data? Otherwise if we enter single step
> for some other reason (e.g. gdb turns it on), won't we incorrectly hit
> the code-path to step over a dabr breakpoint?
>
Yes, I missed it.
> > + }
> > +
> > + stepped = emulate_step(regs, regs->nip);
> > + /*
> > + * Single-step the causative instruction manually if
> > + * emulate_step() could not execute it
> > + */
> > + if (stepped == 0) {
> > + regs->msr |= MSR_SE;
> > + goto out;
> > + }
> > + set_dabr(per_cpu(dabr_data, cpu));
> > + per_cpu(dabr_data, cpu) = 0;
> > +
> > +out:
> > + /* Enable pre-emption only if single-stepping is finished */
> > + if (stepped)
> > + put_cpu_no_resched();
> > + return rc;
> > +}
> > +
> > +/*
> > + * Handle single-step exceptions following a DABR hit.
> > + */
> > +int __kprobes single_step_dabr_instruction(struct die_args *args)
> > +{
> > + struct pt_regs *regs = args->regs;
> > + int cpu = get_cpu();
> > + int ret = NOTIFY_DONE;
> > + siginfo_t info;
> > + unsigned long this_dabr_data = per_cpu(dabr_data, cpu);
> > +
> > + /*
> > + * Check if we are single-stepping as a result of a
> > + * previous HW Breakpoint exception
> > + */
> > + if (this_dabr_data == 0)
> > + goto out;
> > +
> > + regs->msr &= ~MSR_SE;
> > + /* Deliver signal to user-space */
> > + if (this_dabr_data < TASK_SIZE) {
> > + info.si_signo = SIGTRAP;
> > + info.si_errno = 0;
> > + info.si_code = TRAP_HWBKPT;
> > + info.si_addr = (void __user *)(per_cpu(dabr_data, cpu));
> > + force_sig_info(SIGTRAP, &info, current);
>
> Uh.. I recall mentioning in my previous review that in order to match
> previous behaviour we need to deliver the userspace signal *before*
> stepping over the breakpointed instruction, rather than after (which
> I guess is why breakpoints are one-shot in the old scheme).
>
This code would implement the behaviour as stated in the comment for
user-space requests above.
> > _______________________________________________
> > Linuxppc-dev mailing list
> > linuxppc-...@ozlabs.org
> > https://ozlabs.org/mailman/listinfo/linuxppc-dev
> >
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_
> _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
Thanks,
K.Prasad
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
