Skip to contentSkip to conversation

hw_breakpoint: Breakpoint modification fixes

PATCH Superseded Jiri Olsa 5 participants 21 messages Nov 27, 2017 – Nov 28, 2017 perf events

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Jiri Olsa

author reviewer
#0 lore raw

cover hw_breakpoint: Breakpoint modification fixes

hi,
Milind Chabbi introduced new ioctl interface to change live breakpoint [1]. It allows to change its bp_addr, bp_len and bp_type throught new ioctl for perf breakpoint event.

We already have a kernel interface for this via modify_user_hw_breakpoint function. This function however does not update the breakpoint slot counts.

So when the same functionality was exposed to user space (Milind's change), with simple test program I could put wrong slots count on arm server [2] and ended up with no breakpoints available on the system. Note it's not an issue on x86, because it shares slot single counter for both data and inst types (CONFIG_HAVE_MIXED_BREAKPOINTS_REGS).

This patchset contains my fixes for keeping breakpoint slots count updated. On top of it there's Milind's change for new _IOC_MODIFY_ATTRIBUTES ioctl interface to change a breakpoint.

As I mentioned above there're kernel users of modify_user_hw_breakpoint function, all ptrace related AFAICS, which could got broken.. so cc-ing Oleg ;-)

I ran gdb and strace tests suites and got same amount of skip/fail tests as when I run them on unpatched machine, so I assume nothing new got broken.

It's also available in here:
https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
perf/bp

thanks,
jirka

[1] https://marc.info/?l=linux-kernel&m=151012255331565&w=2
[2] https://marc.info/?l=linux-man&m=151172469807302&w=2
---
Jiri Olsa (5):
hw_breakpoint: Pass bp_type directly as find_slot_idx argument
hw_breakpoint: Pass bp_type argument to __reserve_bp_slot|__release_bp_slot
hw_breakpoint: Add modify_bp_slot function
hw_breakpoint: Factor out __modify_user_hw_breakpoint function perf tests: Add breakpoint accounting/modify test

Milind Chabbi (1):
perf/core: fast breakpoint modification via _IOC_MODIFY_ATTRIBUTES.

 include/linux/hw_breakpoint.h         |   6 ++++
 include/uapi/linux/perf_event.h       |   2 ++
 kernel/events/core.c                  |  47 ++++++++++++++++++++++++++++++
 kernel/events/hw_breakpoint.c         | 110 ++++++++++++++++++++++++++++++++++++++++++++++++---------------------
 tools/include/uapi/linux/perf_event.h |   2 ++
 tools/perf/tests/Build                |   1 +
 tools/perf/tests/bp_account.c         | 195 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/builtin-test.c       |   4 +++
 tools/perf/tests/tests.h              |   1 +
 9 files changed, 335 insertions(+), 33 deletions(-)
 create mode 100644 tools/perf/tests/bp_account.c
author reviewer
#1 lore raw

PATCH 1/6 hw_breakpoint: Pass bp_type directly as find_slot_idx argument

Passing bp_type directly as find_slot_idx argument, so we don't need to have whole event to get the breakpoint slot type. It will be used in following changes.

Signed-off-by Jiri Olsa <[email protected]>
---
 kernel/events/hw_breakpoint.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
kernel/events/hw_breakpoint.c +5 -5
index 3f8cb1e14588..395ca07965af 100644
@@ -85,9 +85,9 @@ __weak int hw_breakpoint_weight(struct perf_event *bp)
 	return 1;
 }
 
-static inline enum bp_type_idx find_slot_idx(struct perf_event *bp)
+static inline enum bp_type_idx find_slot_idx(u64 bp_type)
 {
-	if (bp->attr.bp_type & HW_BREAKPOINT_RW)
+	if (bp_type & HW_BREAKPOINT_RW)
 		return TYPE_DATA;
 
 	return TYPE_INST;

@@ -122,7 +122,7 @@ static int task_bp_pinned(int cpu, struct perf_event *bp, enum bp_type_idx type)
 
 	list_for_each_entry(iter, &bp_task_head, hw.bp_list) {
 		if (iter->hw.target == tsk &&
-		    find_slot_idx(iter) == type &&
+		    find_slot_idx(iter->attr.bp_type) == type &&
 		    (iter->cpu < 0 || cpu == iter->cpu))
 			count += hw_breakpoint_weight(iter);
 	}

@@ -292,7 +292,7 @@ static int __reserve_bp_slot(struct perf_event *bp)
 	    bp->attr.bp_type == HW_BREAKPOINT_INVALID)
 		return -EINVAL;
 
-	type = find_slot_idx(bp);
+	type = find_slot_idx(bp->attr.bp_type);
 	weight = hw_breakpoint_weight(bp);
 
 	fetch_bp_busy_slots(&slots, bp, type);

@@ -329,7 +329,7 @@ static void __release_bp_slot(struct perf_event *bp)
 	enum bp_type_idx type;
 	int weight;
 
-	type = find_slot_idx(bp);
+	type = find_slot_idx(bp->attr.bp_type);
 	weight = hw_breakpoint_weight(bp);
 	toggle_bp_slot(bp, false, type, weight);
 }
