linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] perf/x86: set pmu->module in Intel PMU modules
@ 2016-12-23  1:17 David Carrillo-Cisneros
  2017-01-05 15:07 ` [tip:perf/urgent] perf/x86: Set " tip-bot for David Carrillo-Cisneros
  0 siblings, 1 reply; 5+ messages in thread
From: David Carrillo-Cisneros @ 2016-12-23  1:17 UTC (permalink / raw)
  To: linux-kernel
  Cc: x86, Ingo Molnar, Thomas Gleixner, Andi Kleen, Kan Liang,
	Peter Zijlstra, Borislav Petkov, Srinivas Pandruvada,
	Dave Hansen, Paul Turner, Stephane Eranian,
	David Carrillo-Cisneros

The conversion of Intel PMU drivers into modules did not include reference
counting. The machine will crash when attempting to  access deleted code
if an event from a module PMU is started and the module removed before the
event is destroyed.

i.e. this crashes the machine:

	$ insmod intel-rapl-perf.ko
	$ perf stat -e power/energy-cores/ -C 0 &
	$ rmmod intel-rapl-perf.ko

Set THIS_MODULE to pmu->module in Intel module PMUs so that generic code
can handle reference counting and deny rmmod while an event still exists.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 arch/x86/events/intel/cstate.c | 2 ++
 arch/x86/events/intel/rapl.c   | 1 +
 arch/x86/events/intel/uncore.c | 1 +
 3 files changed, 4 insertions(+)

diff --git a/arch/x86/events/intel/cstate.c b/arch/x86/events/intel/cstate.c
index da51e5a..5c08fc7 100644
--- a/arch/x86/events/intel/cstate.c
+++ b/arch/x86/events/intel/cstate.c
@@ -434,6 +434,7 @@ static struct pmu cstate_core_pmu = {
 	.stop		= cstate_pmu_event_stop,
 	.read		= cstate_pmu_event_update,
 	.capabilities	= PERF_PMU_CAP_NO_INTERRUPT,
+	.module		= THIS_MODULE,
 };
 
 static struct pmu cstate_pkg_pmu = {
@@ -447,6 +448,7 @@ static struct pmu cstate_pkg_pmu = {
 	.stop		= cstate_pmu_event_stop,
 	.read		= cstate_pmu_event_update,
 	.capabilities	= PERF_PMU_CAP_NO_INTERRUPT,
+	.module		= THIS_MODULE,
 };
 
 static const struct cstate_model nhm_cstates __initconst = {
diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index 0a535ce..6f2cbd5 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -697,6 +697,7 @@ static int __init init_rapl_pmus(void)
 	rapl_pmus->pmu.start		= rapl_pmu_event_start;
 	rapl_pmus->pmu.stop		= rapl_pmu_event_stop;
 	rapl_pmus->pmu.read		= rapl_pmu_event_read;
+	rapl_pmus->pmu.module		= THIS_MODULE;
 	return 0;
 }
 
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index dbaaf7dc..2fe525b 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -733,6 +733,7 @@ static int uncore_pmu_register(struct intel_uncore_pmu *pmu)
 			.start		= uncore_pmu_event_start,
 			.stop		= uncore_pmu_event_stop,
 			.read		= uncore_pmu_event_read,
+			.module		= THIS_MODULE,
 		};
 	} else {
 		pmu->pmu = *pmu->type->pmu;
-- 
2.8.0.rc3.226.g39d4020

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [tip:perf/urgent] perf/x86: Set pmu->module in Intel PMU modules
  2016-12-23  1:17 [PATCH] perf/x86: set pmu->module in Intel PMU modules David Carrillo-Cisneros
@ 2017-01-05 15:07 ` tip-bot for David Carrillo-Cisneros
  2017-01-05 15:52   ` Peter Zijlstra
  0 siblings, 1 reply; 5+ messages in thread
From: tip-bot for David Carrillo-Cisneros @ 2017-01-05 15:07 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: peterz, pjt, bp, mingo, torvalds, eranian, alexander.shishkin,
	dave.hansen, tglx, jolsa, srinivas.pandruvada, acme, hpa,
	kan.liang, linux-kernel, davidcc

Commit-ID:  74545f63890e38520eb4d1dbedcadaa9c0dbc824
Gitweb:     http://git.kernel.org/tip/74545f63890e38520eb4d1dbedcadaa9c0dbc824
Author:     David Carrillo-Cisneros <davidcc@google.com>
AuthorDate: Thu, 22 Dec 2016 17:17:40 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 5 Jan 2017 09:13:55 +0100

perf/x86: Set pmu->module in Intel PMU modules

The conversion of Intel PMU drivers into modules did not include reference
counting. The machine will crash when attempting to  access deleted code
if an event from a module PMU is started and the module removed before the
event is destroyed.

i.e. this crashes the machine:

	$ insmod intel-rapl-perf.ko
	$ perf stat -e power/energy-cores/ -C 0 &
	$ rmmod intel-rapl-perf.ko

Set THIS_MODULE to pmu->module in Intel module PMUs so that generic code
can handle reference counting and deny rmmod while an event still exists.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul Turner <pjt@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1482455860-116269-1-git-send-email-davidcc@google.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/events/intel/cstate.c | 2 ++
 arch/x86/events/intel/rapl.c   | 1 +
 arch/x86/events/intel/uncore.c | 1 +
 3 files changed, 4 insertions(+)

diff --git a/arch/x86/events/intel/cstate.c b/arch/x86/events/intel/cstate.c
index fec8a46..1076c9a 100644
--- a/arch/x86/events/intel/cstate.c
+++ b/arch/x86/events/intel/cstate.c
@@ -434,6 +434,7 @@ static struct pmu cstate_core_pmu = {
 	.stop		= cstate_pmu_event_stop,
 	.read		= cstate_pmu_event_update,
 	.capabilities	= PERF_PMU_CAP_NO_INTERRUPT,
+	.module		= THIS_MODULE,
 };
 
 static struct pmu cstate_pkg_pmu = {
@@ -447,6 +448,7 @@ static struct pmu cstate_pkg_pmu = {
 	.stop		= cstate_pmu_event_stop,
 	.read		= cstate_pmu_event_update,
 	.capabilities	= PERF_PMU_CAP_NO_INTERRUPT,
+	.module		= THIS_MODULE,
 };
 
 static const struct cstate_model nhm_cstates __initconst = {
diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index bd34124..17c3564 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -697,6 +697,7 @@ static int __init init_rapl_pmus(void)
 	rapl_pmus->pmu.start		= rapl_pmu_event_start;
 	rapl_pmus->pmu.stop		= rapl_pmu_event_stop;
 	rapl_pmus->pmu.read		= rapl_pmu_event_read;
+	rapl_pmus->pmu.module		= THIS_MODULE;
 	return 0;
 }
 
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 97c246f..8c4ccdc 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -733,6 +733,7 @@ static int uncore_pmu_register(struct intel_uncore_pmu *pmu)
 			.start		= uncore_pmu_event_start,
 			.stop		= uncore_pmu_event_stop,
 			.read		= uncore_pmu_event_read,
+			.module		= THIS_MODULE,
 		};
 	} else {
 		pmu->pmu = *pmu->type->pmu;

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [tip:perf/urgent] perf/x86: Set pmu->module in Intel PMU modules
  2017-01-05 15:07 ` [tip:perf/urgent] perf/x86: Set " tip-bot for David Carrillo-Cisneros
