Messages in this thread | | | Date | Thu, 16 Apr 2009 17:19:54 -0400 (EDT) | From | Alan Stern <> | Subject | Re: [Patch 00/11] Hardware Breakpoint interfaces |
| |
On Tue, 7 Apr 2009, K.Prasad wrote:
> Hi Alan, > I am sending you the patches with the changes mentioned in the > Changelog below. Please read the patches in conjunction with my replies > sent to your previous comments. > > Let me know your thoughts on this.
Sorry to take so long on this... lots of other things keep cropping up.
Mostly it looks okay; I noticed only a few problems. Comments inline below.
Alan Stern
> --- arch/x86/include/asm/processor.h.orig > +++ arch/x86/include/asm/processor.h > @@ -429,8 +430,11 @@ struct thread_struct { > unsigned long debugreg1; > unsigned long debugreg2; > unsigned long debugreg3; > + unsigned long debugreg[HB_NUM];
You forgot to get rid of debugreg1 - debugreg3!
> +static int validate_settings(struct hw_breakpoint *bp, struct task_struct *tsk) > +{ > + int ret; > + > + if (!bp) > + return -EINVAL;
Is this test really needed?
> + > + ret = arch_validate_hwbkpt_settings(bp, tsk); > + if (ret < 0) > + goto err; > + > + /* Check that the virtual address is in the proper range */ > + if (tsk) { > + if (!arch_check_va_in_userspace(bp->info.address, bp->info.len)) > + return -EFAULT; > + } else { > + if (!arch_check_va_in_kernelspace(bp->info.address, > + bp->info.len)) > + return -EFAULT; > + } > + err: > + return ret; > +}
At this point, almost nothing in the routine is arch-independent. You might as well move everything into the arch-specific code and eliminate the validate_settings() routine.
> +void unregister_kernel_hw_breakpoint(struct hw_breakpoint *bp) > +{ > + int i, j; > + > + mutex_lock(&hw_breakpoint_mutex); > + > + /* 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) { > + mutex_unlock(&hw_breakpoint_mutex); > + 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]; > + > + hbp_kernel[hbp_kernel_pos] = 0;
s/0/NULL/
> + on_each_cpu(arch_update_kernel_hw_breakpoints, NULL, 0); > + hbp_kernel_pos++;
Don't you want to increment hbp_kernel_pos _before_ calling on_each_cpu()? Otherwise you're telling the other CPUs to install an empty breakpoint.
> +/* Unmasked kernel DR7 value */ > +static unsigned long kdr7; > +static const unsigned long kdr7_masks[HB_NUM] = { > + 0xffff00ff, /* Same for 3, 2, 1, 0 */ > + 0xfff000fc, /* Same for 3, 2, 1 */ > + 0xff0000f0, /* Same for 3, 2 */ > + 0xf00f00c0 /* LEN3, R/W3, G3, L3 */ > +};
It looks like you took out the first line of assignments. Isn't kdr7_masks supposed to contain HB_NUM + 1 elements? Not to mention that reordering the lines has mixed up the comments.
> +void arch_update_kernel_hw_breakpoints(void *unused) > +{ > + struct hw_breakpoint *bp; > + unsigned long dr7; > + int i; > + > + /* Check if there is nothing to update */ > + if (hbp_kernel_pos == HB_NUM) > + return;
This is wrong; you have to set DR7 to 0 first. Might as well leave out this test and just go through the whole routine. The "for" loop will be skipped entirely if hbp_kernel_pos == HB_NUM, so you don't save much by returning early.
Ah, now I see this is why you removed a line from kdr7_masks.
> + > + 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]; > + > + 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); > + } > + } > + > + dr7 |= kdr7; > + > + /* No need to set DR6 */ > + set_debugreg(dr7, 7); > +}
> +int __kprobes hw_breakpoint_handler(struct die_args *args) > +{ > + int i, rc = NOTIFY_STOP; > + struct hw_breakpoint *bp; > + /* The DR6 value is stored in args->err */ > + unsigned long dr7, dr6 = args->err; > + > + /* > + * We are here because BT, BS or BD bit is set and no trap bits are set > + * in dr6 register. Do an early return > + */
I don't like such comments. It says that BT, BS, or BD is set, but they don't necessarily have to be. You should say:
/* Do an early return if no trap bits are set in DR6 */
> + if ((dr6 & (DR_TRAP0|DR_TRAP1|DR_TRAP2|DR_TRAP3)) == 0) > + return NOTIFY_DONE; > + > + get_debugreg(dr7, 7); > + > + /* Disable breakpoints during exception handling */ > + set_debugreg(0UL, 7); > + /* > + * Assert that local interrupts are disabled > + * Reset the DRn bits in the virtualized register value. > + * The ptrace trigger routine will add in whatever is needed. > + */ > + current->thread.debugreg6 &= ~(DR_TRAP0|DR_TRAP1|DR_TRAP2|DR_TRAP3); > + > + /* Lazy debug register switching */ > + if (per_cpu(last_debugged_task, get_cpu()) != current) { > + switch_to_none_hw_breakpoint(); > + put_cpu_no_resched(); > + }
Once again, the get_debugreg and set_debugreg statements above should go here instead.
> + > + /* Handle all the breakpoints that were triggered */ > + for (i = 0; i < HB_NUM; ++i) { > + if (likely(!(dr6 & (DR_TRAP0 << i)))) > + continue; > + /* > + * Find the corresponding hw_breakpoint structure and > + * invoke its triggered callback. > + */ > + if (i >= hbp_kernel_pos) > + bp = hbp_kernel[i]; > + else { > + bp = current->thread.hbp[i]; > + if (!bp) { > + /* False alarm due to lazy DR switching */ > + continue; > + } > + } > + (bp->triggered)(bp, args->regs); > + /* > + * We have more work to do (send signals) if the breakpoint is > + * triggered due to user-space address, or if exceptions other > + * than breakpoint (such as single-stepping) are triggered. > + */ > + if (arch_check_va_in_userspace(bp->info.address, bp->info.len)) > + rc = NOTIFY_DONE;
This comment and test are more complicated than they need to be. Just put "rc = NOTIFY_DONE" (with a comment) in the "else" clause above.
> + } > + if (dr6 & ~(DR_TRAP0|DR_TRAP1|DR_TRAP2|DR_TRAP3)) > + rc = NOTIFY_DONE; > + set_debugreg(dr7, 7); > + return rc; > +}
> dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code) > { > struct task_struct *tsk = current; > - unsigned long condition; > + unsigned long dr6; > int si_code; > > - get_debugreg(condition, 6); > + get_debugreg(dr6, 6); > + set_debugreg(0, 6); /* DR6 may or may not be cleared by the CPU */ > > /* Catch kmemcheck conditions first of all! */ > - if (condition & DR_STEP && kmemcheck_trap(regs)) > + if ((dr6 & DR_STEP && kmemcheck_trap(regs)) && > + !(dr6 & (DR_TRAP0|DR_TRAP1|DR_TRAP2|DR_TRAP3))) > return;
Should this test go before the "set_debugreg(0, 6)" statement?
| |