Signature · 1 line
2.13.6
author reviewer
#2 lore raw

PATCH 2/6 hw_breakpoint: Pass bp_type argument to __reserve_bp_slot|__release_bp_slot

In reply to Jiri Olsa

Passing bp_type argument to __reserve_bp_slot and __release_bp_slot functions, so we can pass another bp_type than the one defined in bp->attr.bp_type. This will be handy in following change that fixes breakpoint slot counts during its modification.

Signed-off-by Jiri Olsa <[email protected]>
---
 kernel/events/hw_breakpoint.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
kernel/events/hw_breakpoint.c +10 -10
index 395ca07965af..9b31fbcc3305 100644
@@ -277,7 +277,7 @@ __weak void arch_unregister_hw_breakpoint(struct perf_event *bp)
  *       ((per_cpu(info->flexible, *) > 1) + max(per_cpu(info->cpu_pinned, *))
  *            + max(per_cpu(info->tsk_pinned, *))) < HBP_NUM
  */
-static int __reserve_bp_slot(struct perf_event *bp)
+static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
 {
 	struct bp_busy_slots slots = {0};
 	enum bp_type_idx type;

@@ -288,11 +288,11 @@ static int __reserve_bp_slot(struct perf_event *bp)
 		return -ENOMEM;
 
 	/* Basic checks */
-	if (bp->attr.bp_type == HW_BREAKPOINT_EMPTY ||
-	    bp->attr.bp_type == HW_BREAKPOINT_INVALID)
+	if (bp_type == HW_BREAKPOINT_EMPTY ||
+	    bp_type == HW_BREAKPOINT_INVALID)
 		return -EINVAL;
 
-	type = find_slot_idx(bp->attr.bp_type);
+	type = find_slot_idx(bp_type);
 	weight = hw_breakpoint_weight(bp);
 
 	fetch_bp_busy_slots(&slots, bp, type);

@@ -317,19 +317,19 @@ int reserve_bp_slot(struct perf_event *bp)
 
 	mutex_lock(&nr_bp_mutex);
 
-	ret = __reserve_bp_slot(bp);
+	ret = __reserve_bp_slot(bp, bp->attr.bp_type);
 
 	mutex_unlock(&nr_bp_mutex);
 
 	return ret;
 }
 
-static void __release_bp_slot(struct perf_event *bp)
+static void __release_bp_slot(struct perf_event *bp, u64 bp_type)
 {
 	enum bp_type_idx type;
 	int weight;
 
-	type = find_slot_idx(bp->attr.bp_type);
+	type = find_slot_idx(bp_type);
 	weight = hw_breakpoint_weight(bp);
 	toggle_bp_slot(bp, false, type, weight);
 }

@@ -339,7 +339,7 @@ void release_bp_slot(struct perf_event *bp)
 	mutex_lock(&nr_bp_mutex);
 
 	arch_unregister_hw_breakpoint(bp);
-	__release_bp_slot(bp);
+	__release_bp_slot(bp, bp->attr.bp_type);
 
 	mutex_unlock(&nr_bp_mutex);
 }

@@ -354,7 +354,7 @@ int dbg_reserve_bp_slot(struct perf_event *bp)
 	if (mutex_is_locked(&nr_bp_mutex))
 		return -1;
 
-	return __reserve_bp_slot(bp);
+	return __reserve_bp_slot(bp, bp->attr.bp_type);
 }
 
 int dbg_release_bp_slot(struct perf_event *bp)

@@ -362,7 +362,7 @@ int dbg_release_bp_slot(struct perf_event *bp)
 	if (mutex_is_locked(&nr_bp_mutex))
 		return -1;
 
-	__release_bp_slot(bp);
+	__release_bp_slot(bp, bp->attr.bp_type);
 
 	return 0;
 }
Signature · 1 line
2.13.6
author reviewer
#3 lore raw

PATCH 3/6 hw_breakpoint: Add modify_bp_slot function

In reply to Jiri Olsa

Adding modify_bp_slot function to keep slot numbers correct when changing the breakpoint type.

Using existing __release_bp_slot/__reserve_bp_slot call sequence to update the slot counts.

Signed-off-by Jiri Olsa <[email protected]>
---
 kernel/events/hw_breakpoint.c | 46 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 7 deletions(-)
kernel/events/hw_breakpoint.c +39 -7
index 9b31fbcc3305..776948beb4ac 100644
@@ -44,6 +44,7 @@
 #include <linux/list.h>
 #include <linux/cpu.h>
 #include <linux/smp.h>
+#include <linux/bug.h>
 
 #include <linux/hw_breakpoint.h>
 /*

@@ -344,6 +345,38 @@ void release_bp_slot(struct perf_event *bp)
 	mutex_unlock(&nr_bp_mutex);
 }
 
+static int __modify_bp_slot(struct perf_event *bp, u64 old_type)
+{
+	int err;
+
+	__release_bp_slot(bp, old_type);
+
+	err = __reserve_bp_slot(bp, bp->attr.bp_type);
+	if (err) {
+		/*
+		 * Reserve the old_type slot back in case
+		 * there's no space for the new type.
+		 *
+		 * This must succeed, because we just released
+		 * the old_type slot in the __release_bp_slot
+		 * call above. If not, something is broken.
+		 */
+		WARN_ON(__reserve_bp_slot(bp, old_type));
+	}
+
+	return err;
+}
+
+static int modify_bp_slot(struct perf_event *bp, u64 old_type)
+{
+	int ret;
+
+	mutex_lock(&nr_bp_mutex);
+	ret = __modify_bp_slot(bp, old_type);
+	mutex_unlock(&nr_bp_mutex);
+	return ret;
+}
+
 /*
  * Allow the kernel debugger to reserve breakpoint slots without
  * taking a lock using the dbg_* variant of for the reserve and

@@ -435,6 +468,7 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 	u64 old_addr = bp->attr.bp_addr;
 	u64 old_len = bp->attr.bp_len;
 	int old_type = bp->attr.bp_type;
+	bool modify = attr->bp_type != old_type;
 	int err = 0;
 
 	/*

@@ -452,12 +486,9 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 	bp->attr.bp_type = attr->bp_type;
 	bp->attr.bp_len = attr->bp_len;
 
-	if (attr->disabled)
-		goto end;
-
 	err = validate_hw_breakpoint(bp);
-	if (!err)
-		perf_event_enable(bp);
+	if (!err && modify)
+		err = modify_bp_slot(bp, old_type);
 
 	if (err) {
 		bp->attr.bp_addr = old_addr;

@@ -469,9 +500,10 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 		return err;
 	}
 
-end:
-	bp->attr.disabled = attr->disabled;
+	if (!attr->disabled)
+		perf_event_enable(bp);
 
+	bp->attr.disabled = attr->disabled;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(modify_user_hw_breakpoint);
Signature · 1 line
2.13.6
author reviewer
#4 lore raw

PATCH 4/6 hw_breakpoint: Factor out __modify_user_hw_breakpoint function

In reply to Jiri Olsa

Moving out the all the functionality without the events disabling/enabling calls, because we want to call another disabling/enabling functions in following change.

Signed-off-by Jiri Olsa <[email protected]>
---
 kernel/events/hw_breakpoint.c | 46 +++++++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 17 deletions(-)
kernel/events/hw_breakpoint.c +29 -17
index 776948beb4ac..a556aba223da 100644
@@ -456,6 +456,33 @@ register_user_hw_breakpoint(struct perf_event_attr *attr,
 }
 EXPORT_SYMBOL_GPL(register_user_hw_breakpoint);
 
+static int __modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr)
+{
+	u64 old_addr = bp->attr.bp_addr;
+	u64 old_len  = bp->attr.bp_len;
+	int old_type = bp->attr.bp_type;
+	bool modify  = attr->bp_type != old_type;
+	int err = 0;
+
+	bp->attr.bp_addr = attr->bp_addr;
+	bp->attr.bp_type = attr->bp_type;
+	bp->attr.bp_len  = attr->bp_len;
+
+	err = validate_hw_breakpoint(bp);
+	if (!err && modify)
+		err = modify_bp_slot(bp, old_type);
+
+	if (err) {
+		bp->attr.bp_addr = old_addr;
+		bp->attr.bp_type = old_type;
+		bp->attr.bp_len  = old_len;
+		return err;
+	}
+
+	bp->attr.disabled = attr->disabled;
+	return 0;
+}
+
 /**
  * modify_user_hw_breakpoint - modify a user-space hardware breakpoint
  * @bp: the breakpoint structure to modify

@@ -465,11 +492,7 @@ EXPORT_SYMBOL_GPL(register_user_hw_breakpoint);
  */
 int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr)
 {
-	u64 old_addr = bp->attr.bp_addr;
-	u64 old_len = bp->attr.bp_len;
-	int old_type = bp->attr.bp_type;
-	bool modify = attr->bp_type != old_type;
-	int err = 0;
+	int err;
 
 	/*
 	 * modify_user_hw_breakpoint can be invoked with IRQs disabled and hence it

@@ -482,18 +505,9 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 	else
 		perf_event_disable(bp);
 
-	bp->attr.bp_addr = attr->bp_addr;
-	bp->attr.bp_type = attr->bp_type;
-	bp->attr.bp_len = attr->bp_len;
-
-	err = validate_hw_breakpoint(bp);
-	if (!err && modify)
-		err = modify_bp_slot(bp, old_type);
+	err = __modify_user_hw_breakpoint(bp, attr);
 
 	if (err) {
-		bp->attr.bp_addr = old_addr;
-		bp->attr.bp_type = old_type;
-		bp->attr.bp_len = old_len;
 		if (!bp->attr.disabled)
 			perf_event_enable(bp);
 

@@ -502,8 +516,6 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 
 	if (!attr->disabled)
 		perf_event_enable(bp);
-
-	bp->attr.disabled = attr->disabled;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(modify_user_hw_breakpoint);
Signature · 1 line
2.13.6
maintainer
#5 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

Quoted 16 lines · Jiri Olsa
kernel/events/hw_breakpoint.c +16 -0
+static int __modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr)
+{
+	u64 old_addr = bp->attr.bp_addr;
+	u64 old_len  = bp->attr.bp_len;
+	int old_type = bp->attr.bp_type;
+	bool modify  = attr->bp_type != old_type;
+	int err = 0;
+
+	bp->attr.bp_addr = attr->bp_addr;
+	bp->attr.bp_type = attr->bp_type;
+	bp->attr.bp_len  = attr->bp_len;
+
+	err = validate_hw_breakpoint(bp);
+	if (!err && modify)
+		err = modify_bp_slot(bp, old_type);
+

kernel/events/hw_breakpoint.c register_user_hw_breakpoint(struct perf_event_attr *attr, · line 484

Quoted 10 lines
kernel/events/hw_breakpoint.c +10 -0
+	if (err) {
+		bp->attr.bp_addr = old_addr;
+		bp->attr.bp_type = old_type;
+		bp->attr.bp_len  = old_len;
+		return err;
+	}
+
+	bp->attr.disabled = attr->disabled;
+	return 0;
+}

I think this function is failing to check if anything else in the attr changes.

For example, someone could have added PERF_SAMPLE_BRANCH_STACK. That's something you'll fail to create breakpoints with, but this modification would 'accept'.

#6 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

Quoted 31 lines · Peter Zijlstra
Quoted 16 lines · Jiri Olsa
kernel/events/hw_breakpoint.c +16 -0
+static int __modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr)
+{
+	u64 old_addr = bp->attr.bp_addr;
+	u64 old_len  = bp->attr.bp_len;
+	int old_type = bp->attr.bp_type;
+	bool modify  = attr->bp_type != old_type;
+	int err = 0;
+
+	bp->attr.bp_addr = attr->bp_addr;
+	bp->attr.bp_type = attr->bp_type;
+	bp->attr.bp_len  = attr->bp_len;
+
+	err = validate_hw_breakpoint(bp);
+	if (!err && modify)
+		err = modify_bp_slot(bp, old_type);
+

kernel/events/hw_breakpoint.c register_user_hw_breakpoint(struct perf_event_attr *attr, · line 484

Quoted 10 lines
kernel/events/hw_breakpoint.c +10 -0
+	if (err) {
+		bp->attr.bp_addr = old_addr;
+		bp->attr.bp_type = old_type;
+		bp->attr.bp_len  = old_len;
+		return err;
+	}
+
+	bp->attr.disabled = attr->disabled;
+	return 0;
+}

I think this function is failing to check if anything else in the attr changes.

Quoted 4 lines

For example, someone could have added PERF_SAMPLE_BRANCH_STACK. That's something you'll fail to create breakpoints with, but this modification would 'accept'.

hum, I dont think so.. the only things you're allowed to change are bp_addr, bp_type and bp_len.. we put new values in those fields and keep the rest untouched.. apart from 'disabled' bit

jirka

maintainer
#7 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

Quoted 32 lines · Jiri Olsa
Quoted 31 lines · Peter Zijlstra
Quoted 16 lines · Jiri Olsa
kernel/events/hw_breakpoint.c +16 -0
+static int __modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr)
+{
+	u64 old_addr = bp->attr.bp_addr;
+	u64 old_len  = bp->attr.bp_len;
+	int old_type = bp->attr.bp_type;
+	bool modify  = attr->bp_type != old_type;
+	int err = 0;
+
+	bp->attr.bp_addr = attr->bp_addr;
+	bp->attr.bp_type = attr->bp_type;
+	bp->attr.bp_len  = attr->bp_len;
+
+	err = validate_hw_breakpoint(bp);
+	if (!err && modify)
+		err = modify_bp_slot(bp, old_type);
+

kernel/events/hw_breakpoint.c register_user_hw_breakpoint(struct perf_event_attr *attr, · line 484

Quoted 10 lines
kernel/events/hw_breakpoint.c +10 -0
+	if (err) {
+		bp->attr.bp_addr = old_addr;
+		bp->attr.bp_type = old_type;
+		bp->attr.bp_len  = old_len;
+		return err;
+	}
+
+	bp->attr.disabled = attr->disabled;
+	return 0;
+}

I think this function is failing to check if anything else in the attr changes.

Quoted 8 lines
Quoted 4 lines

For example, someone could have added PERF_SAMPLE_BRANCH_STACK. That's something you'll fail to create breakpoints with, but this modification would 'accept'.

hum, I dont think so.. the only things you're allowed to change are bp_addr, bp_type and bp_len.. we put new values in those fields and keep the rest untouched.. apart from 'disabled' bit

But what validates the input attr is the same as the event attr, aside from those fields?

#8 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

Quoted 38 lines · Peter Zijlstra
Quoted 37 lines · Jiri Olsa
Quoted 35 lines · Peter Zijlstra
Quoted 16 lines · Jiri Olsa
kernel/events/hw_breakpoint.c +16 -0
+static int __modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr)
+{
+	u64 old_addr = bp->attr.bp_addr;
+	u64 old_len  = bp->attr.bp_len;
+	int old_type = bp->attr.bp_type;
+	bool modify  = attr->bp_type != old_type;
+	int err = 0;
+
+	bp->attr.bp_addr = attr->bp_addr;
+	bp->attr.bp_type = attr->bp_type;
+	bp->attr.bp_len  = attr->bp_len;
+
+	err = validate_hw_breakpoint(bp);
+	if (!err && modify)
+		err = modify_bp_slot(bp, old_type);
+

kernel/events/hw_breakpoint.c register_user_hw_breakpoint(struct perf_event_attr *attr, · line 484

Quoted 10 lines
kernel/events/hw_breakpoint.c +10 -0
+	if (err) {
+		bp->attr.bp_addr = old_addr;
+		bp->attr.bp_type = old_type;
+		bp->attr.bp_len  = old_len;
+		return err;
+	}
+
+	bp->attr.disabled = attr->disabled;
+	return 0;
+}

I think this function is failing to check if anything else in the attr changes.

For example, someone could have added PERF_SAMPLE_BRANCH_STACK. That's something you'll fail to create breakpoints with, but this modification would 'accept'.

Quoted 6 lines
Quoted 3 lines

hum, I dont think so.. the only things you're allowed to change are bp_addr, bp_type and bp_len.. we put new values in those fields and keep the rest untouched.. apart from 'disabled' bit

But what validates the input attr is the same as the event attr, aside from those fields?

we don't.. the attr serves as a holder to carry those fields into the function

the current kernel interface does not check anything else

there's one more check in the ioctl path, we check the type in perf_event_modify_attr:

        if (event->attr.type != attr->type)
                return -EINVAL;

jirka

maintainer
#9 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

Quoted 6 lines · Jiri Olsa
Quoted 2 lines · Peter Zijlstra

But what validates the input attr is the same as the event attr, aside from those fields?

we don't.. the attr serves as a holder to carry those fields into the function

Then that's a straight up bug.

Quoted 1 line

the current kernel interface does not check anything else

Not enough, if the new attr would fail perf_event_open() it should also fail this modify thing.

Quoted 5 lines

there's one more check in the ioctl path, we check the type in perf_event_modify_attr:

        if (event->attr.type != attr->type)
                return -EINVAL;

Note how hw_breakpoint_event_init() tests has_branch_stack() and fails on it.

Ideally we should check a whole lot more and fail, but alas..

maintainer
#10 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

Quoted 8 lines · Peter Zijlstra
Quoted 6 lines · Jiri Olsa
Quoted 2 lines · Peter Zijlstra

But what validates the input attr is the same as the event attr, aside from those fields?

we don't.. the attr serves as a holder to carry those fields into the function

Quoted 6 lines

Then that's a straight up bug.

Quoted 1 line

the current kernel interface does not check anything else

Not enough, if the new attr would fail perf_event_open() it should also fail this modify thing.

On IRC you asked:

<jolsa> peterz, I dont follow.. why should we check fields that we dont use?

Suppose someone does:

	attr = malloc(sizeof(*attr)); // uninitialized memory
	attr->type = BP;
	attr->bp_addr = new_addr;
	attr->bp_type = bp_type;
	attr->bp_len = bp_len;
	ioctl(fd, PERF_IOC_MOD_ATTR, &attr);

And feeds absolute shite for the rest of the fields.

Then we later want to extend IOC_MOD_ATTR to allow changing attr::sample_type but we can't, because that would break the above application.

Therefore we must be very strict to check only the fields we can change have changed.

#11 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

Quoted 31 lines · Peter Zijlstra
Quoted 8 lines · Peter Zijlstra
Quoted 6 lines · Jiri Olsa
Quoted 2 lines · Peter Zijlstra

But what validates the input attr is the same as the event attr, aside from those fields?

we don't.. the attr serves as a holder to carry those fields into the function

Quoted 6 lines

Then that's a straight up bug.

Quoted 1 line

the current kernel interface does not check anything else

Not enough, if the new attr would fail perf_event_open() it should also fail this modify thing.

On IRC you asked:

<jolsa> peterz, I dont follow.. why should we check fields that we dont use?

Suppose someone does:

        attr = malloc(sizeof(*attr)); // uninitialized memory
        attr->type = BP;
        attr->bp_addr = new_addr;
        attr->bp_type = bp_type;
        attr->bp_len = bp_len;
        ioctl(fd, PERF_IOC_MOD_ATTR, &attr);
Quoted 8 lines

And feeds absolute shite for the rest of the fields.

Then we later want to extend IOC_MOD_ATTR to allow changing attr::sample_type but we can't, because that would break the above application.

Therefore we must be very strict to check only the fields we can change have changed.

The possible checks is infinite and checking against what was used in perf_event_open will further complicate matters.
The original objective was very simple: allow bp_addr, bp_type, and bp_len to be modified without the rest of the baggage.

maintainer
#12 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

Quoted 1 line · Milind Chabbi

The possible checks is infinite

struct perf_event_attr is very much a finite data type.

Something as simple as:

	struct perf_event_attr tmp1 = new_attr, tmp2 = event->attr;

	tmp1.bp_type = tmp2.bp_type;
	tmp1.bp_addr = tmp2.bp_addr;
	tmp1.bp_len  = tmp2.bp_len;

	if (memcmp(&tmp1, &tmp2, sizeof(tmp1)))
		return -EINVAL;

would actually do the checks __modify_user_hw_breakpoint() needs to do.

#13 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

Quoted 16 lines · Peter Zijlstra
Quoted 1 line · Milind Chabbi

The possible checks is infinite

struct perf_event_attr is very much a finite data type.

Something as simple as:

        struct perf_event_attr tmp1 = new_attr, tmp2 = event->attr;

        tmp1.bp_type = tmp2.bp_type;
        tmp1.bp_addr = tmp2.bp_addr;
        tmp1.bp_len  = tmp2.bp_len;

        if (memcmp(&tmp1, &tmp2, sizeof(tmp1)))
                return -EINVAL;
Quoted 1 line

would actually do the checks __modify_user_hw_breakpoint() needs to do.

Ok. That seems like a slick idea. I will post a patch that does this.

#14 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

In reply to Peter Zijlstra
Quoted 16 lines · Peter Zijlstra
Quoted 1 line · Milind Chabbi

The possible checks is infinite

struct perf_event_attr is very much a finite data type.

Something as simple as:

	struct perf_event_attr tmp1 = new_attr, tmp2 = event->attr;

	tmp1.bp_type = tmp2.bp_type;
	tmp1.bp_addr = tmp2.bp_addr;
	tmp1.bp_len  = tmp2.bp_len;

	if (memcmp(&tmp1, &tmp2, sizeof(tmp1)))
		return -EINVAL;
Quoted 1 line

would actually do the checks __modify_user_hw_breakpoint() needs to do.

I see.. will post new version

thanks,
jirka

#15 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

Quoted 17 lines · Jiri Olsa
Quoted 16 lines · Peter Zijlstra
Quoted 1 line · Milind Chabbi

The possible checks is infinite

struct perf_event_attr is very much a finite data type.

Something as simple as:

      struct perf_event_attr tmp1 = new_attr, tmp2 = event->attr;

      tmp1.bp_type = tmp2.bp_type;
      tmp1.bp_addr = tmp2.bp_addr;
      tmp1.bp_len  = tmp2.bp_len;

      if (memcmp(&tmp1, &tmp2, sizeof(tmp1)))
              return -EINVAL;
Quoted 6 lines
Quoted 1 line

would actually do the checks __modify_user_hw_breakpoint() needs to do.

I see.. will post new version

thanks,
jirka

Thanks Jirka. I will leave it to you then.

#16 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

In reply to Peter Zijlstra
Quoted 16 lines · Peter Zijlstra
Quoted 1 line · Milind Chabbi

The possible checks is infinite

struct perf_event_attr is very much a finite data type.

Something as simple as:

	struct perf_event_attr tmp1 = new_attr, tmp2 = event->attr;

	tmp1.bp_type = tmp2.bp_type;
	tmp1.bp_addr = tmp2.bp_addr;
	tmp1.bp_len  = tmp2.bp_len;

	if (memcmp(&tmp1, &tmp2, sizeof(tmp1)))
		return -EINVAL;
Quoted 1 line

would actually do the checks __modify_user_hw_breakpoint() needs to do.

It could fail with uninitialized padding.

-Andi

#17 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

Quoted 17 lines · Andi Kleen
Quoted 16 lines · Peter Zijlstra
Quoted 1 line · Milind Chabbi

The possible checks is infinite

struct perf_event_attr is very much a finite data type.

Something as simple as:

      struct perf_event_attr tmp1 = new_attr, tmp2 = event->attr;

      tmp1.bp_type = tmp2.bp_type;
      tmp1.bp_addr = tmp2.bp_addr;
      tmp1.bp_len  = tmp2.bp_len;

      if (memcmp(&tmp1, &tmp2, sizeof(tmp1)))
              return -EINVAL;
Quoted 7 lines
Quoted 1 line

would actually do the checks __modify_user_hw_breakpoint() needs to do.

It could fail with uninitialized padding.

-Andi

Hm...
How about we zero out __reserved_1 and __reserved_2 before memcmp()?

#18 lore raw

Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

In reply to Andi Kleen
Quoted 14 lines · Andi Kleen
Quoted 13 lines · Peter Zijlstra
Quoted 1 line · Milind Chabbi

The possible checks is infinite

struct perf_event_attr is very much a finite data type.

Something as simple as:

	struct perf_event_attr tmp1 = new_attr, tmp2 = event->attr;

	tmp1.bp_type = tmp2.bp_type;
	tmp1.bp_addr = tmp2.bp_addr;
	tmp1.bp_len  = tmp2.bp_len;
Quoted 6 lines
Quoted 4 lines
	if (memcmp(&tmp1, &tmp2, sizeof(tmp1)))
		return -EINVAL;

would actually do the checks __modify_user_hw_breakpoint() needs to do.

It could fail with uninitialized padding.

I think that should be fine.. both attrs go through perf_copy_attr, which should check on it.. I found we init attr.sample_max_stack out of perf_copy_attr, but we can move it there (attached)

also modify_user_hw_breakpoint is exported.. not sure we can add this contrain and potentionaly break some kernel module?

I check kernel all the current kernel users and they copy the whole perf_event_attr into attr argument before they change the allowed bp_* fields, so there's no harm.

thanks,
jirka

---

kernel/events/core.c +3 -3
index 799bb352d99f..028adb24bf7a 100644
@@ -9673,6 +9673,9 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
 			ret = -EINVAL;
 	}
 
+	if (!attr->sample_max_stack)
+		attr->sample_max_stack = sysctl_perf_event_max_stack;
+
 	if (attr->sample_type & PERF_SAMPLE_REGS_INTR)
 		ret = perf_reg_validate(attr->sample_regs_intr);
 out:

@@ -9886,9 +9889,6 @@ SYSCALL_DEFINE5(perf_event_open,
 	    perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
 		return -EACCES;
 
-	if (!attr.sample_max_stack)
-		attr.sample_max_stack = sysctl_perf_event_max_stack;
-
 	/*
 	 * In cgroup mode, the pid argument is used to pass the fd
 	 * opened to the cgroup directory in cgroupfs. The cpu argument
kernel/events/hw_breakpoint.c +3 -0
index a556aba223da..7b85160393b7 100644
@@ -468,6 +468,9 @@ static int __modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_
 	bp->attr.bp_type = attr->bp_type;
 	bp->attr.bp_len  = attr->bp_len;
 
+	if (memcmp(&bp->attr, attr, sizeof(*attr)))
+		return -EINVAL;
+
 	err = validate_hw_breakpoint(bp);
 	if (!err && modify)
 		err = modify_bp_slot(bp, old_type);
author reviewer
#19 lore raw

PATCH 5/6 perf/core: fast breakpoint modification via _IOC_MODIFY_ATTRIBUTES.

In reply to Jiri Olsa

From: Milind Chabbi <[email protected]>

Problem and motivation: Once a breakpoint perf event (PERF_TYPE_BREAKPOINT) is created, there is no flexibility to change the breakpoint type (bp_type), breakpoint address (bp_addr), or breakpoint length (bp_len). The only option is to close the perf event and configure a new breakpoint event. This inflexibility has a significant performance overhead. For example, sampling-based, lightweight performance profilers (and also concurrency bug detection tools), monitor different addresses for a short duration using PERF_TYPE_BREAKPOINT and change the address (bp_addr) to another address or change the kind of breakpoint (bp_type) from "write" to a "read" or vice-versa or change the length (bp_len) of the address being monitored. The cost of these modifications is prohibitive since it involves unmapping the circular buffer associated with the perf event, closing the perf event, opening another perf event and mmaping another circular buffer.

Solution: The new ioctl flag for perf events, PERF_EVENT_IOC_MODIFY_ATTRIBUTES, introduced in this patch takes a pointer to a struct perf_event_attr as an argument to update an old breakpoint event with new address, type, and size. This facility allows retaining a previous mmaped perf events ring buffer and avoids having to close and reopen another perf event.

This patch supports only changing PERF_TYPE_BREAKPOINT event type; future implementations can extend this feature. The patch replicates some of its functionality of modify_user_hw_breakpoint() in kernel/events/hw_breakpoint.c. modify_user_hw_breakpoint cannot be called directly since perf_event_ctx_lock() is already held in _perf_ioctl().

Evidence: Experiments show that the baseline (not able to modify an already created breakpoint) costs an order of magnitude (~10x) more than the suggested optimization (having the ability to dynamically modifying a configured breakpoint via ioctl). When the breakpoints typically do not trap, the speedup due to the suggested optimization is ~10x; even when the breakpoints always trap, the speedup is ~4x due to the suggested optimization.

Testing: tests posted at
https://github.com/linux-contrib/perf_event_modify_bp demonstrate the performance significance of this patch. Tests also check the functional correctness of the patch.

Signed-off-by Milind Chabbi <[email protected]>
[ Using __modify_user_hw_breakpoint function. ]
Signed-off-by Jiri Olsa <[email protected]>
---
 include/linux/hw_breakpoint.h         |  6 +++++
 include/uapi/linux/perf_event.h       |  2 ++
 kernel/events/core.c                  | 47 +++++++++++++++++++++++++++++++++++
 kernel/events/hw_breakpoint.c         |  2 +-
 tools/include/uapi/linux/perf_event.h |  2 ++
 5 files changed, 58 insertions(+), 1 deletion(-)
include/linux/hw_breakpoint.h +6 -0
index cf045885a499..a46feedefa3b 100644
@@ -53,6 +53,8 @@ register_user_hw_breakpoint(struct perf_event_attr *attr,
 /* FIXME: only change from the attr, and don't unregister */
 extern int
 modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr);