@ 2017-01-05 15:52   ` Peter Zijlstra
  2017-01-05 18:46     ` Linus Torvalds
  0 siblings, 1 reply; 5+ messages in thread
From: Peter Zijlstra @ 2017-01-05 15:52 UTC (permalink / raw)
  To: srinivas.pandruvada, tglx, jolsa, dave.hansen, davidcc,
	linux-kernel, kan.liang, acme, hpa, bp, pjt, alexander.shishkin,
	eranian, mingo, torvalds
  Cc: linux-tip-commits

On Thu, Jan 05, 2017 at 07:07:05AM -0800, tip-bot for David Carrillo-Cisneros wrote:
> --- a/arch/x86/events/intel/cstate.c
> +++ b/arch/x86/events/intel/cstate.c
> @@ -434,6 +434,7 @@ static struct pmu cstate_core_pmu = {
>  	.stop		= cstate_pmu_event_stop,
>  	.read		= cstate_pmu_event_update,
>  	.capabilities	= PERF_PMU_CAP_NO_INTERRUPT,
> +	.module		= THIS_MODULE,
>  };
>  
>  static struct pmu cstate_pkg_pmu = {
> @@ -447,6 +448,7 @@ static struct pmu cstate_pkg_pmu = {
>  	.stop		= cstate_pmu_event_stop,
>  	.read		= cstate_pmu_event_update,
>  	.capabilities	= PERF_PMU_CAP_NO_INTERRUPT,
> +	.module		= THIS_MODULE,
>  };
>  
>  static const struct cstate_model nhm_cstates __initconst = {
> diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
> index bd34124..17c3564 100644
> --- a/arch/x86/events/intel/rapl.c
> +++ b/arch/x86/events/intel/rapl.c
> @@ -697,6 +697,7 @@ static int __init init_rapl_pmus(void)
>  	rapl_pmus->pmu.start		= rapl_pmu_event_start;
>  	rapl_pmus->pmu.stop		= rapl_pmu_event_stop;
>  	rapl_pmus->pmu.read		= rapl_pmu_event_read;
> +	rapl_pmus->pmu.module		= THIS_MODULE;
>  	return 0;
>  }
>  
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index 97c246f..8c4ccdc 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -733,6 +733,7 @@ static int uncore_pmu_register(struct intel_uncore_pmu *pmu)
>  			.start		= uncore_pmu_event_start,
>  			.stop		= uncore_pmu_event_stop,
>  			.read		= uncore_pmu_event_read,
> +			.module		= THIS_MODULE,
>  		};
>  	} else {
>  		pmu->pmu = *pmu->type->pmu;


Ah, forgot about this, thanks for picking it up.

The first time I saw this I thought about doing something like the
below, but never got around to testing if that works and subsequently
forgot about things again.

Does this make sense and or work?

---

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 4741ecdb9817..70f8a5a2e224 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -852,7 +852,14 @@ extern int perf_aux_output_skip(struct perf_output_handle *handle,
 				unsigned long size);
 extern void *perf_get_aux(struct perf_output_handle *handle);
 
-extern int perf_pmu_register(struct pmu *pmu, const char *name, int type);
+extern int __perf_pmu_register(struct pmu *pmu, const char *name, int type);
+
+#define perf_pmu_register(_pmu, _name, _type)			\
+({								\
+ 	(_pmu)->module = THIS_MODULE;				\
+ 	__perf_pmu_register((_pmu), (_name), (_type));		\
+})
+
 extern void perf_pmu_unregister(struct pmu *pmu);
 
 extern int perf_num_counters(void);
diff --git a/kernel/events/core.c b/kernel/events/core.c
index faf073d0287f..cde2004d932d 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8752,7 +8767,7 @@ static int pmu_dev_alloc(struct pmu *pmu)
 static struct lock_class_key cpuctx_mutex;
 static struct lock_class_key cpuctx_lock;
 
-int perf_pmu_register(struct pmu *pmu, const char *name, int type)
+int __perf_pmu_register(struct pmu *pmu, const char *name, int type)
 {
 	int cpu, ret;
 

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [tip:perf/urgent] perf/x86: Set pmu->module in Intel PMU modules
  2017-01-05 15:52   ` Peter Zijlstra
@ 2017-01-05 18:46     ` Linus Torvalds
  2017-01-05 20:24       ` Peter Zijlstra
  0 siblings, 1 reply; 5+ messages in thread
From: Linus Torvalds @ 2017-01-05 18:46 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Srinivas Pandruvada, Thomas Gleixner, Jiri Olsa, Dave Hansen,
	davidcc, Linux Kernel Mailing List, kan.liang,
	Arnaldo Carvalho de Melo, Peter Anvin, Borislav Petkov,
	Paul Turner, Alexander Shishkin, Stephane Eranian, Ingo Molnar,
	linux-tip-commits

On Thu, Jan 5, 2017 at 7:52 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> The first time I saw this I thought about doing something like the
> below, but never got around to testing if that works and subsequently
> forgot about things again.
>
> Does this make sense and or work?

I'd argue against it.

It will "work", but it's fragile. In particular, if something ever
ends up using perf_pmu_register() through some indirect helper
function, it will do the wrong thing, because

> +#define perf_pmu_register(_pmu, _name, _type)                  \
> +({                                                             \
> +       (_pmu)->module = THIS_MODULE;                           \
> +       __perf_pmu_register((_pmu), (_name), (_type));          \
> +})

.. at that point "THIS_MODULE" could be the module that contains the
helper function that may not actually be the actual end-point module.

Looking at the situation right now, that never happens and all the
users seem to be "leaf" modules. But it would worry me a bit.

                Linus

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [tip:perf/urgent] perf/x86: Set pmu->module in Intel PMU modules
  2017-01-05 18:46     ` Linus Torvalds
@ 2017-01-05 20:24       ` Peter Zijlstra
  0 siblings, 0 replies; 5+ messages in thread
From: Peter Zijlstra @ 2017-01-05 20:24 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Srinivas Pandruvada, Thomas Gleixner, Jiri Olsa, Dave Hansen,
	davidcc, Linux Kernel Mailing List, kan.liang,
	Arnaldo Carvalho de Melo, Peter Anvin, Borislav Petkov,
	Paul Turner, Alexander Shishkin, Stephane Eranian, Ingo Molnar,
	linux-tip-commits

On Thu, Jan 05, 2017 at 10:46:30AM -0800, Linus Torvalds wrote:
> On Thu, Jan 5, 2017 at 7:52 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > The first time I saw this I thought about doing something like the
> > below, but never got around to testing if that works and subsequently
> > forgot about things again.
> >
> > Does this make sense and or work?
> 
> I'd argue against it.

Fair enough; /me files it in the bit-bucket.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-01-05 20:52 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-23  1:17 [PATCH] perf/x86: set pmu->module in Intel PMU modules David Carrillo-Cisneros
2017-01-05 15:07 ` [tip:perf/urgent] perf/x86: Set " tip-bot for David Carrillo-Cisneros
2017-01-05 15:52   ` Peter Zijlstra
2017-01-05 18:46     ` Linus Torvalds
2017-01-05 20:24       ` Peter Zijlstra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).