diff mbox series

perf/core: Avoid freeing static PMU contexts when PMU is unregistered

Message ID 1507040450-7730-1-git-send-email-will.deacon@arm.com
State Accepted
Commit df0062b27ebf473b372914a3e3574d93790e2b72
Headers show
Series perf/core: Avoid freeing static PMU contexts when PMU is unregistered | expand

Commit Message

Will Deacon Oct. 3, 2017, 2:20 p.m. UTC 
Since commit 1fd7e4169954 ("perf/core: Remove perf_cpu_context::unique_pmu"),
when a PMU is unregistered then its associated ->pmu_cpu_context is
unconditionally freed. Whilst this is fine for dynamically allocated
context types (i.e. those registered using perf_invalid_context), this
causes a problem for sharing of static contexts such as
perf_{sw,hw}_context, which are used by multiple built-in PMUs and
effectively have a global lifetime.

Whilst testing the ARM SPE driver, which must use perf_sw_context to
support per-task AUX tracing, unregistering the driver as a result of a
module unload resulted in:

 Unable to handle kernel NULL pointer dereference at virtual address 00000038
 Internal error: Oops: 96000004 [#1] PREEMPT SMP
 Modules linked in: [last unloaded: arm_spe_pmu]
 PC is at ctx_resched+0x38/0xe8
 LR is at perf_event_exec+0x20c/0x278
 [...]
 ctx_resched+0x38/0xe8
 perf_event_exec+0x20c/0x278
 setup_new_exec+0x88/0x118
 load_elf_binary+0x26c/0x109c
 search_binary_handler+0x90/0x298
 do_execveat_common.isra.14+0x540/0x618
 SyS_execve+0x38/0x48

since the software context has been freed and the ctx.pmu->pmu_disable_count
field has been set to NULL.

This patch fixes the problem by avoiding the freeing of static PMU contexts
altogether. Whilst the sharing of dynamic contexts is questionable, this
actually requires the caller to share their context pointer explicitly
and so the burden is on them to manage the object lifetime.

Cc: Peter Zijlstra <peterz@infradead.org>
Fixes: 1fd7e4169954 ("perf/core: Remove perf_cpu_context::unique_pmu")
Reported-by: Kim Phillips <kim.phillips@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>

---
 kernel/events/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.1.4

Comments

Mark Rutland Oct. 3, 2017, 2:58 p.m. UTC | #1 
On Tue, Oct 03, 2017 at 03:20:50PM +0100, Will Deacon wrote:
> Since commit 1fd7e4169954 ("perf/core: Remove perf_cpu_context::unique_pmu"),

> when a PMU is unregistered then its associated ->pmu_cpu_context is

> unconditionally freed. Whilst this is fine for dynamically allocated

> context types (i.e. those registered using perf_invalid_context), this

> causes a problem for sharing of static contexts such as

> perf_{sw,hw}_context, which are used by multiple built-in PMUs and

> effectively have a global lifetime.

> 

> Whilst testing the ARM SPE driver, which must use perf_sw_context to

> support per-task AUX tracing, unregistering the driver as a result of a

> module unload resulted in:

> 

>  Unable to handle kernel NULL pointer dereference at virtual address 00000038

>  Internal error: Oops: 96000004 [#1] PREEMPT SMP

>  Modules linked in: [last unloaded: arm_spe_pmu]

>  PC is at ctx_resched+0x38/0xe8

>  LR is at perf_event_exec+0x20c/0x278

>  [...]

>  ctx_resched+0x38/0xe8

>  perf_event_exec+0x20c/0x278

>  setup_new_exec+0x88/0x118

>  load_elf_binary+0x26c/0x109c

>  search_binary_handler+0x90/0x298

>  do_execveat_common.isra.14+0x540/0x618

>  SyS_execve+0x38/0x48

> 

> since the software context has been freed and the ctx.pmu->pmu_disable_count

> field has been set to NULL.

> 

> This patch fixes the problem by avoiding the freeing of static PMU contexts

> altogether. Whilst the sharing of dynamic contexts is questionable, this

> actually requires the caller to share their context pointer explicitly

> and so the burden is on them to manage the object lifetime.

> 

> Cc: Peter Zijlstra <peterz@infradead.org>

> Fixes: 1fd7e4169954 ("perf/core: Remove perf_cpu_context::unique_pmu")

> Reported-by: Kim Phillips <kim.phillips@arm.com>

> Signed-off-by: Will Deacon <will.deacon@arm.com>


FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>


Mark.

> ---

>  kernel/events/core.c | 8 ++++++++

>  1 file changed, 8 insertions(+)

> 

> diff --git a/kernel/events/core.c b/kernel/events/core.c

> index 6bc21e202ae4..243bfc68d0fe 100644

> --- a/kernel/events/core.c

> +++ b/kernel/events/core.c

> @@ -8955,6 +8955,14 @@ static struct perf_cpu_context __percpu *find_pmu_context(int ctxn)

>  

>  static void free_pmu_context(struct pmu *pmu)

>  {

> +	/*

> +	 * Static contexts such as perf_sw_context have a global lifetime

> +	 * and may be shared between different PMUs. Avoid freeing them

> +	 * when a single PMU is going away.

> +	 */

> +	if (pmu->task_ctx_nr > perf_invalid_context)

> +		return;

> +

>  	mutex_lock(&pmus_lock);

>  	free_percpu(pmu->pmu_cpu_context);

>  	mutex_unlock(&pmus_lock);

> -- 

> 2.1.4

>
Kim Phillips Oct. 4, 2017, 3:31 p.m. UTC | #2 
On Tue, 3 Oct 2017 15:58:13 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Tue, Oct 03, 2017 at 03:20:50PM +0100, Will Deacon wrote:

> > Since commit 1fd7e4169954 ("perf/core: Remove perf_cpu_context::unique_pmu"),

> > when a PMU is unregistered then its associated ->pmu_cpu_context is

> > unconditionally freed. Whilst this is fine for dynamically allocated

> > context types (i.e. those registered using perf_invalid_context), this

> > causes a problem for sharing of static contexts such as

> > perf_{sw,hw}_context, which are used by multiple built-in PMUs and

> > effectively have a global lifetime.

> > 

> > Whilst testing the ARM SPE driver, which must use perf_sw_context to

> > support per-task AUX tracing, unregistering the driver as a result of a

> > module unload resulted in:

> > 

> >  Unable to handle kernel NULL pointer dereference at virtual address 00000038

> >  Internal error: Oops: 96000004 [#1] PREEMPT SMP

> >  Modules linked in: [last unloaded: arm_spe_pmu]

> >  PC is at ctx_resched+0x38/0xe8

> >  LR is at perf_event_exec+0x20c/0x278

> >  [...]

> >  ctx_resched+0x38/0xe8

> >  perf_event_exec+0x20c/0x278

> >  setup_new_exec+0x88/0x118

> >  load_elf_binary+0x26c/0x109c

> >  search_binary_handler+0x90/0x298

> >  do_execveat_common.isra.14+0x540/0x618

> >  SyS_execve+0x38/0x48

> > 

> > since the software context has been freed and the ctx.pmu->pmu_disable_count

> > field has been set to NULL.

> > 

> > This patch fixes the problem by avoiding the freeing of static PMU contexts

> > altogether. Whilst the sharing of dynamic contexts is questionable, this

> > actually requires the caller to share their context pointer explicitly

> > and so the burden is on them to manage the object lifetime.

> > 

> > Cc: Peter Zijlstra <peterz@infradead.org>

> > Fixes: 1fd7e4169954 ("perf/core: Remove perf_cpu_context::unique_pmu")

> > Reported-by: Kim Phillips <kim.phillips@arm.com>

> > Signed-off-by: Will Deacon <will.deacon@arm.com>

> 

> Acked-by: Mark Rutland <mark.rutland@arm.com>


Tested-by: Kim Phillips <kim.phillips@arm.com>


Thanks,

Kim
diff mbox series

Patch

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 6bc21e202ae4..243bfc68d0fe 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8955,6 +8955,14 @@  static struct perf_cpu_context __percpu *find_pmu_context(int ctxn)
 
 static void free_pmu_context(struct pmu *pmu)
 {
+	/*
+	 * Static contexts such as perf_sw_context have a global lifetime
+	 * and may be shared between different PMUs. Avoid freeing them
+	 * when a single PMU is going away.
+	 */
+	if (pmu->task_ctx_nr > perf_invalid_context)
+		return;
+
 	mutex_lock(&pmus_lock);
 	free_percpu(pmu->pmu_cpu_context);
 	mutex_unlock(&pmus_lock);