Re: [Patch 00/12] Hardware Breakpoint Interfaces

K.Prasad
2009-05-11T11:37:00Z

On Mon, May 04, 2009 at 04:55:03PM -0400, Alan Stern wrote:
> On Fri, 24 Apr 2009, K.Prasad wrote:
>
> > Hi Alan,
> > The following patches contain the changes mentioned below and is based
> > on commit 335a1e07e2281795064b909aa75e3071609abd0e of -tip tree.
> >
> > The changes to passing of DR6 register value in traps.c is separated into
> > [Patch 12/12]. kprobes and HW breakpoints have been found to work fine after
> > the changes on an x86 box.
> >
> > Kindly let me know what you think of the changes.
>
> They look pretty good. However some of the older stuff still needs
> more work.
>
> > --- /dev/null
> > +++ arch/x86/include/asm/hw_breakpoint.h
>
> > +void arch_install_thread_hw_breakpoint(struct task_struct *tsk);
> > +void arch_uninstall_thread_hw_breakpoint(void);
> > +void arch_install_kernel_hw_breakpoint(void *);
>
> This routine doesn't exist, but maybe it should. See below...
>
>

Hi Alan,
Thank you for the comments. I discovered a few locking related
issues in the code and had to fix them before I could send the patch
with your suggestions applied, and hence the delay.

Please find responses to your comments inline, while more changes
accompany the patch sent separately.

> > --- /dev/null
> > +++ kernel/hw_breakpoint.c
>
> > +int register_kernel_hw_breakpoint(struct hw_breakpoint *bp)
> > +{
> > + int rc;
> > +
> > + rc = arch_validate_hwbkpt_settings(bp, NULL);
> > + if (rc)
> > + return rc;
> > +
> > + spin_lock(&hw_breakpoint_lock);
> > +
> > + rc = -EINVAL;
> > + /* Check if we are over-committing */
> > + if ((hbp_kernel_pos > 0) && (!hbp_user_refcount[hbp_kernel_pos-1])) {
> > + hbp_kernel_pos--;
> > + hbp_kernel[hbp_kernel_pos] = bp;
> > + on_each_cpu(arch_update_kernel_hw_breakpoints, NULL, 0);
>
> You shouldn't call on_each_cpu() while holding a spinlock. The same
> thing happens in unregister_kernel_hw_breakpoint().
>

First, on_each_cpu() will now be changed to return only after all
functions invoked through IPIs have returned (by changing @wait
parameter to 1). This is required to prevent side effects of
incrementing hbp_kernel_pos after on_each_cpu() in
unregister_kernel_hw_breakpoint() [hbp_kernel_pos is still incremented
after IPI and I will explain it below].

on_each_cpu() isn't a blocking call (despite @wait being set to 1, which
does a busy wait through cpu_relax()) and should be safe to invoke
inside a spin_lock() context. I would like to know if you think
otherwise.

> > + rc = 0;
> > + }
> > +
> > + spin_unlock(&hw_breakpoint_lock);
> > + return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(register_kernel_hw_breakpoint);
> > +
> > +/**
> > + * unregister_kernel_hw_breakpoint - unregister a HW breakpoint for kernel space
> > + * @bp: the breakpoint structure to unregister
> > + *
> > + * Uninstalls and unregisters @bp.
> > + */
> > +void unregister_kernel_hw_breakpoint(struct hw_breakpoint *bp)
> > +{
> > + int i, j;
> > +
> > + spin_lock(&hw_breakpoint_lock);
> > +
> > + /* Find the 'bp' in our list of breakpoints for kernel */
> > + for (i = hbp_kernel_pos; i < HB_NUM; i++)
> > + if (bp == hbp_kernel[i])
> > + break;
> > +
> > + /* Check if we did not find a match for 'bp'. If so return early */
> > + if (i == HB_NUM) {
> > + spin_unlock(&hw_breakpoint_lock);
> > + return;
> > + }
> > +
> > + /*
> > + * We'll shift the breakpoints one-level above to compact if
> > + * unregistration creates a hole
> > + */
> > + for (j = i; j > hbp_kernel_pos; j--)
> > + hbp_kernel[j] = hbp_kernel[j-1];
>
> What happens if a kernel breakpoint is triggered on another CPU while
> this loop is running? Or what happens if the breakpoint being removed
> is triggered on another CPU before on_each_cpu() is called below?
>
> Basically, it's impossible to change the kernel breakpoints
> simultaneously on all CPUs. That means you somehow have to keep both
> the old set and the new set around until all the CPUs are updated.
>

I must admit that the code did not handle the above scenario. I am
adding a per-cpu instance of 'hbp_kernel[]' called 'this_hbp_kernel[]'.
The breakpoint handler would use the per-cpu instance which will remain
valid throughout the execution of the handler. The per-cpu instance will
be updated with hbp_kernel[] values in arch_update_kernel_hw_breakpoint().
[This necessitates hbp_kernel_pos increment to happen after the IPI call
in unregister_kernel code].

> > +
> > + hbp_kernel[hbp_kernel_pos] = NULL;
> > + on_each_cpu(arch_update_kernel_hw_breakpoints, NULL, 0);
> > + hbp_kernel_pos++;
> > +
> > + spin_unlock(&hw_breakpoint_lock);
> > +}
>
>
> > --- /dev/null
> > +++ arch/x86/kernel/hw_breakpoint.c
>
> > +/* Unmasked kernel DR7 value */
> > +static unsigned long kdr7;
> > +static const unsigned long kdr7_masks[HB_NUM + 1] = {
> > + 0xffff00ff, /* LEN3, R/W3, G3, L3 */
> > + 0xfff000fc, /* Same for 3, 2 */
> > + 0xff0000f0, /* Same for 3, 2, 1 */
> > + 0xf00f00c0, /* Same for 3, 2, 1, 0 */
> > + 0x00000000 /* Dummy mask used when 'pos' is HB_NUM */
> > +};
>
> These comments are completely messed up. The comment on the first
> line, "LEN3, R/W3, G3, L3", actually applies to the fourth value,
> 0xf00f00c0. Likewise for the others.
>
> In the end this may not matter...
>
>

