Re: [Patch 00/12] Hardware Breakpoint Interfaces

[Alan Stern]

On Mon, 11 May 2009, K.Prasad wrote:

> > Still, in hw_breakpoint_handler(), perhaps the "if (i >= 
> > hbp_kernel_pos)" path should also check for !bp.  Just to be safe.  In 
> > other words, move the 
> > 
> > +			if (!bp)
> > +				continue;
> > +			rc = NOTIFY_DONE;
> > 
> > lines outside the "if" statement.
> > 
> > 
> 
> I don't see where the out-of-sync situation between hbp_kernel_pos and
> this_hbp_kernel[] can cause problem. Our concern is when an exception
> arises in the small time-window starting at the time when hbp_kernel[]
> is updated in common code and ends when exceptions are disabled through
> the IPI routine arch_update_kernel_hw_breakpoint. Consider these two
> cases now:
> 
> i)register_kernel_hw_breakpoint() - Although hbp_kernel_pos is
> decremented before the IPI, no new exceptions will arise because of the
> same on a CPU where this_hbp_kernel[] is not synced because the physical
> debug registers are not yet set. So, the 'i' in "if (i >= hbp_kernel_pos)"
> cannot belong to the newly registered breakpoint. If it is for the new
> breakpoint, then it is on a CPU where this_hbp_kernel[] is synced with
> hbp_kernel[].
> 
> ii)unregister_kernel_hw_breakpoint() - hbp_kernel_pos is incremented
> after all the IPIs have finished, so hbp_kernel_pos still points to the
> old value whose hbp_kernel[] is NULL. However in "if (i >= hbp_kernel_pos)"
> if 'i' points to the 'pending-delete' breakpoint, then 'bp' is derived
> from this_hbp_kernel[] which contains the old value. The callback
> bp->triggered() is invoked and the exception returns fine.
> 
> Let me know if I am missing any possibility leading to incorrect
> behaviour. 

Your analysis is correct, but to me it feels a little fragile.  Future
changes to the code might cause an unexpected interaction.  Moving that
test won't hurt anything and it will make the handler slightly more
robust.


> > I still think it would be a good idea to avoid freeing any breakpoint
> > structures until you know for certain they aren't needed.  Otherwise
> > you might find that you're unable to allocate memory to re-create a
> > structure that was already present.
> > 
> > Alan Stern
> >
> 
> I agree that we shouldn't free memory temporarily that would be needed
> later, as memory may not be available. However such a situation does not
> arise in ptrace_write_dr7() after the re-write using
> (un)register_user_hw_breakpoint(). kfree(bp) is done when
> i) an existing breakpoint is disabled
> ii) a breakpoint is requested but register_user_hw_breakpoint() routine
> fails for some reasons. Hence the breakpoint structure kzalloc()'ed
> afresh is kfree()'ed.
> iii) In case of modifications to the type of breakpoint (such as
> read<-->write), the breakpoint structure is not de-allocated and this is
> where __modify_user_hw_breakpoint() helps us. There is no opportunity
> window where the breakpoint can be grabbed by other requests when a
> modification is done.
> 
> Let me know if you think there's still some discrepancy here.

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

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