[PATCH] ARM: hw_breakpoint: Handle inexact watchpoint addresses

[PATCH] ARM: hw_breakpoint: Handle inexact watchpoint addresses

[Douglas Anderson]

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@chromium.org>
---

 arch/arm/kernel/hw_breakpoint.c | 96 ++++++++++++++++++++++++---------
 1 file changed, 70 insertions(+), 26 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index b0c195e3a06d..d394878409db 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -680,26 +680,62 @@ static void disable_single_step(struct perf_event *bp)
 	arch_install_hw_breakpoint(bp);
 }
 
+/*
+ * Arm32 hardware does not always report a watchpoint hit address that matches
+ * one of the watchpoints set. It can also report an address "near" the
+ * watchpoint if a single instruction access both watched and unwatched
+ * addresses. There is no straight-forward way, short of disassembling the
+ * offending instruction, to map that address back to the watchpoint. This
+ * function computes the distance of the memory access from the watchpoint as a
+ * heuristic for the likelyhood that a given access triggered the watchpoint.
+ *
+ * See this same function in the arm64 platform code, which has the same
+ * problem.
+ *
+ * The function returns the distance of the address from the bytes watched by
+ * the watchpoint. In case of an exact match, it returns 0.
+ */
+static u32 get_distance_from_watchpoint(unsigned long addr, u32 val,
+					struct arch_hw_breakpoint_ctrl *ctrl)
+{
+	u32 wp_low, wp_high;
+	u32 lens, lene;
+
+	lens = __ffs(ctrl->len);
+	lene = __fls(ctrl->len);
+
+	wp_low = val + lens;
+	wp_high = val + lene;
+	if (addr < wp_low)
+		return wp_low - addr;
+	else if (addr > wp_high)
+		return addr - wp_high;
+	else
+		return 0;
+}
+
 static void watchpoint_handler(unsigned long addr, unsigned int fsr,
 			       struct pt_regs *regs)
 {
-	int i, access;
-	u32 val, ctrl_reg, alignment_mask;
+	int i, access, closest_match = 0;
+	u32 min_dist = -1, dist;
+	u32 val, ctrl_reg;
 	struct perf_event *wp, **slots;
 	struct arch_hw_breakpoint *info;
 	struct arch_hw_breakpoint_ctrl ctrl;
 
 	slots = this_cpu_ptr(wp_on_reg);
 
+	/*
+	 * Find all watchpoints that match the reported address. If no exact
+	 * match is found. Attribute the hit to the closest watchpoint.
+	 */
+	rcu_read_lock();
 	for (i = 0; i < core_num_wrps; ++i) {
-		rcu_read_lock();
-
 		wp = slots[i];
-
 		if (wp == NULL)
-			goto unlock;
+			continue;
 
-		info = counter_arch_bp(wp);
 		/*
 		 * The DFAR is an unknown value on debug architectures prior
 		 * to 7.1. Since we only allow a single watchpoint on these
@@ -708,33 +744,31 @@ static void watchpoint_handler(unsigned long addr, unsigned int fsr,
 		 */
 		if (debug_arch < ARM_DEBUG_ARCH_V7_1) {
 			BUG_ON(i > 0);
+			info = counter_arch_bp(wp);
 			info->trigger = wp->attr.bp_addr;
 		} else {
-			if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
-				alignment_mask = 0x7;
-			else
-				alignment_mask = 0x3;
-
-			/* Check if the watchpoint value matches. */
-			val = read_wb_reg(ARM_BASE_WVR + i);
-			if (val != (addr & ~alignment_mask))
-				goto unlock;
-
-			/* Possible match, check the byte address select. */
-			ctrl_reg = read_wb_reg(ARM_BASE_WCR + i);
-			decode_ctrl_reg(ctrl_reg, &ctrl);
-			if (!((1 << (addr & alignment_mask)) & ctrl.len))
-				goto unlock;
-
 			/* Check that the access type matches. */
 			if (debug_exception_updates_fsr()) {
 				access = (fsr & ARM_FSR_ACCESS_MASK) ?
 					  HW_BREAKPOINT_W : HW_BREAKPOINT_R;
 				if (!(access & hw_breakpoint_type(wp)))
-					goto unlock;
+					continue;
 			}
 
+			val = read_wb_reg(ARM_BASE_WVR + i);
+			ctrl_reg = read_wb_reg(ARM_BASE_WCR + i);
+			decode_ctrl_reg(ctrl_reg, &ctrl);
+			dist = get_distance_from_watchpoint(addr, val, &ctrl);
+			if (dist < min_dist) {
+				min_dist = dist;
+				closest_match = i;
+			}
+			/* Is this an exact match? */
+			if (dist != 0)
+				continue;
+
 			/* We have a winner. */
+			info = counter_arch_bp(wp);
 			info->trigger = addr;
 		}
 
@@ -748,10 +782,20 @@ static void watchpoint_handler(unsigned long addr, unsigned int fsr,
 		 */
 		if (is_default_overflow_handler(wp))
 			enable_single_step(wp, instruction_pointer(regs));
+	}
 
-unlock:
-		rcu_read_unlock();
+	if (min_dist > 0 && min_dist != -1) {
+		/* No exact match found. */
+		wp = slots[closest_match];
+		info = counter_arch_bp(wp);
+		info->trigger = addr;
+		pr_debug("watchpoint fired: address = 0x%x\n", info->trigger);
+		perf_bp_event(wp, regs);
+		if (is_default_overflow_handler(wp))
+			enable_single_step(wp, instruction_pointer(regs));
 	}
+
+	rcu_read_unlock();
 }
 
 static void watchpoint_single_step_handler(unsigned long pc)
-- 
2.23.0.866.gb869b98d4c-goog

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

[Matthias Kaehlcke]

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@chromium.org>
> ---
> 
>  arch/arm/kernel/hw_breakpoint.c | 96 ++++++++++++++++++++++++---------
>  1 file changed, 70 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index b0c195e3a06d..d394878409db 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -680,26 +680,62 @@ static void disable_single_step(struct perf_event *bp)
>  	arch_install_hw_breakpoint(bp);
>  }
>  
> +/*
> + * Arm32 hardware does not always report a watchpoint hit address that matches
> + * one of the watchpoints set. It can also report an address "near" the
> + * watchpoint if a single instruction access both watched and unwatched
> + * addresses. There is no straight-forward way, short of disassembling the
> + * offending instruction, to map that address back to the watchpoint. This
> + * function computes the distance of the memory access from the watchpoint as a
> + * heuristic for the likelyhood that a given access triggered the watchpoint.
> + *
> + * See this same function in the arm64 platform code, which has the same
> + * problem.
> + *
> + * The function returns the distance of the address from the bytes watched by
> + * the watchpoint. In case of an exact match, it returns 0.
> + */
> +static u32 get_distance_from_watchpoint(unsigned long addr, u32 val,
> +					struct arch_hw_breakpoint_ctrl *ctrl)
> +{
> +	u32 wp_low, wp_high;
> +	u32 lens, lene;
> +
> +	lens = __ffs(ctrl->len);

Doesn't this always end up with 'lens == 0'? IIUC ctrl->len can have
the values ARM_BREAKPOINT_LEN_{1,2,4,8}:

#define ARM_BREAKPOINT_LEN_1	0x1
#define ARM_BREAKPOINT_LEN_2	0x3
#define ARM_BREAKPOINT_LEN_4	0xf
#define ARM_BREAKPOINT_LEN_8	0xff

> +	lene = __fls(ctrl->len);
> +
> +	wp_low = val + lens;
> +	wp_high = val + lene;

First I thought these values are off by one, but in difference to
ffs() from glibc the kernel functions start with index 0, instead
of using zero as 'no bit set'.

> +	if (addr < wp_low)
> +		return wp_low - addr;
> +	else if (addr > wp_high)
> +		return addr - wp_high;
> +	else
> +		return 0;
> +}
> +
>  static void watchpoint_handler(unsigned long addr, unsigned int fsr,
>  			       struct pt_regs *regs)
>  {
> -	int i, access;
> -	u32 val, ctrl_reg, alignment_mask;
> +	int i, access, closest_match = 0;
> +	u32 min_dist = -1, dist;
> +	u32 val, ctrl_reg;
>  	struct perf_event *wp, **slots;
>  	struct arch_hw_breakpoint *info;
>  	struct arch_hw_breakpoint_ctrl ctrl;
>  
>  	slots = this_cpu_ptr(wp_on_reg);
>  
> +	/*
> +	 * Find all watchpoints that match the reported address. If no exact
> +	 * match is found. Attribute the hit to the closest watchpoint.
> +	 */
> +	rcu_read_lock();
>  	for (i = 0; i < core_num_wrps; ++i) {
> -		rcu_read_lock();
> -
>  		wp = slots[i];
> -
>  		if (wp == NULL)
> -			goto unlock;
> +			continue;
>  
> -		info = counter_arch_bp(wp);
>  		/*
>  		 * The DFAR is an unknown value on debug architectures prior
>  		 * to 7.1. Since we only allow a single watchpoint on these
> @@ -708,33 +744,31 @@ static void watchpoint_handler(unsigned long addr, unsigned int fsr,
>  		 */
>  		if (debug_arch < ARM_DEBUG_ARCH_V7_1) {
>  			BUG_ON(i > 0);
> +			info = counter_arch_bp(wp);
>  			info->trigger = wp->attr.bp_addr;
>  		} else {
> -			if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
> -				alignment_mask = 0x7;
> -			else
> -				alignment_mask = 0x3;
> -
> -			/* Check if the watchpoint value matches. */
> -			val = read_wb_reg(ARM_BASE_WVR + i);
> -			if (val != (addr & ~alignment_mask))
> -				goto unlock;
> -
> -			/* Possible match, check the byte address select. */
> -			ctrl_reg = read_wb_reg(ARM_BASE_WCR + i);
> -			decode_ctrl_reg(ctrl_reg, &ctrl);
> -			if (!((1 << (addr & alignment_mask)) & ctrl.len))
> -				goto unlock;
> -
>  			/* Check that the access type matches. */
>  			if (debug_exception_updates_fsr()) {
>  				access = (fsr & ARM_FSR_ACCESS_MASK) ?
>  					  HW_BREAKPOINT_W : HW_BREAKPOINT_R;
>  				if (!(access & hw_breakpoint_type(wp)))
> -					goto unlock;
> +					continue;
>  			}
>  
> +			val = read_wb_reg(ARM_BASE_WVR + i);
> +			ctrl_reg = read_wb_reg(ARM_BASE_WCR + i);
> +			decode_ctrl_reg(ctrl_reg, &ctrl);
> +			dist = get_distance_from_watchpoint(addr, val, &ctrl);
> +			if (dist < min_dist) {
> +				min_dist = dist;
> +				closest_match = i;
> +			}
> +			/* Is this an exact match? */
> +			if (dist != 0)
> +				continue;
> +
>  			/* We have a winner. */
> +			info = counter_arch_bp(wp);
>  			info->trigger = addr;

Unless we care about using the 'last' watchpoint in case multiple WPs have
the same address I think it would be clearer to change the above to:

	       	       	if (dist == 0) {
				/* We have a winner. */
				info = counter_arch_bp(wp);
				info->trigger = addr;
				break;
			}

>  		}
>  
> @@ -748,10 +782,20 @@ static void watchpoint_handler(unsigned long addr, unsigned int fsr,
>  		 */
>  		if (is_default_overflow_handler(wp))
>  			enable_single_step(wp, instruction_pointer(regs));
> +	}
>  
> -unlock:
> -		rcu_read_unlock();
> +	if (min_dist > 0 && min_dist != -1) {
> +		/* No exact match found. */
> +		wp = slots[closest_match];
> +		info = counter_arch_bp(wp);
> +		info->trigger = addr;
> +		pr_debug("watchpoint fired: address = 0x%x\n", info->trigger);
> +		perf_bp_event(wp, regs);
> +		if (is_default_overflow_handler(wp))
> +			enable_single_step(wp, instruction_pointer(regs));
>  	}
> +
> +	rcu_read_unlock();
>  }
>  
>  static void watchpoint_single_step_handler(unsigned long pc)
> -- 
> 2.23.0.866.gb869b98d4c-goog
> 

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

[Doug Anderson]

Hi,

On Mon, Oct 21, 2019 at 11:47 AM Matthias Kaehlcke <mka@chromium.org> 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@chromium.org>
> > ---
> >
> >  arch/arm/kernel/hw_breakpoint.c | 96 ++++++++++++++++++++++++---------
> >  1 file changed, 70 insertions(+), 26 deletions(-)
> >
> > diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> > index b0c195e3a06d..d394878409db 100644
> > --- a/arch/arm/kernel/hw_breakpoint.c
> > +++ b/arch/arm/kernel/hw_breakpoint.c
> > @@ -680,26 +680,62 @@ static void disable_single_step(struct perf_event *bp)
> >       arch_install_hw_breakpoint(bp);
> >  }
> >
> > +/*
> > + * Arm32 hardware does not always report a watchpoint hit address that matches
> > + * one of the watchpoints set. It can also report an address "near" the
> > + * watchpoint if a single instruction access both watched and unwatched
> > + * addresses. There is no straight-forward way, short of disassembling the
> > + * offending instruction, to map that address back to the watchpoint. This
> > + * function computes the distance of the memory access from the watchpoint as a
> > + * heuristic for the likelyhood that a given access triggered the watchpoint.
> > + *
> > + * See this same function in the arm64 platform code, which has the same
> > + * problem.
> > + *
> > + * The function returns the distance of the address from the bytes watched by
> > + * the watchpoint. In case of an exact match, it returns 0.
> > + */
> > +static u32 get_distance_from_watchpoint(unsigned long addr, u32 val,
> > +                                     struct arch_hw_breakpoint_ctrl *ctrl)
> > +{
> > +     u32 wp_low, wp_high;
> > +     u32 lens, lene;
> > +
> > +     lens = __ffs(ctrl->len);
>
> Doesn't this always end up with 'lens == 0'? IIUC ctrl->len can have
> the values ARM_BREAKPOINT_LEN_{1,2,4,8}:
>
> #define ARM_BREAKPOINT_LEN_1    0x1
> #define ARM_BREAKPOINT_LEN_2    0x3
> #define ARM_BREAKPOINT_LEN_4    0xf
> #define ARM_BREAKPOINT_LEN_8    0xff

Yes, but my best guess without digging into the ARM ARM is that the
underlying hardware is more flexible.  I don't think it hurts to
support the flexibility here even if the code creating the breakpoint
never creates one line that.  ...especially since it makes the arm32
and arm64 code match in this way.


> > +     lene = __fls(ctrl->len);
> > +
> > +     wp_low = val + lens;
> > +     wp_high = val + lene;
>
> First I thought these values are off by one, but in difference to
> ffs() from glibc the kernel functions start with index 0, instead
> of using zero as 'no bit set'.

Yes, this took me a while.  If you look at the original commit
fdfeff0f9e3d ("arm64: hw_breakpoint: Handle inexact watchpoint
addresses") this was clearly done on purpose though.  Specifically
note the part of the commit message:

[will: use __ffs instead of ffs - 1]


> > +     if (addr < wp_low)
> > +             return wp_low - addr;
> > +     else if (addr > wp_high)
> > +             return addr - wp_high;
> > +     else
> > +             return 0;
> > +}
> > +
> >  static void watchpoint_handler(unsigned long addr, unsigned int fsr,
> >                              struct pt_regs *regs)
> >  {
> > -     int i, access;
> > -     u32 val, ctrl_reg, alignment_mask;
> > +     int i, access, closest_match = 0;
> > +     u32 min_dist = -1, dist;
> > +     u32 val, ctrl_reg;
> >       struct perf_event *wp, **slots;
> >       struct arch_hw_breakpoint *info;
> >       struct arch_hw_breakpoint_ctrl ctrl;
> >
> >       slots = this_cpu_ptr(wp_on_reg);
> >
> > +     /*
> > +      * Find all watchpoints that match the reported address. If no exact
> > +      * match is found. Attribute the hit to the closest watchpoint.
> > +      */
> > +     rcu_read_lock();
> >       for (i = 0; i < core_num_wrps; ++i) {
> > -             rcu_read_lock();
> > -
> >               wp = slots[i];
> > -
> >               if (wp == NULL)
> > -                     goto unlock;
> > +                     continue;
> >
> > -             info = counter_arch_bp(wp);
> >               /*
> >                * The DFAR is an unknown value on debug architectures prior
> >                * to 7.1. Since we only allow a single watchpoint on these
> > @@ -708,33 +744,31 @@ static void watchpoint_handler(unsigned long addr, unsigned int fsr,
> >                */
> >               if (debug_arch < ARM_DEBUG_ARCH_V7_1) {
> >                       BUG_ON(i > 0);
> > +                     info = counter_arch_bp(wp);
> >                       info->trigger = wp->attr.bp_addr;
> >               } else {
> > -                     if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
> > -                             alignment_mask = 0x7;
> > -                     else
> > -                             alignment_mask = 0x3;
> > -
> > -                     /* Check if the watchpoint value matches. */
> > -                     val = read_wb_reg(ARM_BASE_WVR + i);
> > -                     if (val != (addr & ~alignment_mask))
> > -                             goto unlock;
> > -
> > -                     /* Possible match, check the byte address select. */
> > -                     ctrl_reg = read_wb_reg(ARM_BASE_WCR + i);
> > -                     decode_ctrl_reg(ctrl_reg, &ctrl);
> > -                     if (!((1 << (addr & alignment_mask)) & ctrl.len))
> > -                             goto unlock;
> > -
> >                       /* Check that the access type matches. */
> >                       if (debug_exception_updates_fsr()) {
> >                               access = (fsr & ARM_FSR_ACCESS_MASK) ?
> >                                         HW_BREAKPOINT_W : HW_BREAKPOINT_R;
> >                               if (!(access & hw_breakpoint_type(wp)))
> > -                                     goto unlock;
> > +                                     continue;
> >                       }
> >
> > +                     val = read_wb_reg(ARM_BASE_WVR + i);
> > +                     ctrl_reg = read_wb_reg(ARM_BASE_WCR + i);
> > +                     decode_ctrl_reg(ctrl_reg, &ctrl);
> > +                     dist = get_distance_from_watchpoint(addr, val, &ctrl);
> > +                     if (dist < min_dist) {
> > +                             min_dist = dist;
> > +                             closest_match = i;
> > +                     }
> > +                     /* Is this an exact match? */
> > +                     if (dist != 0)
> > +                             continue;
> > +
> >                       /* We have a winner. */
> > +                     info = counter_arch_bp(wp);
> >                       info->trigger = addr;
>
> Unless we care about using the 'last' watchpoint in case multiple WPs have
> the same address I think it would be clearer to change the above to:
>
>                         if (dist == 0) {
>                                 /* We have a winner. */
>                                 info = counter_arch_bp(wp);
>                                 info->trigger = addr;
>                                 break;
>                         }

Without being an expert on the Hardware Breakpoint API, my
understanding (based on how the old arm32 code worked and how the
existing arm64 code works) is that the API accounts for the fact that
more than one watchpoint can trigger and that we should report on all
of them.

Specifically if you do:

watch 1 byte at 0x1000
watch 1 byte at 0x1003

...and then someone does a single 4-byte write at 0x1000 then both
watchpoints should trigger.  If we do a "break" here then they won't
both trigger.  Also note that the triggering happens below in the
"perf_bp_event(wp, regs)" so with your break I think you'll miss it,
no?

That being said, with my patch we still won't do exactly the right
thing that for an "imprecise" watchpoint.  Specifically if you do:

watch 1 byte at 0x1008
watch 1 byte at 0x100b
write 16 bytes at 0x1000

...then we will _only_ trigger the 0x1008 watchpoint.  ...but that's
the limitation in how the breakpoints work.  You can see this is what
happens because the imprecise stuff is outside the for loop and only
triggers when nothing else did.


-Doug

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

[Matthias Kaehlcke]

On Mon, Oct 21, 2019 at 04:49:48PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Oct 21, 2019 at 11:47 AM Matthias Kaehlcke <mka@chromium.org> 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@chromium.org>
> > > ---
> > >
> > >  arch/arm/kernel/hw_breakpoint.c | 96 ++++++++++++++++++++++++---------
> > >  1 file changed, 70 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> > > index b0c195e3a06d..d394878409db 100644
> > > --- a/arch/arm/kernel/hw_breakpoint.c
> > > +++ b/arch/arm/kernel/hw_breakpoint.c
> > > @@ -680,26 +680,62 @@ static void disable_single_step(struct perf_event *bp)
> > >       arch_install_hw_breakpoint(bp);
> > >  }
> > >
> > > +/*
> > > + * Arm32 hardware does not always report a watchpoint hit address that matches
> > > + * one of the watchpoints set. It can also report an address "near" the
> > > + * watchpoint if a single instruction access both watched and unwatched
> > > + * addresses. There is no straight-forward way, short of disassembling the
> > > + * offending instruction, to map that address back to the watchpoint. This
> > > + * function computes the distance of the memory access from the watchpoint as a
> > > + * heuristic for the likelyhood that a given access triggered the watchpoint.
> > > + *
> > > + * See this same function in the arm64 platform code, which has the same
> > > + * problem.
> > > + *
> > > + * The function returns the distance of the address from the bytes watched by
> > > + * the watchpoint. In case of an exact match, it returns 0.
> > > + */
> > > +static u32 get_distance_from_watchpoint(unsigned long addr, u32 val,
> > > +                                     struct arch_hw_breakpoint_ctrl *ctrl)
> > > +{
> > > +     u32 wp_low, wp_high;
> > > +     u32 lens, lene;
> > > +
> > > +     lens = __ffs(ctrl->len);
> >
> > Doesn't this always end up with 'lens == 0'? IIUC ctrl->len can have
> > the values ARM_BREAKPOINT_LEN_{1,2,4,8}:
> >
> > #define ARM_BREAKPOINT_LEN_1    0x1
> > #define ARM_BREAKPOINT_LEN_2    0x3
> > #define ARM_BREAKPOINT_LEN_4    0xf
> > #define ARM_BREAKPOINT_LEN_8    0xff
> 
> Yes, but my best guess without digging into the ARM ARM is that the
> underlying hardware is more flexible.  I don't think it hurts to
> support the flexibility here even if the code creating the breakpoint
> never creates one line that.  ...especially since it makes the arm32
> and arm64 code match in this way.

ok

> > > +     lene = __fls(ctrl->len);
> > > +
> > > +     wp_low = val + lens;
> > > +     wp_high = val + lene;
> >
> > First I thought these values are off by one, but in difference to
> > ffs() from glibc the kernel functions start with index 0, instead
> > of using zero as 'no bit set'.
> 
> Yes, this took me a while.  If you look at the original commit
> fdfeff0f9e3d ("arm64: hw_breakpoint: Handle inexact watchpoint
> addresses") this was clearly done on purpose though.  Specifically
> note the part of the commit message:
> 
> [will: use __ffs instead of ffs - 1]
> 
> 
> > > +     if (addr < wp_low)
> > > +             return wp_low - addr;
> > > +     else if (addr > wp_high)
> > > +             return addr - wp_high;
> > > +     else
> > > +             return 0;
> > > +}
> > > +
> > >  static void watchpoint_handler(unsigned long addr, unsigned int fsr,
> > >                              struct pt_regs *regs)
> > >  {
> > > -     int i, access;
> > > -     u32 val, ctrl_reg, alignment_mask;
> > > +     int i, access, closest_match = 0;
> > > +     u32 min_dist = -1, dist;
> > > +     u32 val, ctrl_reg;
> > >       struct perf_event *wp, **slots;
> > >       struct arch_hw_breakpoint *info;
> > >       struct arch_hw_breakpoint_ctrl ctrl;
> > >
> > >       slots = this_cpu_ptr(wp_on_reg);
> > >
> > > +     /*
> > > +      * Find all watchpoints that match the reported address. If no exact
> > > +      * match is found. Attribute the hit to the closest watchpoint.
> > > +      */
> > > +     rcu_read_lock();
> > >       for (i = 0; i < core_num_wrps; ++i) {
> > > -             rcu_read_lock();
> > > -
> > >               wp = slots[i];
> > > -
> > >               if (wp == NULL)
> > > -                     goto unlock;
> > > +                     continue;
> > >
> > > -             info = counter_arch_bp(wp);
> > >               /*
> > >                * The DFAR is an unknown value on debug architectures prior
> > >                * to 7.1. Since we only allow a single watchpoint on these
> > > @@ -708,33 +744,31 @@ static void watchpoint_handler(unsigned long addr, unsigned int fsr,
> > >                */
> > >               if (debug_arch < ARM_DEBUG_ARCH_V7_1) {
> > >                       BUG_ON(i > 0);
> > > +                     info = counter_arch_bp(wp);
> > >                       info->trigger = wp->attr.bp_addr;
> > >               } else {
> > > -                     if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
> > > -                             alignment_mask = 0x7;
> > > -                     else
> > > -                             alignment_mask = 0x3;
> > > -
> > > -                     /* Check if the watchpoint value matches. */
> > > -                     val = read_wb_reg(ARM_BASE_WVR + i);
> > > -                     if (val != (addr & ~alignment_mask))
> > > -                             goto unlock;
> > > -
> > > -                     /* Possible match, check the byte address select. */
> > > -                     ctrl_reg = read_wb_reg(ARM_BASE_WCR + i);
> > > -                     decode_ctrl_reg(ctrl_reg, &ctrl);
> > > -                     if (!((1 << (addr & alignment_mask)) & ctrl.len))
> > > -                             goto unlock;
> > > -
> > >                       /* Check that the access type matches. */
> > >                       if (debug_exception_updates_fsr()) {
> > >                               access = (fsr & ARM_FSR_ACCESS_MASK) ?
> > >                                         HW_BREAKPOINT_W : HW_BREAKPOINT_R;
> > >                               if (!(access & hw_breakpoint_type(wp)))
> > > -                                     goto unlock;
> > > +                                     continue;
> > >                       }
> > >
> > > +                     val = read_wb_reg(ARM_BASE_WVR + i);
> > > +                     ctrl_reg = read_wb_reg(ARM_BASE_WCR + i);
> > > +                     decode_ctrl_reg(ctrl_reg, &ctrl);
> > > +                     dist = get_distance_from_watchpoint(addr, val, &ctrl);
> > > +                     if (dist < min_dist) {
> > > +                             min_dist = dist;
> > > +                             closest_match = i;
> > > +                     }
> > > +                     /* Is this an exact match? */
> > > +                     if (dist != 0)
> > > +                             continue;
> > > +
> > >                       /* We have a winner. */
> > > +                     info = counter_arch_bp(wp);
> > >                       info->trigger = addr;
> >
> > Unless we care about using the 'last' watchpoint in case multiple WPs have
> > the same address I think it would be clearer to change the above to:
> >
> >                         if (dist == 0) {
> >                                 /* We have a winner. */
> >                                 info = counter_arch_bp(wp);
> >                                 info->trigger = addr;
> >                                 break;
> >                         }
> 
> Without being an expert on the Hardware Breakpoint API, my
> understanding (based on how the old arm32 code worked and how the
> existing arm64 code works) is that the API accounts for the fact that
> more than one watchpoint can trigger and that we should report on all
> of them.
> 
> Specifically if you do:
> 
> watch 1 byte at 0x1000
> watch 1 byte at 0x1003
> 
> ...and then someone does a single 4-byte write at 0x1000 then both
> watchpoints should trigger.  If we do a "break" here then they won't
> both trigger.

Makes sense, thanks for the example!

> Also note that the triggering happens below in the "perf_bp_event(wp, regs)"
> so with your break I think you'll miss it, no?

You are right, I put that mentally after the loop, we definitely don't
want to skip it :)

> That being said, with my patch we still won't do exactly the right
> thing that for an "imprecise" watchpoint.  Specifically if you do:
> 
> watch 1 byte at 0x1008
> watch 1 byte at 0x100b
> write 16 bytes at 0x1000
> 
> ...then we will _only_ trigger the 0x1008 watchpoint.  ...but that's
> the limitation in how the breakpoints work.  You can see this is what
> happens because the imprecise stuff is outside the for loop and only
> triggers when nothing else did.

It's still an improvement from not triggering at all :)

Not that I'm an expert in this area, but the change looks good to me, so:

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

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

[Will Deacon]

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@chromium.org>
> ---
> 
>  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, I think it looks fine, so please put it into the patch system
with my Ack:

Acked-by: Will Deacon <will@kernel.org>

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.

Will

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

[Doug Anderson]

Hi,

On Wed, Nov 20, 2019 at 11:18 AM Will Deacon <will@kernel.org> 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@chromium.org>
> > ---
> >
> >  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@kernel.org>

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?

-Doug

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@kernel.org> 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@chromium.org>
> > > ---
> > >
> > >  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@kernel.org>
> 
> 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

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

[Doug Anderson]

Hi,

On Mon, Dec 2, 2019 at 8:36 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Nov 20, 2019 at 11:18 AM Will Deacon <will@kernel.org> 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@chromium.org>
> > > ---
> > >
> > >  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@kernel.org>
>
> Thanks!  Submitted as:
>
> https://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8944/1

Oddly, I found that if I go visit that page now I see:

> - - - Note 2 submitted by Russell King on 17 Jan 2020 11:16:34 (UTC) - - -
> Moved to applied
>
> Applied to git-curr (misc branch).

Yet if I go check mainline the patch is not there.  This came to my
attention since we had my patch picked to the Chrome OS 4.19 tree and
suddenly recently got a stable merge conflict with "ARM: 8986/1:
hw_breakpoint: Don't invoke overflow handler on uaccess watchpoints".

Anyone know what happened here?

-Doug

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

[Russell King - ARM Linux admin]

On Thu, Aug 06, 2020 at 08:05:10AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 2, 2019 at 8:36 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Nov 20, 2019 at 11:18 AM Will Deacon <will@kernel.org> 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@chromium.org>
> > > > ---
> > > >
> > > >  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@kernel.org>
> >
> > Thanks!  Submitted as:
> >
> > https://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8944/1
> 
> Oddly, I found that if I go visit that page now I see:
> 
> > - - - Note 2 submitted by Russell King on 17 Jan 2020 11:16:34 (UTC) - - -
> > Moved to applied
> >
> > Applied to git-curr (misc branch).
> 
> Yet if I go check mainline the patch is not there.  This came to my
> attention since we had my patch picked to the Chrome OS 4.19 tree and
> suddenly recently got a stable merge conflict with "ARM: 8986/1:
> hw_breakpoint: Don't invoke overflow handler on uaccess watchpoints".
> 
> Anyone know what happened here?

Yes.  Stephen Rothwell raised a complaint against it, which you were
copied with:

> Hi all,
> 
> Commit
> 
>   116375be0461 ("ARM: 8944/1: hw_breakpoint: Handle inexact watchpoint addresses")
> 
> is missing a Signed-off-by from its author.

My reply to Stephen's email was:

> Thanks Stephen, patch dropped.
> 
> It looks like Doug used his "m.disordat.com" address to submit the
> patch through the web interface, and there was no From: in the patch
> itself, so that was used as the patch author.  However, as you spotted,
> it was signed off using Doug's "chromium.org" address.
> 
> I think it's time to make the patch system a bit more strict, checking
> that the submission address is mentioned in a signed-off-by tag
> somewhere in the commit message.
> 
> Doug, the patch system does have your "chromium.org" address, if that's
> the one you want to use as the author, please submit using that instead.
> Thanks.
> 
> Russell.

Neither email got a response from you, so the patch was dropped and
nothing further happened.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Doug Anderson]

Hi,

On Thu, Aug 6, 2020 at 8:41 AM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, Aug 06, 2020 at 08:05:10AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Dec 2, 2019 at 8:36 AM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Nov 20, 2019 at 11:18 AM Will Deacon <will@kernel.org> 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@chromium.org>
> > > > > ---
> > > > >
> > > > >  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@kernel.org>
> > >
> > > Thanks!  Submitted as:
> > >
> > > https://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8944/1
> >
> > Oddly, I found that if I go visit that page now I see:
> >
> > > - - - Note 2 submitted by Russell King on 17 Jan 2020 11:16:34 (UTC) - - -
> > > Moved to applied
> > >
> > > Applied to git-curr (misc branch).
> >
> > Yet if I go check mainline the patch is not there.  This came to my
> > attention since we had my patch picked to the Chrome OS 4.19 tree and
> > suddenly recently got a stable merge conflict with "ARM: 8986/1:
> > hw_breakpoint: Don't invoke overflow handler on uaccess watchpoints".
> >
> > Anyone know what happened here?
>
> Yes.  Stephen Rothwell raised a complaint against it, which you were
> copied with:

Was a mailing list copied?  If so, do you have a lore.kernel.org link?
 I certainly don't see the email so I can only assume it made it to
spam.  That's unfortunate.


> > Hi all,
> >
> > Commit
> >
> >   116375be0461 ("ARM: 8944/1: hw_breakpoint: Handle inexact watchpoint addresses")
> >
> > is missing a Signed-off-by from its author.
>
> My reply to Stephen's email was:
>
> > Thanks Stephen, patch dropped.
> >
> > It looks like Doug used his "m.disordat.com" address to submit the
> > patch through the web interface, and there was no From: in the patch
> > itself, so that was used as the patch author.  However, as you spotted,
> > it was signed off using Doug's "chromium.org" address.
> >
> > I think it's time to make the patch system a bit more strict, checking
> > that the submission address is mentioned in a signed-off-by tag
> > somewhere in the commit message.
> >
> > Doug, the patch system does have your "chromium.org" address, if that's
> > the one you want to use as the author, please submit using that instead.
> > Thanks.
> >
> > Russell.
>
> Neither email got a response from you, so the patch was dropped and
> nothing further happened.

OK, I guess I'll have to rebase and resend.

-Doug

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

[Russell King - ARM Linux admin]

On Thu, Aug 06, 2020 at 04:41:44PM +0100, Russell King - ARM Linux admin wrote:
> On Thu, Aug 06, 2020 at 08:05:10AM -0700, Doug Anderson wrote:
> > Yet if I go check mainline the patch is not there.  This came to my
> > attention since we had my patch picked to the Chrome OS 4.19 tree and
> > suddenly recently got a stable merge conflict with "ARM: 8986/1:
> > hw_breakpoint: Don't invoke overflow handler on uaccess watchpoints".
> > 
> > Anyone know what happened here?
> 
> Yes.  Stephen Rothwell raised a complaint against it, which you were
> copied with:
> 
> > Hi all,
> > 
> > Commit
> > 
> >   116375be0461 ("ARM: 8944/1: hw_breakpoint: Handle inexact watchpoint addresses")
> > 
> > is missing a Signed-off-by from its author.
> 
> My reply to Stephen's email was:
> 
> > Thanks Stephen, patch dropped.
> > 
> > It looks like Doug used his "m.disordat.com" address to submit the
> > patch through the web interface, and there was no From: in the patch
> > itself, so that was used as the patch author.  However, as you spotted,
> > it was signed off using Doug's "chromium.org" address.
> > 
> > I think it's time to make the patch system a bit more strict, checking
> > that the submission address is mentioned in a signed-off-by tag
> > somewhere in the commit message.
> > 
> > Doug, the patch system does have your "chromium.org" address, if that's
> > the one you want to use as the author, please submit using that instead.
> > Thanks.
> > 
> > Russell.
> 
> Neither email got a response from you, so the patch was dropped and
> nothing further happened.

I should've also said, there's two ways to avoid that problem:

1. Provide a From: line in the standard way to tell the patch system
   who the author of the patch is (the author defaults to the known
   login email address.)

2. Update your login email address in the system to the one you
   normally author patches.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Doug Anderson]

Hi,

On Thu, Aug 6, 2020 at 8:45 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Aug 6, 2020 at 8:41 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Thu, Aug 06, 2020 at 08:05:10AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Mon, Dec 2, 2019 at 8:36 AM Doug Anderson <dianders@chromium.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Wed, Nov 20, 2019 at 11:18 AM Will Deacon <will@kernel.org> 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@chromium.org>
> > > > > > ---
> > > > > >
> > > > > >  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@kernel.org>
> > > >
> > > > Thanks!  Submitted as:
> > > >
> > > > https://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8944/1
> > >
> > > Oddly, I found that if I go visit that page now I see:
> > >
> > > > - - - Note 2 submitted by Russell King on 17 Jan 2020 11:16:34 (UTC) - - -
> > > > Moved to applied
> > > >
> > > > Applied to git-curr (misc branch).
> > >
> > > Yet if I go check mainline the patch is not there.  This came to my
> > > attention since we had my patch picked to the Chrome OS 4.19 tree and
> > > suddenly recently got a stable merge conflict with "ARM: 8986/1:
> > > hw_breakpoint: Don't invoke overflow handler on uaccess watchpoints".
> > >
> > > Anyone know what happened here?
> >
> > Yes.  Stephen Rothwell raised a complaint against it, which you were
> > copied with:
>
> Was a mailing list copied?  If so, do you have a lore.kernel.org link?
>  I certainly don't see the email so I can only assume it made it to
> spam.  That's unfortunate.
>
>
> > > Hi all,
> > >
> > > Commit
> > >
> > >   116375be0461 ("ARM: 8944/1: hw_breakpoint: Handle inexact watchpoint addresses")
> > >
> > > is missing a Signed-off-by from its author.
> >
> > My reply to Stephen's email was:
> >
> > > Thanks Stephen, patch dropped.
> > >
> > > It looks like Doug used his "m.disordat.com" address to submit the
> > > patch through the web interface, and there was no From: in the patch
> > > itself, so that was used as the patch author.  However, as you spotted,
> > > it was signed off using Doug's "chromium.org" address.
> > >
> > > I think it's time to make the patch system a bit more strict, checking
> > > that the submission address is mentioned in a signed-off-by tag
> > > somewhere in the commit message.
> > >
> > > Doug, the patch system does have your "chromium.org" address, if that's
> > > the one you want to use as the author, please submit using that instead.
> > > Thanks.
> > >
> > > Russell.
> >
> > Neither email got a response from you, so the patch was dropped and
> > nothing further happened.
>
> OK, I guess I'll have to rebase and resend.

Let's hope this one works:

https://www.armlinux.org.uk/developer/patches/viewpatch.php?id=8997/1

I re-tested it and it also matches how the conflict was resolved in
the Chrome OS tree which means that (once the stable merge lands in
our tree) this conflict resolution will get a bit of testing, though
the vast majority of devices running this code have since stopped
receiving auto updates.

-Doug

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

[Russell King - ARM Linux admin]

On Thu, Aug 06, 2020 at 11:28:09AM -0700, Doug Anderson wrote:
> Let's hope this one works:
> 
> https://www.armlinux.org.uk/developer/patches/viewpatch.php?id=8997/1

Almost.  It seems that you must have grabbed a copy off the patch
system, edited it and sent it back?

The commit message appears to contain:

Reviewed-by: Matthias Kaehlcke <(address hidden)>
Acked-by: Will Deacon <(address hidden)>

which is a transformation done for by the website front end to avoid
leaking people's email addresses to web crawling spam bots.

Provided I remember when I come to apply it, I could fix it up by
looking at the original patch (8944/1) but I'll probably forget by
that time.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Doug Anderson]

Hi,

On Thu, Aug 6, 2020 at 3:02 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, Aug 06, 2020 at 11:28:09AM -0700, Doug Anderson wrote:
> > Let's hope this one works:
> >
> > https://www.armlinux.org.uk/developer/patches/viewpatch.php?id=8997/1
>
> Almost.  It seems that you must have grabbed a copy off the patch
> system, edited it and sent it back?
>
> The commit message appears to contain:
>
> Reviewed-by: Matthias Kaehlcke <(address hidden)>
> Acked-by: Will Deacon <(address hidden)>
>
> which is a transformation done for by the website front end to avoid
> leaking people's email addresses to web crawling spam bots.
>
> Provided I remember when I come to apply it, I could fix it up by
> looking at the original patch (8944/1) but I'll probably forget by
> that time.

Sigh.  OK, hopefully correct now:

https://www.armlinux.org.uk/developer/patches/viewpatch.php?id=8997/2

-Doug

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

[Russell King - ARM Linux admin]

On Thu, Aug 06, 2020 at 03:26:09PM -0700, Doug Anderson wrote:
> Hi,
> 
> Sigh.  OK, hopefully correct now:
> 
> https://www.armlinux.org.uk/developer/patches/viewpatch.php?id=8997/2

LGTM.  Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!