Message-ID: <20090512171210.GC6033@in.ibm.com>
Date:	Tue, 12 May 2009 22:42:10 +0530
From:	"K.Prasad" <prasad@...ux.vnet.ibm.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Steven Rostedt <rostedt@...dmis.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...e.hu>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Benjamin Herrenschmidt <benh@....ibm.com>,
	maneesh@...ux.vnet.ibm.com, Roland McGrath <roland@...hat.com>
Subject: Re: [Patch 00/12] Hardware Breakpoint Interfaces

On Tue, May 12, 2009 at 10:55:10AM -0400, Alan Stern wrote:
> On Tue, 12 May 2009, K.Prasad wrote:
> 
> > I agree it keeps the code much safer. Here's the relevant code-snippet:
> > 
> > 		if (i >= hbp_kernel_pos)
> > 			bp = per_cpu(this_hbp_kernel[i], cpu);
> > 		else {
> > 			bp = current->thread.hbp[i];
> > 			rc = NOTIFY_DONE;
> > 		}
> > 		/*
> > 		 * bp can be NULL due to lazy debug register switching
> > 		 * for
> > 		 * user-space requests or the exception was triggered in
> > 		 * the
> > 		 * mismatch window when 'hbp_kernel_pos' and
> > 		 * 'this_hbp_kernel[]'
> > 		 * are out-of-sync
> > 		 */
> 
> The comment could be a little more grammatical and more succinct.  For 
> example:
> 
> 		/*
> 		 * bp can be NULL due to lazy debug register switching
> 		 * or to the delay between updates of hbp_kernel_pos 
> 		 * and this_hbp_kernel.
> 		 */
> 

Ok.

> > > Okay.  Let's suppose a single userspace breakpoint has already been
> > > allocated and set up as breakpoint 0.  Now another ptrace call comes
> > > along, in which bp 0 is disabled and bp 1 is requested.  This is your
> > > case (i); the breakpoint structure for bp 0 will be deallocated.  But
> > > maybe something goes wrong with the register_user_hw_breakpoint() call
> > > for bp 1, so we have to restore the old settings.  The code will try to
> > > allocate a new breakpoint structure to hold bp 0; what happens if
> > > that allocation fails?  The user program will know that the write to
> > > DR7 didn't succeed, so it will think the pre-existing breakpoint is
> > > still valid.  But it isn't.
> > > 
> > > If instead you had avoided deallocated the structure for bp 0 until the 
> > > end, then there would be no need to reallocate it during the restore 
> > > and so the restore would succeed.
> > > 
> > > Alan Stern
> > 
> > Thanks for explaining the case so well, I did not foresee it! The
> > ptrace_write_dr7() is modified to perform changes in two-passes. The first
> > one will effect all register/modify operations while the second one will
> > unregister. One issue that I see in this method is if/when virtual debug
> > registers are implemented, we may incorrectly deny a register request
> > for want of physical debug registers (because they have not been free-ed
> > through an earlier unregister operation). Let me know if you think it
> > can be done better. Here's the new ptrace_write_dr7():
> > 
> > 
> > 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, second_pass_reqd = 1;
> > 	unsigned len, type;
> > 	struct hw_breakpoint *bp;
> > 
> > 	data &= ~DR_CONTROL_RESERVED;
> > restore:
> > 	/*
> > 	 * Loop through all the hardware breakpoints, making the
> > 	 * appropriate changes to each.
> > 	 */
> > 	for (i = 0; i < HB_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_reqd)
> > 					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)
> > 					set_tsk_thread_flag(tsk, TIF_DEBUG);
> > 				else
> > 					kfree(bp);
> > 			}
> > 		} else {
> > 			spin_lock_bh(&hw_breakpoint_lock);
> > 			rc = __modify_user_hw_breakpoint(i, tsk, bp);
> > 			spin_unlock_bh(&hw_breakpoint_lock);
> 
> With these changes, __register_user_hw_breakpoint() and 
> __unregister_user_hw_breakpoint() no longer need to be separate 
> routines.  Similarly, __modify_user_hw_breakpoint() can be renamed 
> without the "__" prefix, and it can acquire the spinlock instead of 
> making the caller do the locking.
>
> Then hw_breakpoint_lock can be made static.  There's no need to lock it 
> in ptrace_get_debugreg() any more.
> 

hmmh...The (un)register_user_<> and __(un)register_user_<> break-up
looks so modular and am not sure if combining them would be of any
benefit (and as you might be aware, the compiler can always choose
to inline though). And doing this might not be helpful if we like to
virtualise the debug registers (as in the original implementation) in
the future.

The __modify_user_hw_breakpoint() had the "__" prefix because it had
"int pos" as a parameter, although it can be eliminated by adding a loop
that iterates through the breakpoints of "tsk". If you prefer
modify_user_hw_breakpoint(), I will introduce the same in
kernel/hw_breakpoint.c which acts as a wrapper over __modify_user_<> and
takes the hw_breakpoint_lock.

Let me know what you think on the above.

> > 		}
> > 
> > 		if (rc)
> > 			break;
> > 	}
> > 
> > 	/* If anything above failed, restore the original settings */
> > 	if (rc < 0) {
> > 		data = old_dr7;
> > 		second_pass_reqd = 0;
> > 		goto restore;
> > 	}
> > 	if (second_pass_reqd) {
> > 		/* Enter the second-pass after disabling 'second_pass_reqd' */
> > 		second_pass_reqd = 0;
> > 		goto restore;
> > 	}
> > 	return rc;
> > }
> > 
> > I will repost the patchset along with the changes explained above.
> 
> I think it would be less confusing if you change the name
> "second_pass_reqd" to just "second_pass", setting it to 0 the first
> time through and 1 the second time.  So at the end:
> 
> 	/*
> 	 * 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)
> 			data = old_dr7;
> 		goto restore;
> 	}
> 
> Also, you need to be more careful about the value of rc.  The error 
> code you return should be the error that caused the first pass to fail.  
> Errors or successes during the second pass shouldn't overwrite that 
> value.
> 
> Alan Stern
> 

Ok. Will change the code as suggested. The return values in the above
implementation will be modified in the second pass here:

rc = __modify_user_hw_breakpoint(i, tsk, bp);

I will save the 'rc' value from the first pass in a separate variable
and use that as the return value. Operations in the second pass (such as
unregister/modify are not expected to fail).

Here's the revised code:

/*
 * 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, rc = 0;
	int enabled, second_pass = 0;
	unsigned len, type;
	struct hw_breakpoint *bp;

	data &= ~DR_CONTROL_RESERVED;
restore:
	/*
	 * Loop through all the hardware breakpoints, making the
	 * appropriate changes to each.
	 */
	for (i = 0; i < HB_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)
					set_tsk_thread_flag(tsk, TIF_DEBUG);
				else
					kfree(bp);
			}
		} else {
			spin_lock_bh(&hw_breakpoint_lock);
			rc = __modify_user_hw_breakpoint(i, tsk, bp);
			spin_unlock_bh(&hw_breakpoint_lock);
		}
		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); 
}

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