On Mon, Jun 07, 2010 at 09:25:59PM +1000, Paul Mackerras wrote:

On Mon, Jun 07, 2010 at 12:33:51PM +0530, K.Prasad wrote:

quoted

Given that 'ptrace_bps' is used only for ptrace originated breakpoints
and that we return early i.e. before detecting extraneous interrupts
in hw_breakpoint_handler() (as shown above) they shouldn't overlap each
other. The following comment in hw_breakpoint_handler() explains the
same.
		/*
		 * To prevent invocation of perf_event_bp(), we shall overload
		 * thread.ptrace_bps[] pointer (unused for non-ptrace
		 * exceptions) to flag an extraneous interrupt which must be
		 * skipped.
		 */

My point is that while we are using ptrace_bps[0] to mark a non-ptrace
breakpoint that we're single-stepping, some other process could be
ptracing this process and could get into ptrace_set_debugreg() and
would think that the process already has a ptrace breakpoint and call
modify_user_hw_breakpoint() when it should be calling
register_user_hw_breakpoint().  Or this process could die and so we
call flush_ptrace_hw_breakpoint() and it incorrectly thinks we have a
ptrace breakpoint.

If there is a reason why we can be quite sure that while we are using
current->thread.ptrace_bps[0] in this way, ptrace_set_debugreg() can
never get called with this task as the ptracee, and nor can
flush_ptrace_hw_breakpoint() get called on this task, then maybe it's
safe.  But it's not at all obviously safe.  So I'd very much rather we
just use an extra flag somewhere, that isn't used elsewhere for
anything else, so we can convince ourselves that it is all correct
without having to look at lots of different pieces of code.

There are 3 bytes of padding in struct arch_hw_breakpoint; couldn't we
use one of them as a "not really hit" flag?

Paul.
_______________________________________________

I get your reasoning now; ptrace_bps[] re-use will cause failures under
these circumstances. I've sent a new version of the patchset which adds
a new flag in 'struct arch_hw_breakpoint' (I was always thinking of
'struct thread_struct' before and was scared to introduce another new
member in it, thereby leading me to incorrectly optimise using ptrace_bps)
to flag extraneous_interrupt (Given that it's your idea I've added your
signed-off too).

Kindly let me know your comments, if any.

Thanks,
K.Prasad