On Thu, Nov 09, 2017 at 08:46:58AM +0100, Jiri Olsa wrote:
SNIP
> > Jirka,
> >
> > I carefully looked at bp_cpuinfo[] and nr_slots[] data structures.
> > nr_slots[] is an array of length two (one slot of TYPE_INST and
> > another for TYPE_DATA).
> > The accounting "thinks" that there is one limit on the number of
> > instruction breakpoints and another limit on the number of data
> > breakpoints.
> > The assumption is clearly broken; for example, on x86 there exists a
> > limit on the *total* number of all breakpoints disregarding their kind
> > and the code has failed to capture this aspect.
>
> there's the CONFIG_HAVE_MIXED_BREAKPOINTS_REGS that puts DATA and INST
> under one count on x86.. but that seems to be the enabled only for:
>
> arch/sh/Kconfig: select HAVE_MIXED_BREAKPOINTS_REGS
> arch/x86/Kconfig: select HAVE_MIXED_BREAKPOINTS_REGS
>
> >
> > As such, modify_user_hw_breakpoint() makes no attempt to keep the
> > counts correct. Instead, it simply tries to change and install a new
> > breakpoint and fails if the hardware disallows.
> > This can lead to a situation where, say on x86, someone creates 4
> > TYPE_DATA breakpoints, then changes one of them to TYPE_INS via
> > modify_user_hw_breakpoint() and then releases the TYPE_INS breakpoint.
> > Since the accounting still thinks that there are four TYPE_DATA
> > breakpoints, it will disallow creating a new TYPE_DATA breakpoint,
> > although there is place for one TYPE_DATA breakpoint.
> >
> > This convinces me that the problem and the solution are outside of
> > this current patch.
> > Do you agree?
>
> I'll leave this decision to maintainer ;-) but seems better to fix
> the interface before we add any new dependent function calls
how about something like below (untested)
looks like there's no irq caller for modify_user_hw_breakpoint,
so we should be fine with locking nr_bp_mutex
jirka
---
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 3f8cb1e14588..f062b68399ea 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -448,6 +448,8 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct
perf_event_attr *att
else
perf_event_disable(bp);
+ release_bp_slot(bp);
+
bp->attr.bp_addr = attr->bp_addr;
bp->attr.bp_type = attr->bp_type;
bp->attr.bp_len = attr->bp_len;
@@ -455,9 +457,9 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct
perf_event_attr *att
if (attr->disabled)
goto end;
- err = validate_hw_breakpoint(bp);
+ err = reserve_bp_slot(bp);
if (!err)
- perf_event_enable(bp);
+ err = validate_hw_breakpoint(bp);
if (err) {
bp->attr.bp_addr = old_addr;
@@ -469,6 +471,7 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct
perf_event_attr *att
return err;
}
+ perf_event_enable(bp);
end:
bp->attr.disabled = attr->disabled;
