Re: [Patch 08/12] Modify Ptrace routines to access breakpoint registers

K.Prasad
2009-05-27T08:46:18Z

On Wed, May 27, 2009 at 02:07:04AM +0200, Frederic Weisbecker wrote:
> On Thu, May 21, 2009 at 07:32:28PM +0530, K.Prasad wrote:
> > This patch modifies the ptrace code to use the new wrapper routines around the
> > debug/breakpoint registers.
> >
> > Original-patch-by: Alan Stern <stern@rowland.harvard.edu>
> > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> > Signed-off-by: Maneesh Soni <maneesh@linux.vnet.ibm.com>
> > Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
> > ---
> > -static unsigned long ptrace_get_debugreg(struct task_struct *child, int n)
> > +static int decode_dr7(unsigned long dr7, int bpnum, unsigned *len,
> > + unsigned *type)
> > {
> > - switch (n) {
> > - case 0: return child->thread.debugreg0;
> > - case 1: return child->thread.debugreg1;
> > - case 2: return child->thread.debugreg2;
> > - case 3: return child->thread.debugreg3;
> > - case 6: return child->thread.debugreg6;
> > - case 7: return child->thread.debugreg7;
> > - }
> > - return 0;
> > + int temp = dr7 >> (DR_CONTROL_SHIFT + bpnum * DR_CONTROL_SIZE);
>
>
>
> temp is usually considered as a poor identifier, even for quick
> uses like this.
>
> Why not bp_info?
>
>

Okay.

> > +
> > + *len = (temp & 0xc) | 0x40;
> > + *type = (temp & 0x3) | 0x80;
> > + return (dr7 >> (bpnum * DR_ENABLE_SIZE)) & 0x3;
> > }
> >
> > -static int ptrace_set_debugreg(struct task_struct *child,
> > - int n, unsigned long data)
> > +static void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs)
> > {
> > + struct thread_struct *thread = &(current->thread);
> > int i;
> >
> > - if (unlikely(n == 4 || n == 5))
> > - return -EIO;
> > + /* Store in the virtual DR6 register the fact that the breakpoint
> > + * was hit so the thread's debugger will see it.
> > + */
>
>
> Please use the common comment convention:
>
> /*
> * Comment
> */
>
>

I missed it. Will correct. Thanks.

> > + for (i = 0; i < hbp_kernel_pos; i++)
> > + /*
> > + * We will check bp->info.address against the address stored in
> > + * thread's hbp structure and not debugreg[i]. This is to ensure
> > + * that the corresponding bit for 'i' in DR7 register is enabled
> > + */
> > + if (bp->info.address == thread->hbp[i]->info.address)
> > + break;
> >
> > - if (n < 4 && unlikely(data >= debugreg_addr_limit(child)))
> > - return -EIO;
> > + thread->debugreg6 |= (DR_TRAP0 << i);
> > +}
> >
> > - switch (n) {
> > - case 0: child->thread.debugreg0 = data; break;
> > - case 1: child->thread.debugreg1 = data; break;
> > - case 2: child->thread.debugreg2 = data; break;
> > - case 3: child->thread.debugreg3 = data; break;
> > +/*
> > + * Handle ptrace writes to debug register 7.
> > + */
> > +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, orig_ret = 0, rc = 0;
> > + int enabled, second_pass = 0;
> > + unsigned len, type;
> > + struct hw_breakpoint *bp;
> >
> > - case 6:
> > - if ((data & ~0xffffffffUL) != 0)
> > - return -EIO;
> > - child->thread.debugreg6 = data;
> > - break;
> > + data &= ~DR_CONTROL_RESERVED;
> > +restore:
> > + /*
> > + * Loop through all the hardware breakpoints, making the
> > + * appropriate changes to each.
> > + */
> > + for (i = 0; i < HBP_NUM; i++) {
> > + enabled = decode_dr7(data, i, &len, &type);
> > + bp = thread->hbp[i];
> > +
> > + if (!enabled) {
> > + if (bp) {
> > + /* Don't unregister the breakpoints right-away,
> > + * unless all register_user_hw_breakpoint()
> > + * requests have succeeded. This prevents
> > + * any window of opportunity for debug
> > + * register grabbing by other users.
> > + */
> > + if (!second_pass)
> > + continue;
> > + unregister_user_hw_breakpoint(tsk, bp);
> > + kfree(bp);
> > + }
> > + continue;
> > + }
> > + if (!bp) {
> > + rc = -ENOMEM;
> > + bp = kzalloc(sizeof(struct hw_breakpoint), GFP_KERNEL);
> > + if (bp) {
> > + bp->info.address = thread->debugreg[i];
> > + bp->triggered = ptrace_triggered;
> > + bp->info.len = len;
> > + bp->info.type = type;
> > + rc = register_user_hw_breakpoint(tsk, bp);
> > + if (rc)
> > + kfree(bp);
> > + }
> > + } else
> > + rc = modify_user_hw_breakpoint(tsk, bp);
> > + if (rc)
> > + break;
> > + }
> > + /*
> > + * Make a second pass to free the remaining unused breakpoints
> > + * or to restore the original breakpoints if an error occurred.
> > + */
> > + if (!second_pass) {
> > + second_pass = 1;
> > + if (rc < 0) {
> > + orig_ret = rc;
> > + data = old_dr7;
> > + }
> > + goto restore;
> > + }
> > + return ((orig_ret < 0) ? orig_ret : rc);
> > +}
> >
> > - case 7:
> > - /*
> > - * Sanity-check data. Take one half-byte at once with
> > - * check = (val >> (16 + 4*i)) & 0xf. It contains the
> > - * R/Wi and LENi bits; bits 0 and 1 are R/Wi, and bits
> > - * 2 and 3 are LENi. Given a list of invalid values,
> > - * we do mask |= 1 << invalid_value, so that
> > - * (mask >> check) & 1 is a correct test for invalid
> > - * values.
> > - *
> > - * R/Wi contains the type of the breakpoint /
> > - * watchpoint, LENi contains the length of the watched
> > - * data in the watchpoint case.
> > - *
> > - * The invalid values are:
> > - * - LENi == 0x10 (undefined), so mask |= 0x0f00. [32-bit]
> > - * - R/Wi == 0x10 (break on I/O reads or writes), so
> > - * mask |= 0x4444.
> > - * - R/Wi == 0x00 && LENi != 0x00, so we have mask |=
> > - * 0x1110.
> > - *
> > - * Finally, mask = 0x0f00 | 0x4444 | 0x1110 == 0x5f54.
> > - *
> > - * See the Intel Manual "System Programming Guide",
> > - * 15.2.4
> > - *
> > - * Note that LENi == 0x10 is defined on x86_64 in long
> > - * mode (i.e. even for 32-bit userspace software, but
> > - * 64-bit kernel), so the x86_64 mask value is 0x5454.
> > - * See the AMD manual no. 24593 (AMD64 System Programming)
> > - */
> > -#ifdef CONFIG_X86_32
> > -#define DR7_MASK 0x5f54
> > -#else
> > -#define DR7_MASK 0x5554
> > -#endif
> > - data &= ~DR_CONTROL_RESERVED;
> > - for (i = 0; i < 4; i++)
> > - if ((DR7_MASK >> ((data >> (16 + 4*i)) & 0xf)) & 1)
> > - return -EIO;
> > - child->thread.debugreg7 = data;
> > - if (data)
> > - set_tsk_thread_flag(child, TIF_DEBUG);
> > - else
> > - clear_tsk_thread_flag(child, TIF_DEBUG);
> > - break;
> > +/*
> > + * Handle PTRACE_PEEKUSR calls for the debug register area.
> > + */
> > +unsigned long ptrace_get_debugreg(struct task_struct *tsk, int n)
> > +{
> > + struct thread_struct *thread = &(tsk->thread);
> > + unsigned long val = 0;
> > +
> > + if (n < HBP_NUM)
> > + val = thread->debugreg[n];
> > + else if (n == 6)
> > + val = thread->debugreg6;
> > + else if (n == 7)
> > + val = thread->debugreg7;
> > + return val;
> > +}
> > +
> > +/*
> > + * Handle PTRACE_POKEUSR calls for the debug register area.
> > + */
> > +int ptrace_set_debugreg(struct task_struct *tsk, int n, unsigned long val)
> > +{
> > + struct thread_struct *thread = &(tsk->thread);
> > + int rc = 0;
> > +
> > + /* There are no DR4 or DR5 registers */
> > + if (n == 4 || n == 5)
> > + return -EIO;
> > +
> > + if (n == 6) {
> > + tsk->thread.debugreg6 = val;
> > + goto ret_path;
> > + }
> > + if (n < HBP_NUM) {
> > + if (thread->hbp[n]) {
> > + if (arch_check_va_in_userspace(val,
> > + thread->hbp[n]->info.len) == 0) {
> > + rc = -EIO;
> > + goto ret_path;
> > + }
> > + thread->hbp[n]->info.address = val;
> > + }
> > + thread->debugreg[n] = val;
> > }
> > + /* All that's left is DR7 */
> > + if (n == 7)
> > + rc = ptrace_write_dr7(tsk, val);
> >
> > - return 0;
> > +ret_path:
> > + return rc;
> > }
> >
> > /*
> >
>
>
> I also add the ptrace maintainers in Cc, in case they have
> comments about it.
>
> Roland, Oleg, this patch is part of the hardware breakpoint
> generic interface integration from Prasad based on an older
> work from Alan Stern.
>
> The following patches may help you to better understand
> the new API:
>
> (generic headers) [Patch 01/12] Prepare the code for Hardware Breakpoint interfaces
> (generic Api) [Patch 02/12] Introducing generic hardware breakpoint handler interfaces
> (arch dependent side) [Patch 03/12] x86 architecture implementation of Hardware Breakpoint interfaces
>
> Thanks.
>

Hi Roland/Oleg,
Adding to what Frederic has stated, the ptrace patches do not
modify the behaviour as seen by the end-user. The ptrace_set_debugreg()
behaviour on registers DR0 - DR3 is the same as the existing one. For
DR7, a new function ptrace_write_dr7() is in place and most error returns
are confined to this function (there are a few new ones like -ENOMEM).

If a ptrace write operation on DR7 fails, any partially written values
on the register is restored to a state that existed before the ptrace
call along with an error return. This enables the user-space application
to safely recover from the error.

Hope that the changes on ptrace_<set><get>_debugreg() look acceptable to
you.

Thanks,
K.Prasad