The kdr7_masks[] are now removed as a result of the code re-write you
suggested below!!

> > +void arch_update_kernel_hw_breakpoints(void *unused)
> > +{
> > + struct hw_breakpoint *bp;
> > + unsigned long dr7;
> > + int i;
> > +
> > + get_debugreg(dr7, 7);
> > + /* Don't allow debug exceptions while we update the registers */
> > + set_debugreg(0UL, 7);
> > +
> > + /* Clear all kernel-space bits in kdr7 and dr7 before we set them */
> > + kdr7 &= ~kdr7_masks[hbp_kernel_pos];
> > + dr7 &= ~kdr7_masks[hbp_kernel_pos];
>
> You probably should use current->thread.debugreg7 and eliminate the
> dr7 variable entirely. That also means you can get rid of kdr7_masks,
> and it means you can increment hpb_kernel_pos before doing the
> on_each_cpu() call.
>

It's a nice optimisation. I've included your suggestion.

> > +
> > + for (i = hbp_kernel_pos; i < HB_NUM; i++) {
> > + bp = hbp_kernel[i];
> > + if (bp) {
> > + kdr7 |= encode_dr7(i, bp->info.len, bp->info.type);
> > + set_debugreg(hbp_kernel[i]->info.address, i);
> > + }
> > + }
>
> Another problem: kdr7 is a global variable, and here you've got every
> CPU recomputing it whenever a kernel breakpoint is added or removed.
> It should be computed just once, before the on_each_cpu() call.
>

If kdr7 needs to be updated only once, it has to be kept outside the IPI
through the use of a wrapper routine (in arch/x86/kernel/hw_breakpoint.c
as it is arch-specific). This would mean one more function call in
(un)register_kernel_<> routines taking the code back to one of its previous
designs. In a trade-off between code-brevity and efficiency, the present one
chose the former keeping in mind some of the comments received during the
early stages of this patch.

> > +
> > + dr7 |= kdr7;
> > +
> > + /* No need to set DR6 */
> > + set_debugreg(dr7, 7);
> > +}
>
>
> > --- arch/x86/kernel/ptrace.c.orig
> > +++ arch/x86/kernel/ptrace.c
>
> > +static int ptrace_write_dr7(struct task_struct *tsk, unsigned long data)
> > +{
> > + struct thread_struct *thread = &(tsk->thread);
> > + unsigned long old_dr7 = thread->debugreg7;
> > + int i, rc = 0;
> > + int enabled;
> > + unsigned len, type;
> > + struct hw_breakpoint *bp;
> > + /*
> > + * We want to use allocated memory inside a spinlock and we use the
> > + * trick below
> > + */
> > + int temp_mem_used = 0;
> > + void *temp_mem = kzalloc(sizeof(struct hw_breakpoint), GFP_KERNEL);
> > + if (!temp_mem)
> > + temp_mem_used = -ENOMEM;
>
> I don't think this is a good idea...
>

I agree that it turned out to be wrong. ptrace is now modified to use
the (un)register_user_hw_breakpoint() interfaces directly and not the
worker routines, thereby avoiding all this complexity. Please find the
changes in the new patch.

> > +restore:
> > + /*
> > + * Loop through all the hardware breakpoints, making the
> > + * appropriate changes to each.
> > + */
> > + spin_lock(&hw_breakpoint_lock);
> > +
> > + for (i = 0; i < HB_NUM; i++) {
> > + enabled = decode_dr7(data, i, &len, &type);
> > + bp = thread->hbp[i];
> > +
> > + if (!enabled) {
> > + if (bp) {
> > + __unregister_user_hw_breakpoint(i, tsk);
> > + kfree(bp);
> > + }
> > + continue;
> > + }
> > +
> > + if (!bp) {
> > + rc = -ENOMEM;
> > + if (temp_mem_used != -ENOMEM) {
> > + bp = temp_mem;
>
> What happens if several new breakpoints are present at the same time?
> You'd end up using the same memory for all of them.
>
> > + bp->info.address = thread->debugreg[i];
> > + bp->triggered = ptrace_triggered;
> > + bp->info.len = len;
> > + bp->info.type = type;
> > + temp_mem_used = 1;
> > + rc = __register_user_hw_breakpoint(i, tsk, bp);
> > + if (!rc)
> > + set_tsk_thread_flag(tsk, TIF_DEBUG);
> > + else
> > + kfree(bp);
> > + }
> > + } else
> > + rc = __modify_user_hw_breakpoint(i, tsk, bp);
> > +
> > + if (rc)
> > + break;
> > + }
> > + spin_unlock(&hw_breakpoint_lock);
> > +
> > + /* If anything above failed, restore the original settings */
> > + if (rc < 0) {
> > + data = old_dr7;
> > + goto restore;
>
> And now if something went wrong, you have already freed the memory
> holding the original breakpoint structures. It would be better to
> keep them around until you know they aren't going to be needed.
>
> Alan Stern
>

Thanks again for your comments.

-- K.Prasad