Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII

["K.Prasad" <prasad@linux.vnet.ibm.com>]

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:
> 
> > 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