| Message ID | 20191019111216.1.I82eae759ca6dc28a245b043f485ca490e3015321@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: hw_breakpoint: Handle inexact watchpoint addresses | expand |
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 >
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
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>
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
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
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
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
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.
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
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.
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
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.
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
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.
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)
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(-)