Re: [PATCH] ARM: hw_breakpoint: Handle inexact watchpoint addresses

[Will Deacon]

On Mon, Dec 02, 2019 at 08:36:19AM -0800, Doug Anderson wrote:
> On Wed, Nov 20, 2019 at 11:18 AM Will Deacon <will@xxxxxxxxxx> wrote:
> >
> > On Sat, Oct 19, 2019 at 11:12:26AM -0700, Douglas Anderson wrote:
> > > This is commit fdfeff0f9e3d ("arm64: hw_breakpoint: Handle inexact
> > > watchpoint addresses") but ported to arm32, which has the same
> > > problem.
> > >
> > > This problem was found by Android CTS tests, notably the
> > > "watchpoint_imprecise" test [1].  I tested locally against a copycat
> > > (simplified) version of the test though.
> > >
> > > [1] https://android.googlesource.com/platform/bionic/+/master/tests/sys_ptrace_test.cpp
> > >
> > > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > > ---
> > >
> > >  arch/arm/kernel/hw_breakpoint.c | 96 ++++++++++++++++++++++++---------
> > >  1 file changed, 70 insertions(+), 26 deletions(-)
> >
> > Sorry for taking so long to look at this. After wrapping my head around the
> > logic again
> 
> Yeah.  It was a little weird and (unfortunately) arbitrarily different
> in some places compared to the arm64 code.
> 
> 
> > I think it looks fine, so please put it into the patch system
> > with my Ack:
> >
> > Acked-by: Will Deacon <will@xxxxxxxxxx>
> 
> Thanks!  Submitted as:
> 
> https://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8944/1
> 
> 
> > One interesting difference between the implementation here and the arm64
> > code is that I think if you have multiple watchpoints, all of which fire
> > with a distance != 0, then arm32 will actually report them all whereas
> > you'd only get one on arm64.
> 
> Are you sure about that?  The "/* No exact match found. */" code is
> outside the for loop so it should only be able to trigger for exactly
> one breakpoint, no?

I didn't test it, but I think that we'll convert the first watchpoint into a
mismatch breakpoint on arm32 and then when we resume execution, we'll hit
the subsequent watchpoint and so on until we actually manage to "step" the
instruction. On arm64, we'll use hardware step directly and therefore
disable all watchpoints prior to performing the step.

Will