Date:	Wed, 27 May 2009 14:15:38 +0530
From:	"K.Prasad" <prasad@...ux.vnet.ibm.com>
To:	Frederic Weisbecker <fweisbec@...il.com>,
	Roland McGrath <roland@...hat.com>,
	Oleg Nesterov <oleg@...hat.com>
Cc:	Ingo Molnar <mingo@...e.hu>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Alan Stern <stern@...land.harvard.edu>,
	Maneesh Soni <maneesh@...ux.vnet.ibm.com>
Subject: Re: [Patch 08/12] Modify Ptrace routines to access breakpoint
	registers

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@...land.harvard.edu>
> > Signed-off-by: K.Prasad <prasad@...ux.vnet.ibm.com>
> > Signed-off-by: Maneesh Soni <maneesh@...ux.vnet.ibm.com>
> > Reviewed-by: Alan Stern <stern@...land.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists