hw-breakpoints: Use struct perf_event_attr to define user breakpoints

In-kernel user breakpoints are created using functions in which we pass breakpoint parameters as individual variables: address, length and type. Although it fits well for x86, this just does not scale across archictectures that may support this api later as these may have more or different needs. Pass in a perf_event_attr structure instead because it is meant to evolve as much as possible into a generic hardware breakpoint parameter structure. Reported-by: K.Prasad <prasad@linux.vnet.ibm.com> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> LKML-Reference: <1259294154-5197-1-git-send-regression-fweisbec@gmail.com> Signed-off-by: Ingo Molnar <mingo@elte.hu>

hw-breakpoints: Use struct perf_event_attr to define user breakpoints
In-kernel user breakpoints are created using functions in which we pass breakpoint parameters as individual variables: address, length and type. Although it fits well for x86, this just does not scale across archictectures that may support this api later as these may have more or different needs. Pass in a perf_event_attr structure instead because it is meant to evolve as much as possible into a generic hardware breakpoint parameter structure. Reported-by: K.Prasad <prasad@linux.vnet.ibm.com> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> LKML-Reference: <1259294154-5197-1-git-send-regression-fweisbec@gmail.com> Signed-off-by: Ingo Molnar <mingo@elte.hu>
Frederic Weisbecker · Ingo Molnar
1 parent b2e74a265d
Showing 3 changed files with 75 additions and 122 deletions Side-by-side Diff
arch/x86/kernel/ptrace.c
include/linux/hw_breakpoint.h
kernel/hw_breakpoint.c
@@ -593,6 +593,34 @@
 	return dr7;
 }
  
+static struct perf_event *
+ptrace_modify_breakpoint(struct perf_event *bp, int len, int type,
+			 struct task_struct *tsk)
+{
+	int err;
+	int gen_len, gen_type;
+	DEFINE_BREAKPOINT_ATTR(attr);
+
+	/*
+	 * We shoud have at least an inactive breakpoint at this
+	 * slot. It means the user is writing dr7 without having
+	 * written the address register first
+	 */
+	if (!bp)
+		return ERR_PTR(-EINVAL);
+
+	err = arch_bp_generic_fields(len, type, &gen_len, &gen_type);
+	if (err)
+		return ERR_PTR(err);
+
+	attr = bp->attr;
+	attr.bp_len = gen_len;
+	attr.bp_type = gen_type;
+	attr.disabled = 0;
+
+	return modify_user_hw_breakpoint(bp, &attr, bp->callback, tsk);
+}
+
 /*
  * Handle ptrace writes to debug register 7.
  */
@@ -603,7 +631,6 @@
 	int i, orig_ret = 0, rc = 0;
 	int enabled, second_pass = 0;
 	unsigned len, type;
-	int gen_len, gen_type;
 	struct perf_event *bp;
  
 	data &= ~DR_CONTROL_RESERVED;
  
  
@@ -634,33 +661,12 @@
 			continue;
 		}
  
-		/*
-		 * We shoud have at least an inactive breakpoint at this
-		 * slot. It means the user is writing dr7 without having
-		 * written the address register first
-		 */
-		if (!bp) {
-			rc = -EINVAL;
-			break;
-		}
+		bp = ptrace_modify_breakpoint(bp, len, type, tsk);
  
-		rc = arch_bp_generic_fields(len, type, &gen_len, &gen_type);
-		if (rc)
-			break;
-
-		/*
-		 * This is a temporary thing as bp is unregistered/registered
-		 * to simulate modification
-		 */
-		bp = modify_user_hw_breakpoint(bp, bp->attr.bp_addr, gen_len,
-					       gen_type, bp->callback,
-					       tsk, true);
-		thread->ptrace_bps[i] = NULL;
-
 		/* Incorrect bp, or we have a bug in bp API */
 		if (IS_ERR(bp)) {
 			rc = PTR_ERR(bp);
-			bp = NULL;
+			thread->ptrace_bps[i] = NULL;
 			break;
 		}
 		thread->ptrace_bps[i] = bp;
  
  
@@ -707,24 +713,26 @@
 {
 	struct perf_event *bp;
 	struct thread_struct *t = &tsk->thread;
+	DEFINE_BREAKPOINT_ATTR(attr);
  
 	if (!t->ptrace_bps[nr]) {
 		/*
 		 * Put stub len and type to register (reserve) an inactive but
 		 * correct bp
 		 */
-		bp = register_user_hw_breakpoint(addr, HW_BREAKPOINT_LEN_1,
-						 HW_BREAKPOINT_W,
-						 ptrace_triggered, tsk,
-						 false);
+		attr.bp_addr = addr;
+		attr.bp_len = HW_BREAKPOINT_LEN_1;
+		attr.bp_type = HW_BREAKPOINT_W;
+		attr.disabled = 1;
+
+		bp = register_user_hw_breakpoint(&attr, ptrace_triggered, tsk);
 	} else {
 		bp = t->ptrace_bps[nr];
 		t->ptrace_bps[nr] = NULL;
-		bp = modify_user_hw_breakpoint(bp, addr, bp->attr.bp_len,
-					       bp->attr.bp_type,
-					       bp->callback,
-					       tsk,
-					       bp->attr.disabled);
+
+		attr = bp->attr;
+		attr.bp_addr = addr;
+		bp = modify_user_hw_breakpoint(bp, &attr, bp->callback, tsk);
 	}
 	/*
 	 * CHECKME: the previous code returned -EIO if the addr wasn't a
@@ -20,6 +20,14 @@
  
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
  
+/* As it's for in-kernel or ptrace use, we want it to be pinned */
+#define DEFINE_BREAKPOINT_ATTR(name)	\
+struct perf_event_attr name = {		\
+	.type = PERF_TYPE_BREAKPOINT,	\
+	.size = sizeof(name),		\
+	.pinned = 1,			\
+};
+
 static inline unsigned long hw_breakpoint_addr(struct perf_event *bp)
 {
 	return bp->attr.bp_addr;
  
  
  
@@ -36,22 +44,16 @@
 }
  
 extern struct perf_event *
-register_user_hw_breakpoint(unsigned long addr,
-			    int len,
-			    int type,
+register_user_hw_breakpoint(struct perf_event_attr *attr,
 			    perf_callback_t triggered,
-			    struct task_struct *tsk,
-			    bool active);
+			    struct task_struct *tsk);
  
 /* FIXME: only change from the attr, and don't unregister */
 extern struct perf_event *
 modify_user_hw_breakpoint(struct perf_event *bp,
-			  unsigned long addr,
-			  int len,
-			  int type,
+			  struct perf_event_attr *attr,
 			  perf_callback_t triggered,
-			  struct task_struct *tsk,
-			  bool active);
+			  struct task_struct *tsk);
  
 /*
  * Kernel breakpoints are not associated with any particular thread.
  
  
  
@@ -89,20 +91,14 @@
 #else /* !CONFIG_HAVE_HW_BREAKPOINT */
  
 static inline struct perf_event *
-register_user_hw_breakpoint(unsigned long addr,
-			    int len,
-			    int type,
+register_user_hw_breakpoint(struct perf_event_attr *attr,
 			    perf_callback_t triggered,
-			    struct task_struct *tsk,
-			    bool active)		{ return NULL; }
+			    struct task_struct *tsk)	{ return NULL; }
 static inline struct perf_event *
 modify_user_hw_breakpoint(struct perf_event *bp,
-			  unsigned long addr,
-			  int len,
-			  int type,
+			  struct perf_event_attr *attr,
 			  perf_callback_t triggered,
-			  struct task_struct *tsk,
-			  bool active)			{ return NULL; }
+			  struct task_struct *tsk)	{ return NULL; }
 static inline struct perf_event *
 register_wide_hw_breakpoint_cpu(unsigned long addr,
 				int len,
@@ -289,90 +289,32 @@
 	return __register_perf_hw_breakpoint(bp);
 }
  
-/*
- * Register a breakpoint bound to a task and a given cpu.
- * If cpu is -1, the breakpoint is active for the task in every cpu
- * If the task is -1, the breakpoint is active for every tasks in the given
- * cpu.
- */
-static struct perf_event *
-register_user_hw_breakpoint_cpu(unsigned long addr,
-				int len,
-				int type,
-				perf_callback_t triggered,
-				pid_t pid,
-				int cpu,
-				bool active)
-{
-	struct perf_event_attr *attr;
-	struct perf_event *bp;
-
-	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
-	if (!attr)
-		return ERR_PTR(-ENOMEM);
-
-	attr->type = PERF_TYPE_BREAKPOINT;
-	attr->size = sizeof(*attr);
-	attr->bp_addr = addr;
-	attr->bp_len = len;
-	attr->bp_type = type;
-	/*
-	 * Such breakpoints are used by debuggers to trigger signals when
-	 * we hit the excepted memory op. We can't miss such events, they
-	 * must be pinned.
-	 */
-	attr->pinned = 1;
-
-	if (!active)
-		attr->disabled = 1;
-
-	bp = perf_event_create_kernel_counter(attr, cpu, pid, triggered);
-	kfree(attr);
-
-	return bp;
-}
-
 /**
  * register_user_hw_breakpoint - register a hardware breakpoint for user space
- * @addr: is the memory address that triggers the breakpoint
- * @len: the length of the access to the memory (1 byte, 2 bytes etc...)
- * @type: the type of the access to the memory (read/write/exec)
+ * @attr: breakpoint attributes
  * @triggered: callback to trigger when we hit the breakpoint
  * @tsk: pointer to 'task_struct' of the process to which the address belongs
- * @active: should we activate it while registering it
- *
  */
 struct perf_event *
-register_user_hw_breakpoint(unsigned long addr,
-			    int len,
-			    int type,
+register_user_hw_breakpoint(struct perf_event_attr *attr,
 			    perf_callback_t triggered,
-			    struct task_struct *tsk,
-			    bool active)
+			    struct task_struct *tsk)
 {
-	return register_user_hw_breakpoint_cpu(addr, len, type, triggered,
-					       tsk->pid, -1, active);
+	return perf_event_create_kernel_counter(attr, -1, tsk->pid, triggered);
 }
 EXPORT_SYMBOL_GPL(register_user_hw_breakpoint);
  
 /**
  * modify_user_hw_breakpoint - modify a user-space hardware breakpoint
  * @bp: the breakpoint structure to modify
- * @addr: is the memory address that triggers the breakpoint
- * @len: the length of the access to the memory (1 byte, 2 bytes etc...)
- * @type: the type of the access to the memory (read/write/exec)
+ * @attr: new breakpoint attributes
  * @triggered: callback to trigger when we hit the breakpoint
  * @tsk: pointer to 'task_struct' of the process to which the address belongs
- * @active: should we activate it while registering it
  */
 struct perf_event *
-modify_user_hw_breakpoint(struct perf_event *bp,
-			  unsigned long addr,
-			  int len,
-			  int type,
+modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr,
 			  perf_callback_t triggered,
-			  struct task_struct *tsk,
-			  bool active)
+			  struct task_struct *tsk)
 {
 	/*
 	 * FIXME: do it without unregistering
@@ -381,8 +323,7 @@
 	 */
 	unregister_hw_breakpoint(bp);
  
-	return register_user_hw_breakpoint(addr, len, type, triggered,
-					   tsk, active);
+	return perf_event_create_kernel_counter(attr, -1, tsk->pid, triggered);
 }
 EXPORT_SYMBOL_GPL(modify_user_hw_breakpoint);
  
@@ -406,8 +347,16 @@
 				  int cpu,
 				  bool active)
 {
-	return register_user_hw_breakpoint_cpu(addr, len, type, triggered,
-					       -1, cpu, active);
+	DEFINE_BREAKPOINT_ATTR(attr);
+
+	attr.bp_addr = addr;
+	attr.bp_len = len;
+	attr.bp_type = type;
+
+	if (!active)
+		attr.disabled = 1;
+
+	return perf_event_create_kernel_counter(&attr, cpu, -1, triggered);
 }
  
 /**
...	...	@@ -593,6 +593,34 @@
593	593	return dr7;
594	594	}
595	595
	596	+static struct perf_event *
	597	+ptrace_modify_breakpoint(struct perf_event *bp, int len, int type,
	598	+ struct task_struct *tsk)
	599	+{
	600	+ int err;
	601	+ int gen_len, gen_type;
	602	+ DEFINE_BREAKPOINT_ATTR(attr);
	603	+
	604	+ /*
	605	+ * We shoud have at least an inactive breakpoint at this
	606	+ * slot. It means the user is writing dr7 without having
	607	+ * written the address register first
	608	+ */
	609	+ if (!bp)
	610	+ return ERR_PTR(-EINVAL);
	611	+
	612	+ err = arch_bp_generic_fields(len, type, &gen_len, &gen_type);
	613	+ if (err)
	614	+ return ERR_PTR(err);
	615	+
	616	+ attr = bp->attr;
	617	+ attr.bp_len = gen_len;
	618	+ attr.bp_type = gen_type;
	619	+ attr.disabled = 0;
	620	+
	621	+ return modify_user_hw_breakpoint(bp, &attr, bp->callback, tsk);
	622	+}
	623	+
596	624	/*
597	625	* Handle ptrace writes to debug register 7.
598	626	*/
...	...	@@ -603,7 +631,6 @@
603	631	int i, orig_ret = 0, rc = 0;
604	632	int enabled, second_pass = 0;
605	633	unsigned len, type;
606		- int gen_len, gen_type;
607	634	struct perf_event *bp;
608	635
609	636	data &= ~DR_CONTROL_RESERVED;
610	637
611	638
...	...	@@ -634,33 +661,12 @@
634	661	continue;
635	662	}
636	663
637		- /*
638		- * We shoud have at least an inactive breakpoint at this
639		- * slot. It means the user is writing dr7 without having
640		- * written the address register first
641		- */
642		- if (!bp) {
643		- rc = -EINVAL;
644		- break;
645		- }
	664	+ bp = ptrace_modify_breakpoint(bp, len, type, tsk);
646	665
647		- rc = arch_bp_generic_fields(len, type, &gen_len, &gen_type);
648		- if (rc)
649		- break;
650		-
651		- /*
652		- * This is a temporary thing as bp is unregistered/registered
653		- * to simulate modification
654		- */
655		- bp = modify_user_hw_breakpoint(bp, bp->attr.bp_addr, gen_len,
656		- gen_type, bp->callback,
657		- tsk, true);
658		- thread->ptrace_bps[i] = NULL;
659		-
660	666	/* Incorrect bp, or we have a bug in bp API */
661	667	if (IS_ERR(bp)) {
662	668	rc = PTR_ERR(bp);
663		- bp = NULL;
	669	+ thread->ptrace_bps[i] = NULL;
664	670	break;
665	671	}
666	672	thread->ptrace_bps[i] = bp;
667	673
668	674
...	...	@@ -707,24 +713,26 @@
707	713	{
708	714	struct perf_event *bp;
709	715	struct thread_struct *t = &tsk->thread;
	716	+ DEFINE_BREAKPOINT_ATTR(attr);
710	717
711	718	if (!t->ptrace_bps[nr]) {
712	719	/*
713	720	* Put stub len and type to register (reserve) an inactive but
714	721	* correct bp
715	722	*/
716		- bp = register_user_hw_breakpoint(addr, HW_BREAKPOINT_LEN_1,
717		- HW_BREAKPOINT_W,
718		- ptrace_triggered, tsk,
719		- false);
	723	+ attr.bp_addr = addr;
	724	+ attr.bp_len = HW_BREAKPOINT_LEN_1;
	725	+ attr.bp_type = HW_BREAKPOINT_W;
	726	+ attr.disabled = 1;
	727	+
	728	+ bp = register_user_hw_breakpoint(&attr, ptrace_triggered, tsk);
720	729	} else {
721	730	bp = t->ptrace_bps[nr];
722	731	t->ptrace_bps[nr] = NULL;
723		- bp = modify_user_hw_breakpoint(bp, addr, bp->attr.bp_len,
724		- bp->attr.bp_type,
725		- bp->callback,
726		- tsk,
727		- bp->attr.disabled);
	732	+
	733	+ attr = bp->attr;
	734	+ attr.bp_addr = addr;
	735	+ bp = modify_user_hw_breakpoint(bp, &attr, bp->callback, tsk);
728	736	}
729	737	/*
730	738	* CHECKME: the previous code returned -EIO if the addr wasn't a
...	...	@@ -20,6 +20,14 @@
20	20
21	21	#ifdef CONFIG_HAVE_HW_BREAKPOINT
22	22
	23	+/* As it's for in-kernel or ptrace use, we want it to be pinned */
	24	+#define DEFINE_BREAKPOINT_ATTR(name) \
	25	+struct perf_event_attr name = { \
	26	+ .type = PERF_TYPE_BREAKPOINT, \
	27	+ .size = sizeof(name), \
	28	+ .pinned = 1, \
	29	+};
	30	+
23	31	static inline unsigned long hw_breakpoint_addr(struct perf_event *bp)
24	32	{
25	33	return bp->attr.bp_addr;
26	34
27	35
28	36
...	...	@@ -36,22 +44,16 @@
36	44	}
37	45
38	46	extern struct perf_event *
39		-register_user_hw_breakpoint(unsigned long addr,
40		- int len,
41		- int type,
	47	+register_user_hw_breakpoint(struct perf_event_attr *attr,
42	48	perf_callback_t triggered,
43		- struct task_struct *tsk,
44		- bool active);
	49	+ struct task_struct *tsk);
45	50
46	51	/* FIXME: only change from the attr, and don't unregister */
47	52	extern struct perf_event *
48	53	modify_user_hw_breakpoint(struct perf_event *bp,
49		- unsigned long addr,
50		- int len,
51		- int type,
	54	+ struct perf_event_attr *attr,
52	55	perf_callback_t triggered,
53		- struct task_struct *tsk,
54		- bool active);
	56	+ struct task_struct *tsk);
55	57
56	58	/*
57	59	* Kernel breakpoints are not associated with any particular thread.
58	60
59	61
60	62
...	...	@@ -89,20 +91,14 @@
89	91	#else /* !CONFIG_HAVE_HW_BREAKPOINT */
90	92
91	93	static inline struct perf_event *
92		-register_user_hw_breakpoint(unsigned long addr,
93		- int len,
94		- int type,
	94	+register_user_hw_breakpoint(struct perf_event_attr *attr,
95	95	perf_callback_t triggered,
96		- struct task_struct *tsk,
97		- bool active) { return NULL; }
	96	+ struct task_struct *tsk) { return NULL; }
98	97	static inline struct perf_event *
99	98	modify_user_hw_breakpoint(struct perf_event *bp,
100		- unsigned long addr,
101		- int len,
102		- int type,
	99	+ struct perf_event_attr *attr,
103	100	perf_callback_t triggered,
104		- struct task_struct *tsk,
105		- bool active) { return NULL; }
	101	+ struct task_struct *tsk) { return NULL; }
106	102	static inline struct perf_event *
107	103	register_wide_hw_breakpoint_cpu(unsigned long addr,
108	104	int len,
...	...	@@ -289,90 +289,32 @@
289	289	return __register_perf_hw_breakpoint(bp);
290	290	}
291	291
292		-/*
293		- * Register a breakpoint bound to a task and a given cpu.
294		- * If cpu is -1, the breakpoint is active for the task in every cpu
295		- * If the task is -1, the breakpoint is active for every tasks in the given
296		- * cpu.
297		- */
298		-static struct perf_event *
299		-register_user_hw_breakpoint_cpu(unsigned long addr,
300		- int len,
301		- int type,
302		- perf_callback_t triggered,
303		- pid_t pid,
304		- int cpu,
305		- bool active)
306		-{
307		- struct perf_event_attr *attr;
308		- struct perf_event *bp;
309		-
310		- attr = kzalloc(sizeof(*attr), GFP_KERNEL);
311		- if (!attr)
312		- return ERR_PTR(-ENOMEM);
313		-
314		- attr->type = PERF_TYPE_BREAKPOINT;
315		- attr->size = sizeof(*attr);
316		- attr->bp_addr = addr;
317		- attr->bp_len = len;
318		- attr->bp_type = type;
319		- /*
320		- * Such breakpoints are used by debuggers to trigger signals when
321		- * we hit the excepted memory op. We can't miss such events, they
322		- * must be pinned.
323		- */
324		- attr->pinned = 1;
325		-
326		- if (!active)
327		- attr->disabled = 1;
328		-
329		- bp = perf_event_create_kernel_counter(attr, cpu, pid, triggered);
330		- kfree(attr);
331		-
332		- return bp;
333		-}
334		-
335	292	/**
336	293	* register_user_hw_breakpoint - register a hardware breakpoint for user space
337		- * @addr: is the memory address that triggers the breakpoint
338		- * @len: the length of the access to the memory (1 byte, 2 bytes etc...)
339		- * @type: the type of the access to the memory (read/write/exec)
	294	+ * @attr: breakpoint attributes
340	295	* @triggered: callback to trigger when we hit the breakpoint
341	296	* @tsk: pointer to 'task_struct' of the process to which the address belongs
342		- * @active: should we activate it while registering it
343		- *
344	297	*/
345	298	struct perf_event *
346		-register_user_hw_breakpoint(unsigned long addr,
347		- int len,
348		- int type,
	299	+register_user_hw_breakpoint(struct perf_event_attr *attr,
349	300	perf_callback_t triggered,
350		- struct task_struct *tsk,
351		- bool active)
	301	+ struct task_struct *tsk)
352	302	{
353		- return register_user_hw_breakpoint_cpu(addr, len, type, triggered,
354		- tsk->pid, -1, active);
	303	+ return perf_event_create_kernel_counter(attr, -1, tsk->pid, triggered);
355	304	}
356	305	EXPORT_SYMBOL_GPL(register_user_hw_breakpoint);
357	306
358	307	/**
359	308	* modify_user_hw_breakpoint - modify a user-space hardware breakpoint
360	309	* @bp: the breakpoint structure to modify
361		- * @addr: is the memory address that triggers the breakpoint
362		- * @len: the length of the access to the memory (1 byte, 2 bytes etc...)
363		- * @type: the type of the access to the memory (read/write/exec)
	310	+ * @attr: new breakpoint attributes
364	311	* @triggered: callback to trigger when we hit the breakpoint
365	312	* @tsk: pointer to 'task_struct' of the process to which the address belongs
366		- * @active: should we activate it while registering it
367	313	*/
368	314	struct perf_event *
369		-modify_user_hw_breakpoint(struct perf_event *bp,
370		- unsigned long addr,
371		- int len,
372		- int type,
	315	+modify_user_hw_breakpoint(struct perf_event bp, struct perf_event_attr attr,
373	316	perf_callback_t triggered,
374		- struct task_struct *tsk,
375		- bool active)
	317	+ struct task_struct *tsk)
376	318	{
377	319	/*
378	320	* FIXME: do it without unregistering
...	...	@@ -381,8 +323,7 @@
381	323	*/
382	324	unregister_hw_breakpoint(bp);
383	325
384		- return register_user_hw_breakpoint(addr, len, type, triggered,
385		- tsk, active);
	326	+ return perf_event_create_kernel_counter(attr, -1, tsk->pid, triggered);
386	327	}
387	328	EXPORT_SYMBOL_GPL(modify_user_hw_breakpoint);
388	329
...	...	@@ -406,8 +347,16 @@
406	347	int cpu,
407	348	bool active)
408	349	{
409		- return register_user_hw_breakpoint_cpu(addr, len, type, triggered,
410		- -1, cpu, active);
	350	+ DEFINE_BREAKPOINT_ATTR(attr);
	351	+
	352	+ attr.bp_addr = addr;
	353	+ attr.bp_len = len;
	354	+ attr.bp_type = type;
	355	+
	356	+ if (!active)
	357	+ attr.disabled = 1;
	358	+
	359	+ return perf_event_create_kernel_counter(&attr, cpu, -1, triggered);
411	360	}
412	361
413	362	/**