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 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
