| Message ID | 20131211170911.GH26730@mudshark.cambridge.arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Will, Your patch works without errors. Thanks Aaron 2013/12/12 Will Deacon <will.deacon@arm.com>: > On Wed, Dec 11, 2013 at 07:16:11AM +0000, Aaron Liu wrote: >> Hi Will, > > Hi Aaron, > >> gdb calls ptrace twice to set watch points and break points as >> following. After first ptrace call is completed, second ptrace call >> fails with no space error. The first ptrace will reserve 16 watch >> points in static LIST_HEAD(bp_task_head)@kernel/events/hw_breakpoint.c >> and each entry, ie. struct perf_event, has a member called >> attr.bp_type. The bp_type in the entry should be used as indicator for >> watch points or break points. You could see >> __reserve_bp_slot@kernel/events/hw_breakpoint.c and find it does a >> basic check bp_type should be not HW_BREAKPOINT_EMPTY and >> HW_BREAKPOINT_INVALID. After the __reserve_bp_slot call, the reserved >> slot's bp_type is HW_BREAKPOINT_RW or HW_BREAKPOINT_X. However, >> ptrace_hbp_fill_attr_ctrl@arch/arm64/kernel/ptrace.c set the bp_type >> to HW_BREAKPOINT_EMPTY if it's disabled. This causes the >> find_slot_idx@kernel/event/hw_breakpoint.c to judge the existing >> entries as TYPE_INST. So, after first 16 watch points are reserved in >> list, the following break points determine no space to reserve, ie. >> max 16 break points. The patch only set disable bit even user zeroing >> control bits and it could still pass >> modify_user_hw_breakpoint@kernel/events/hw_breakpoint.c. > > Ok, thanks for the explanation. This used to work, but I suspect something > changed in the core code which caused this to start breaking with > HW_BREAKPOINT_EMPTY. > > Lucky for us, our ptrace_hbp_create function will infer the type/len for us > using values that are known to pass validation. In that case, the use of > HW_BREAKPOINT_EMPTY is redundant -- we just need to make sure we avoid > populating anything other than the disabled field in the perf_event_attr. > > A curious behaviour here (which also exists in the current code) is that the > breakpoint register effectively becomes read-only apart from the enable bit > if you are disabling it. That shouldn't effect gdb, though, and is required > to allow nuking of the breakpoint without changing its type. > > Can you test the following patch please? > > Thanks, > > Will > > --->8 > > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c > index 6777a2192b83..6a8928bba03c 100644 > --- a/arch/arm64/kernel/ptrace.c > +++ b/arch/arm64/kernel/ptrace.c > @@ -214,31 +214,29 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type, > { > int err, len, type, disabled = !ctrl.enabled; > > - if (disabled) { > - len = 0; > - type = HW_BREAKPOINT_EMPTY; > - } else { > - err = arch_bp_generic_fields(ctrl, &len, &type); > - if (err) > - return err; > - > - switch (note_type) { > - case NT_ARM_HW_BREAK: > - if ((type & HW_BREAKPOINT_X) != type) > - return -EINVAL; > - break; > - case NT_ARM_HW_WATCH: > - if ((type & HW_BREAKPOINT_RW) != type) > - return -EINVAL; > - break; > - default: > + attr->disabled = disabled; > + if (disabled) > + return 0; > + > + err = arch_bp_generic_fields(ctrl, &len, &type); > + if (err) > + return err; > + > + switch (note_type) { > + case NT_ARM_HW_BREAK: > + if ((type & HW_BREAKPOINT_X) != type) > return -EINVAL; > - } > + break; > + case NT_ARM_HW_WATCH: > + if ((type & HW_BREAKPOINT_RW) != type) > + return -EINVAL; > + break; > + default: > + return -EINVAL; > } > > attr->bp_len = len; > attr->bp_type = type; > - attr->disabled = disabled; > > return 0; > }
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index 6777a2192b83..6a8928bba03c 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -214,31 +214,29 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type, { int err, len, type, disabled = !ctrl.enabled; - if (disabled) { - len = 0; - type = HW_BREAKPOINT_EMPTY; - } else { - err = arch_bp_generic_fields(ctrl, &len, &type); - if (err) - return err; - - switch (note_type) { - case NT_ARM_HW_BREAK: - if ((type & HW_BREAKPOINT_X) != type) - return -EINVAL; - break; - case NT_ARM_HW_WATCH: - if ((type & HW_BREAKPOINT_RW) != type) - return -EINVAL; - break; - default: + attr->disabled = disabled; + if (disabled) + return 0; + + err = arch_bp_generic_fields(ctrl, &len, &type); + if (err) + return err; + + switch (note_type) { + case NT_ARM_HW_BREAK: + if ((type & HW_BREAKPOINT_X) != type) return -EINVAL; - } + break; + case NT_ARM_HW_WATCH: + if ((type & HW_BREAKPOINT_RW) != type) + return -EINVAL; + break; + default: + return -EINVAL; } attr->bp_len = len; attr->bp_type = type; - attr->disabled = disabled; return 0; }