+extern int
+__modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr);
 
 /*
  * Kernel breakpoints are not associated with any particular thread.

@@ -97,6 +99,10 @@ register_user_hw_breakpoint(struct perf_event_attr *attr,
 static inline int
 modify_user_hw_breakpoint(struct perf_event *bp,
 			  struct perf_event_attr *attr)	{ return -ENOSYS; }
+static inline int
+__modify_user_hw_breakpoint(struct perf_event *bp,
+			    struct perf_event_attr *attr) { return -ENOSYS; }
+
 static inline struct perf_event *
 register_wide_hw_breakpoint_cpu(struct perf_event_attr *attr,
 				perf_overflow_handler_t	 triggered,
include/uapi/linux/perf_event.h +2 -0
index 362493a2f950..cd430821f022 100644
@@ -433,6 +433,8 @@ struct perf_event_attr {
 #define PERF_EVENT_IOC_ID		_IOR('$', 7, __u64 *)
 #define PERF_EVENT_IOC_SET_BPF		_IOW('$', 8, __u32)
 #define PERF_EVENT_IOC_PAUSE_OUTPUT	_IOW('$', 9, __u32)
+#define PERF_EVENT_IOC_MODIFY_ATTRIBUTES	\
+			_IOW('$', 10, struct perf_event_attr *)
 
 enum perf_event_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,
kernel/events/core.c +47 -0

65 lines not on this page · raw message

kernel/events/hw_breakpoint.c +1 -1
index a556aba223da..831c20be8b98 100644
@@ -456,7 +456,7 @@ register_user_hw_breakpoint(struct perf_event_attr *attr,
 }
 EXPORT_SYMBOL_GPL(register_user_hw_breakpoint);
 
-static int __modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr)
+int __modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr)
 {
 	u64 old_addr = bp->attr.bp_addr;
 	u64 old_len  = bp->attr.bp_len;
tools/include/uapi/linux/perf_event.h +2 -0
index 362493a2f950..4602c260f802 100644
@@ -433,6 +433,8 @@ struct perf_event_attr {
 #define PERF_EVENT_IOC_ID		_IOR('$', 7, __u64 *)
 #define PERF_EVENT_IOC_SET_BPF		_IOW('$', 8, __u32)
 #define PERF_EVENT_IOC_PAUSE_OUTPUT	_IOW('$', 9, __u32)
+#define PERF_EVENT_IOC_MODIFY_ATTRIBUTES	\
+				_IOW('$', 10, struct perf_event_attr *)
 
 enum perf_event_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,
Signature · 1 line
2.13.6
author reviewer
#20 lore raw

PATCH 6/6 perf tests: Add breakpoint accounting/modify test

In reply to Jiri Olsa

Adding test that:
- detects the number of watch/break-points, skip test if any is missing
- detects PERF_EVENT_IOC_MODIFY_ATTRIBUTES ioctl, skip test if it's missing
- detects if watchpoints and breakpoints share same slots
- create all possible watchpoints on cpu 0
- change one of it to breakpoint
- in case wp and bp do not share slots, we create another watchpoint to ensure the slot accounting is correct

Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/tests/Build          |   1 +
 tools/perf/tests/bp_account.c   | 195 ++++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/builtin-test.c |   4 +
 tools/perf/tests/tests.h        |   1 +
 4 files changed, 201 insertions(+)
 create mode 100644 tools/perf/tests/bp_account.c
tools/perf/tests/Build +1 -0
index 87bf3edb037c..62ca0174d5e1 100644
@@ -20,6 +20,7 @@ perf-y += hists_cumulate.o
 perf-y += python-use.o
 perf-y += bp_signal.o
 perf-y += bp_signal_overflow.o
+perf-y += bp_account.o
 perf-y += task-exit.o
 perf-y += sw-clock.o
 perf-y += mmap-thread-lookup.o
tools/perf/tests/bp_account.c new file +195 -0

195 lines not on this page · raw message

tools/perf/tests/builtin-test.c +4 -0
index 766573e236e4..705558805a8c 100644
@@ -116,6 +116,10 @@ static struct test generic_tests[] = {
 		.is_supported = test__bp_signal_is_supported,
 	},
 	{
+		.desc = "Breakpoint accounting",
+		.func = test__bp_accounting,
+	},
+	{
 		.desc = "Number of exit events of a simple workload",
 		.func = test__task_exit,
 	},
tools/perf/tests/tests.h +1 -0
index 2862b80bc288..9f51edac44ae 100644
@@ -58,6 +58,7 @@ int test__hists_link(struct test *test, int subtest);
 int test__python_use(struct test *test, int subtest);
 int test__bp_signal(struct test *test, int subtest);
 int test__bp_signal_overflow(struct test *test, int subtest);
+int test__bp_accounting(struct test *test, int subtest);
 int test__task_exit(struct test *test, int subtest);
 int test__mem(struct test *test, int subtest);
 int test__sw_clock_freq(struct test *test, int subtest);
Signature · 1 line
2.13.6