* [PATCH 0/3] perf_events and hw_breakpoints fix and cleanup
@ 2010-09-13 20:01 Matt Helsley
2010-09-13 20:01 ` [PATCH 1/3] hw breakpoints: Fix pid namespace bug Matt Helsley
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Matt Helsley @ 2010-09-13 20:01 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
Fixes a bug in the way that hardward breakpoints pass around [v]pids.
The last two patches move the pid resolution closer to the syscall
and avoid passing pids entirely so that the perf event code does not
need to worry about being in the wrong pid namespace when kernel
counters are created.
NOTE: Applies to -tip perf/core. Compile-tested only at the moment.
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/3] hw breakpoints: Fix pid namespace bug 2010-09-13 20:01 [PATCH 0/3] perf_events and hw_breakpoints fix and cleanup Matt Helsley @ 2010-09-13 20:01 ` Matt Helsley 2010-09-13 20:01 ` [PATCH 2/3] perf events: Split out task search into helper Matt Helsley ` (3 more replies) [not found] ` <1284408080-2135-1-git-send-email-matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> 2010-09-13 20:08 ` Frederic Weisbecker 2 siblings, 4 replies; 18+ messages in thread From: Matt Helsley @ 2010-09-13 20:01 UTC (permalink / raw) To: Frederic Weisbecker Cc: containers, Matt Helsley, Robin Green, linux-kernel, Prasad, Peter Zijlstra, Arnaldo Carvalho de Melo, Steven Rostedt, Ingo Molnar, Will Deacon, Mahesh Salgaonkar Hardware breakpoints can't be registered within pid namespaces because tsk->pid is passed rather than the pid in the current namespace. (See https://bugzilla.kernel.org/show_bug.cgi?id=17281 ) This is a quick fix demonstrating the problem but is not the best method of solving the problem since passing pids internally is not the best way to avoid pid namespace bugs. Subsequent patches will show a better solution. Much thanks to Frederic Weisbecker <fweisbec@gmail.com> for doing the bulk of the work finding this bug. Cc: Robin Green <greenrd@greenrd.org> Cc: linux-kernel@vger.kernel.org Cc: containers@lists.linux-foundation.org Cc: Prasad <prasad@linux.vnet.ibm.com> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Ingo Molnar <mingo@elte.hu> Cc: Will Deacon <will.deacon@arm.com> Cc: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com> Signed-off-by: Matt Helsley <matthltc@us.ibm.com> --- kernel/hw_breakpoint.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c index 3b2aaff..6122f02 100644 --- a/kernel/hw_breakpoint.c +++ b/kernel/hw_breakpoint.c @@ -433,7 +433,8 @@ register_user_hw_breakpoint(struct perf_event_attr *attr, perf_overflow_handler_t triggered, struct task_struct *tsk) { - return perf_event_create_kernel_counter(attr, -1, tsk->pid, triggered); + return perf_event_create_kernel_counter(attr, -1, task_pid_vnr(tsk), + triggered); } EXPORT_SYMBOL_GPL(register_user_hw_breakpoint); -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/3] perf events: Split out task search into helper 2010-09-13 20:01 ` [PATCH 1/3] hw breakpoints: Fix pid namespace bug Matt Helsley @ 2010-09-13 20:01 ` Matt Helsley 2010-09-13 20:01 ` [PATCH 3/3] perf events: Cleanup pid passing Matt Helsley 2010-09-15 10:03 ` [tip:perf/core] perf events: Split out task search into helper tip-bot for Matt Helsley [not found] ` <f63454af09fb1915717251570423eb9ddd338340.1284407762.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> ` (2 subsequent siblings) 3 siblings, 2 replies; 18+ messages in thread From: Matt Helsley @ 2010-09-13 20:01 UTC (permalink / raw) To: Frederic Weisbecker Cc: containers, Matt Helsley, Robin Green, linux-kernel, Prasad, Peter Zijlstra, Arnaldo Carvalho de Melo, Steven Rostedt, Ingo Molnar, Will Deacon, Mahesh Salgaonkar Split out the code which searches for non-exiting tasks into its own helper. Creating this helper not only makes the code slightly more readable it prepares to move the search out of find_get_context() in a subsequent commit. Cc: Robin Green <greenrd@greenrd.org> Cc: linux-kernel@vger.kernel.org Cc: containers@lists.linux-foundation.org Cc: Prasad <prasad@linux.vnet.ibm.com> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Ingo Molnar <mingo@elte.hu> Cc: Will Deacon <will.deacon@arm.com> Cc: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com> Signed-off-by: Matt Helsley <matthltc@us.ibm.com> --- kernel/perf_event.c | 63 ++++++++++++++++++++++++++++++++------------------ 1 files changed, 40 insertions(+), 23 deletions(-) diff --git a/kernel/perf_event.c b/kernel/perf_event.c index 440f9ca..3f5309d 100644 --- a/kernel/perf_event.c +++ b/kernel/perf_event.c @@ -2015,6 +2015,43 @@ alloc_perf_context(struct pmu *pmu, struct task_struct *task) return ctx; } +static struct task_struct * +find_lively_task_by_vpid(pid_t vpid) +{ + struct task_struct *task; + int err; + + rcu_read_lock(); + if (!vpid) + task = current; + else + task = find_task_by_vpid(vpid); + if (task) + get_task_struct(task); + rcu_read_unlock(); + + if (!task) + return ERR_PTR(-ESRCH); + + /* + * Can't attach events to a dying task. + */ + err = -ESRCH; + if (task->flags & PF_EXITING) + goto errout; + + /* Reuse ptrace permission checks for now. */ + err = -EACCES; + if (!ptrace_may_access(task, PTRACE_MODE_READ)) + goto errout; + + return task; +errout: + put_task_struct(task); + return ERR_PTR(err); + +} + static struct perf_event_context * find_get_context(struct pmu *pmu, pid_t pid, int cpu) { @@ -2047,29 +2084,9 @@ find_get_context(struct pmu *pmu, pid_t pid, int cpu) return ctx; } - rcu_read_lock(); - if (!pid) - task = current; - else - task = find_task_by_vpid(pid); - if (task) - get_task_struct(task); - rcu_read_unlock(); - - if (!task) - return ERR_PTR(-ESRCH); - - /* - * Can't attach events to a dying task. - */ - err = -ESRCH; - if (task->flags & PF_EXITING) - goto errout; - - /* Reuse ptrace permission checks for now. */ - err = -EACCES; - if (!ptrace_may_access(task, PTRACE_MODE_READ)) - goto errout; + task = find_lively_task_by_vpid(pid); + if (IS_ERR(task)) + return (void*)task; err = -EINVAL; ctxn = pmu->task_ctx_nr; -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/3] perf events: Cleanup pid passing 2010-09-13 20:01 ` [PATCH 2/3] perf events: Split out task search into helper Matt Helsley @ 2010-09-13 20:01 ` Matt Helsley 2010-09-15 8:18 ` Peter Zijlstra ` (2 more replies) 2010-09-15 10:03 ` [tip:perf/core] perf events: Split out task search into helper tip-bot for Matt Helsley 1 sibling, 3 replies; 18+ messages in thread From: Matt Helsley @ 2010-09-13 20:01 UTC (permalink / raw) To: Frederic Weisbecker Cc: containers, Matt Helsley, Robin Green, linux-kernel, Prasad, Peter Zijlstra, Arnaldo Carvalho de Melo, Steven Rostedt, Ingo Molnar, Will Deacon, Mahesh Salgaonkar The kernel perf event creation path shouldn't use find_task_by_vpid() because a vpid exists in a specific namespace. find_task_by_vpid() uses current's pid namespace which isn't always the correct namespace to use for the vpid in all the places perf_event_create_kernel_counter() (and thus find_get_context()) is called. The goal is to clean up pid namespace handling and prevent bugs like: https://bugzilla.kernel.org/show_bug.cgi?id=17281 Instead of using pids switch find_get_context() to use task struct pointers directly. The syscall is responsible for resolving the pid to a task struct. This moves the pid namespace resolution into the syscall much like every other syscall that takes pid parameters. Cc: Robin Green <greenrd@greenrd.org> Cc: linux-kernel@vger.kernel.org Cc: Prasad <prasad@linux.vnet.ibm.com> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Ingo Molnar <mingo@elte.hu> Cc: Will Deacon <will.deacon@arm.com> Cc: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com> Signed-off-by: Matt Helsley <matthltc@us.ibm.com> --- arch/arm/oprofile/common.c | 2 +- include/linux/perf_event.h | 2 +- kernel/hw_breakpoint.c | 5 ++--- kernel/perf_event.c | 23 ++++++++++++----------- kernel/watchdog.c | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c index 0691176..aad63e6 100644 --- a/arch/arm/oprofile/common.c +++ b/arch/arm/oprofile/common.c @@ -96,7 +96,7 @@ static int op_create_counter(int cpu, int event) return ret; pevent = perf_event_create_kernel_counter(&counter_config[event].attr, - cpu, -1, + cpu, NULL, op_overflow_handler); if (IS_ERR(pevent)) { diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 93bf53a..39d8860 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -902,7 +902,7 @@ extern int perf_event_release_kernel(struct perf_event *event); extern struct perf_event * perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu, - pid_t pid, + struct task_struct *task, perf_overflow_handler_t callback); extern u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running); diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c index 6122f02..3b714e8 100644 --- a/kernel/hw_breakpoint.c +++ b/kernel/hw_breakpoint.c @@ -433,8 +433,7 @@ register_user_hw_breakpoint(struct perf_event_attr *attr, perf_overflow_handler_t triggered, struct task_struct *tsk) { - return perf_event_create_kernel_counter(attr, -1, task_pid_vnr(tsk), - triggered); + return perf_event_create_kernel_counter(attr, -1, tsk, triggered); } EXPORT_SYMBOL_GPL(register_user_hw_breakpoint); @@ -516,7 +515,7 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr, get_online_cpus(); for_each_online_cpu(cpu) { pevent = per_cpu_ptr(cpu_events, cpu); - bp = perf_event_create_kernel_counter(attr, cpu, -1, triggered); + bp = perf_event_create_kernel_counter(attr, cpu, NULL, triggered); *pevent = bp; diff --git a/kernel/perf_event.c b/kernel/perf_event.c index 3f5309d..8477479 100644 --- a/kernel/perf_event.c +++ b/kernel/perf_event.c @@ -2053,15 +2053,14 @@ errout: } static struct perf_event_context * -find_get_context(struct pmu *pmu, pid_t pid, int cpu) +find_get_context(struct pmu *pmu, struct task_struct *task, int cpu) { struct perf_event_context *ctx; struct perf_cpu_context *cpuctx; - struct task_struct *task; unsigned long flags; int ctxn, err; - if (pid == -1 && cpu != -1) { + if (!task && cpu != -1) { /* Must be root to operate on a CPU event: */ if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN)) return ERR_PTR(-EACCES); @@ -2084,10 +2083,6 @@ find_get_context(struct pmu *pmu, pid_t pid, int cpu) return ctx; } - task = find_lively_task_by_vpid(pid); - if (IS_ERR(task)) - return (void*)task; - err = -EINVAL; ctxn = pmu->task_ctx_nr; if (ctxn < 0) @@ -5527,6 +5522,7 @@ SYSCALL_DEFINE5(perf_event_open, struct perf_event_context *ctx; struct file *event_file = NULL; struct file *group_file = NULL; + struct task_struct *task; struct pmu *pmu; int event_fd; int fput_needed = 0; @@ -5584,7 +5580,12 @@ SYSCALL_DEFINE5(perf_event_open, /* * Get the target context (task or percpu): */ - ctx = find_get_context(pmu, pid, cpu); + if (pid == -1 && cpu != -1) + task = NULL; + else + task = find_lively_task_by_vpid(pid); + + ctx = find_get_context(pmu, task, cpu); if (IS_ERR(ctx)) { err = PTR_ERR(ctx); goto err_group_fd; @@ -5666,11 +5667,11 @@ err_fd: * * @attr: attributes of the counter to create * @cpu: cpu in which the counter is bound - * @pid: task to profile + * @task: task to profile (NULL for percpu) */ struct perf_event * perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu, - pid_t pid, + struct task_struct *task, perf_overflow_handler_t overflow_handler) { struct perf_event_context *ctx; @@ -5687,7 +5688,7 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu, goto err; } - ctx = find_get_context(event->pmu, pid, cpu); + ctx = find_get_context(event->pmu, task, cpu); if (IS_ERR(ctx)) { err = PTR_ERR(ctx); goto err_free; diff --git a/kernel/watchdog.c b/kernel/watchdog.c index fa71aeb..8ba21ca 100644 --- a/kernel/watchdog.c +++ b/kernel/watchdog.c @@ -358,7 +358,7 @@ static int watchdog_nmi_enable(int cpu) /* Try to register using hardware perf events */ wd_attr = &wd_hw_attr; wd_attr->sample_period = hw_nmi_get_sample_period(); - event = perf_event_create_kernel_counter(wd_attr, cpu, -1, watchdog_overflow_callback); + event = perf_event_create_kernel_counter(wd_attr, cpu, NULL, watchdog_overflow_callback); if (!IS_ERR(event)) { printk(KERN_INFO "NMI watchdog enabled, takes one hw-pmu counter.\n"); goto out_save; -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] perf events: Cleanup pid passing 2010-09-13 20:01 ` [PATCH 3/3] perf events: Cleanup pid passing Matt Helsley @ 2010-09-15 8:18 ` Peter Zijlstra [not found] ` <a134e5e392ab0204961fd1a62c84a222bf5874a9.1284407763.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> 2010-09-15 10:03 ` [tip:perf/core] perf events: Clean up " tip-bot for Matt Helsley 2 siblings, 0 replies; 18+ messages in thread From: Peter Zijlstra @ 2010-09-15 8:18 UTC (permalink / raw) To: Matt Helsley Cc: Frederic Weisbecker, containers, Robin Green, linux-kernel, Prasad, Arnaldo Carvalho de Melo, Steven Rostedt, Ingo Molnar, Will Deacon, Mahesh Salgaonkar On Mon, 2010-09-13 at 13:01 -0700, Matt Helsley wrote: > The kernel perf event creation path shouldn't use find_task_by_vpid() > because a vpid exists in a specific namespace. find_task_by_vpid() uses > current's pid namespace which isn't always the correct namespace to use > for the vpid in all the places perf_event_create_kernel_counter() (and > thus find_get_context()) is called. > > The goal is to clean up pid namespace handling and prevent bugs like: > > https://bugzilla.kernel.org/show_bug.cgi?id=17281 > > Instead of using pids switch find_get_context() to use task struct > pointers directly. The syscall is responsible for resolving the pid to > a task struct. This moves the pid namespace resolution into the syscall > much like every other syscall that takes pid parameters. I took the three patches with the following change to this patch: --- Index: linux-2.6/kernel/perf_event.c =================================================================== --- linux-2.6.orig/kernel/perf_event.c +++ linux-2.6/kernel/perf_event.c @@ -5522,7 +5522,7 @@ SYSCALL_DEFINE5(perf_event_open, struct perf_event_context *ctx; struct file *event_file = NULL; struct file *group_file = NULL; - struct task_struct *task; + struct task_struct *task = NULL; struct pmu *pmu; int event_fd; int fput_needed = 0; @@ -5577,14 +5577,12 @@ SYSCALL_DEFINE5(perf_event_open, if ((pmu->task_ctx_nr == perf_sw_context) && group_leader) pmu = group_leader->pmu; + if (pid != -1) + task = find_lively_task_by_vpid(pid); + /* * Get the target context (task or percpu): */ - if (pid == -1 && cpu != -1) - task = NULL; - else - task = find_lively_task_by_vpid(pid); - ctx = find_get_context(pmu, task, cpu); if (IS_ERR(ctx)) { err = PTR_ERR(ctx); ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <a134e5e392ab0204961fd1a62c84a222bf5874a9.1284407763.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 3/3] perf events: Cleanup pid passing [not found] ` <a134e5e392ab0204961fd1a62c84a222bf5874a9.1284407763.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> @ 2010-09-15 8:18 ` Peter Zijlstra 0 siblings, 0 replies; 18+ messages in thread From: Peter Zijlstra @ 2010-09-15 8:18 UTC (permalink / raw) To: Matt Helsley Cc: Frederic Weisbecker, containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Will Deacon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steven Rostedt, Arnaldo Carvalho de Melo, Mahesh Salgaonkar, Ingo Molnar, Robin Green, Prasad On Mon, 2010-09-13 at 13:01 -0700, Matt Helsley wrote: > The kernel perf event creation path shouldn't use find_task_by_vpid() > because a vpid exists in a specific namespace. find_task_by_vpid() uses > current's pid namespace which isn't always the correct namespace to use > for the vpid in all the places perf_event_create_kernel_counter() (and > thus find_get_context()) is called. > > The goal is to clean up pid namespace handling and prevent bugs like: > > https://bugzilla.kernel.org/show_bug.cgi?id=17281 > > Instead of using pids switch find_get_context() to use task struct > pointers directly. The syscall is responsible for resolving the pid to > a task struct. This moves the pid namespace resolution into the syscall > much like every other syscall that takes pid parameters. I took the three patches with the following change to this patch: --- Index: linux-2.6/kernel/perf_event.c =================================================================== --- linux-2.6.orig/kernel/perf_event.c +++ linux-2.6/kernel/perf_event.c @@ -5522,7 +5522,7 @@ SYSCALL_DEFINE5(perf_event_open, struct perf_event_context *ctx; struct file *event_file = NULL; struct file *group_file = NULL; - struct task_struct *task; + struct task_struct *task = NULL; struct pmu *pmu; int event_fd; int fput_needed = 0; @@ -5577,14 +5577,12 @@ SYSCALL_DEFINE5(perf_event_open, if ((pmu->task_ctx_nr == perf_sw_context) && group_leader) pmu = group_leader->pmu; + if (pid != -1) + task = find_lively_task_by_vpid(pid); + /* * Get the target context (task or percpu): */ - if (pid == -1 && cpu != -1) - task = NULL; - else - task = find_lively_task_by_vpid(pid); - ctx = find_get_context(pmu, task, cpu); if (IS_ERR(ctx)) { err = PTR_ERR(ctx); ^ permalink raw reply [flat|nested] 18+ messages in thread
* [tip:perf/core] perf events: Clean up pid passing 2010-09-13 20:01 ` [PATCH 3/3] perf events: Cleanup pid passing Matt Helsley 2010-09-15 8:18 ` Peter Zijlstra [not found] ` <a134e5e392ab0204961fd1a62c84a222bf5874a9.1284407763.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> @ 2010-09-15 10:03 ` tip-bot for Matt Helsley 2010-09-22 12:22 ` Peter Zijlstra 2 siblings, 1 reply; 18+ messages in thread From: tip-bot for Matt Helsley @ 2010-09-15 10:03 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, acme, hpa, mingo, will.deacon, a.p.zijlstra, matthltc, mahesh, rostedt, tglx, mingo, greenrd, prasad Commit-ID: 38a81da2205f94e8a2a834b51a6b99c91fc7c2e8 Gitweb: http://git.kernel.org/tip/38a81da2205f94e8a2a834b51a6b99c91fc7c2e8 Author: Matt Helsley <matthltc@us.ibm.com> AuthorDate: Mon, 13 Sep 2010 13:01:20 -0700 Committer: Ingo Molnar <mingo@elte.hu> CommitDate: Wed, 15 Sep 2010 10:44:00 +0200 perf events: Clean up pid passing The kernel perf event creation path shouldn't use find_task_by_vpid() because a vpid exists in a specific namespace. find_task_by_vpid() uses current's pid namespace which isn't always the correct namespace to use for the vpid in all the places perf_event_create_kernel_counter() (and thus find_get_context()) is called. The goal is to clean up pid namespace handling and prevent bugs like: https://bugzilla.kernel.org/show_bug.cgi?id=17281 Instead of using pids switch find_get_context() to use task struct pointers directly. The syscall is responsible for resolving the pid to a task struct. This moves the pid namespace resolution into the syscall much like every other syscall that takes pid parameters. Signed-off-by: Matt Helsley <matthltc@us.ibm.com> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Robin Green <greenrd@greenrd.org> Cc: Prasad <prasad@linux.vnet.ibm.com> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Will Deacon <will.deacon@arm.com> Cc: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com> LKML-Reference: <a134e5e392ab0204961fd1a62c84a222bf5874a9.1284407763.git.matthltc@us.ibm.com> Signed-off-by: Ingo Molnar <mingo@elte.hu> --- arch/arm/oprofile/common.c | 2 +- include/linux/perf_event.h | 2 +- kernel/hw_breakpoint.c | 5 ++--- kernel/perf_event.c | 21 ++++++++++----------- kernel/watchdog.c | 2 +- 5 files changed, 15 insertions(+), 17 deletions(-) diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c index 0691176..aad63e6 100644 --- a/arch/arm/oprofile/common.c +++ b/arch/arm/oprofile/common.c @@ -96,7 +96,7 @@ static int op_create_counter(int cpu, int event) return ret; pevent = perf_event_create_kernel_counter(&counter_config[event].attr, - cpu, -1, + cpu, NULL, op_overflow_handler); if (IS_ERR(pevent)) { diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 93bf53a..39d8860 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -902,7 +902,7 @@ extern int perf_event_release_kernel(struct perf_event *event); extern struct perf_event * perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu, - pid_t pid, + struct task_struct *task, perf_overflow_handler_t callback); extern u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running); diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c index 6122f02..3b714e8 100644 --- a/kernel/hw_breakpoint.c +++ b/kernel/hw_breakpoint.c @@ -433,8 +433,7 @@ register_user_hw_breakpoint(struct perf_event_attr *attr, perf_overflow_handler_t triggered, struct task_struct *tsk) { - return perf_event_create_kernel_counter(attr, -1, task_pid_vnr(tsk), - triggered); + return perf_event_create_kernel_counter(attr, -1, tsk, triggered); } EXPORT_SYMBOL_GPL(register_user_hw_breakpoint); @@ -516,7 +515,7 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr, get_online_cpus(); for_each_online_cpu(cpu) { pevent = per_cpu_ptr(cpu_events, cpu); - bp = perf_event_create_kernel_counter(attr, cpu, -1, triggered); + bp = perf_event_create_kernel_counter(attr, cpu, NULL, triggered); *pevent = bp; diff --git a/kernel/perf_event.c b/kernel/perf_event.c index 3f5309d..86f394e 100644 --- a/kernel/perf_event.c +++ b/kernel/perf_event.c @@ -2053,15 +2053,14 @@ errout: } static struct perf_event_context * -find_get_context(struct pmu *pmu, pid_t pid, int cpu) +find_get_context(struct pmu *pmu, struct task_struct *task, int cpu) { struct perf_event_context *ctx; struct perf_cpu_context *cpuctx; - struct task_struct *task; unsigned long flags; int ctxn, err; - if (pid == -1 && cpu != -1) { + if (!task && cpu != -1) { /* Must be root to operate on a CPU event: */ if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN)) return ERR_PTR(-EACCES); @@ -2084,10 +2083,6 @@ find_get_context(struct pmu *pmu, pid_t pid, int cpu) return ctx; } - task = find_lively_task_by_vpid(pid); - if (IS_ERR(task)) - return (void*)task; - err = -EINVAL; ctxn = pmu->task_ctx_nr; if (ctxn < 0) @@ -5527,6 +5522,7 @@ SYSCALL_DEFINE5(perf_event_open, struct perf_event_context *ctx; struct file *event_file = NULL; struct file *group_file = NULL; + struct task_struct *task = NULL; struct pmu *pmu; int event_fd; int fput_needed = 0; @@ -5581,10 +5577,13 @@ SYSCALL_DEFINE5(perf_event_open, if ((pmu->task_ctx_nr == perf_sw_context) && group_leader) pmu = group_leader->pmu; + if (pid != -1) + task = find_lively_task_by_vpid(pid); + /* * Get the target context (task or percpu): */ - ctx = find_get_context(pmu, pid, cpu); + ctx = find_get_context(pmu, task, cpu); if (IS_ERR(ctx)) { err = PTR_ERR(ctx); goto err_group_fd; @@ -5666,11 +5665,11 @@ err_fd: * * @attr: attributes of the counter to create * @cpu: cpu in which the counter is bound - * @pid: task to profile + * @task: task to profile (NULL for percpu) */ struct perf_event * perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu, - pid_t pid, + struct task_struct *task, perf_overflow_handler_t overflow_handler) { struct perf_event_context *ctx; @@ -5687,7 +5686,7 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu, goto err; } - ctx = find_get_context(event->pmu, pid, cpu); + ctx = find_get_context(event->pmu, task, cpu); if (IS_ERR(ctx)) { err = PTR_ERR(ctx); goto err_free; diff --git a/kernel/watchdog.c b/kernel/watchdog.c index 89eadbb..dc8e168 100644 --- a/kernel/watchdog.c +++ b/kernel/watchdog.c @@ -358,7 +358,7 @@ static int watchdog_nmi_enable(int cpu) /* Try to register using hardware perf events */ wd_attr = &wd_hw_attr; wd_attr->sample_period = hw_nmi_get_sample_period(); - event = perf_event_create_kernel_counter(wd_attr, cpu, -1, watchdog_overflow_callback); + event = perf_event_create_kernel_counter(wd_attr, cpu, NULL, watchdog_overflow_callback); if (!IS_ERR(event)) { printk(KERN_INFO "NMI watchdog enabled, takes one hw-pmu counter.\n"); goto out_save; ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [tip:perf/core] perf events: Clean up pid passing 2010-09-15 10:03 ` [tip:perf/core] perf events: Clean up " tip-bot for Matt Helsley @ 2010-09-22 12:22 ` Peter Zijlstra 0 siblings, 0 replies; 18+ messages in thread From: Peter Zijlstra @ 2010-09-22 12:22 UTC (permalink / raw) To: mingo, hpa, acme, linux-kernel, will.deacon, matthltc, mahesh, rostedt, tglx, prasad, greenrd, mingo Cc: linux-tip-commits On Wed, 2010-09-15 at 10:03 +0000, tip-bot for Matt Helsley wrote: > +++ b/kernel/hw_breakpoint.c > @@ -433,8 +433,7 @@ register_user_hw_breakpoint(struct perf_event_attr *attr, > perf_overflow_handler_t triggered, > struct task_struct *tsk) > { > - return perf_event_create_kernel_counter(attr, -1, task_pid_vnr(tsk), > - triggered); > + return perf_event_create_kernel_counter(attr, -1, tsk, triggered); > } > EXPORT_SYMBOL_GPL(register_user_hw_breakpoint); > That seems to have gotten lost somehow.. /usr/src/linux-2.6/kernel/hw_breakpoint.c: In function ‘register_user_hw_breakpoint’: /usr/src/linux-2.6/kernel/hw_breakpoint.c:436: warning: passing argument 3 of ‘perf_event_create_kernel_counter’ makes pointer from integer without a cast /usr/src/linux-2.6/include/linux/perf_event.h:909: note: expected ‘struct task_struct *’ but argument is of type ‘pid_t’ The below cures it --- diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c index 8f36c99..3b714e8 100644 --- a/kernel/hw_breakpoint.c +++ b/kernel/hw_breakpoint.c @@ -433,7 +433,7 @@ register_user_hw_breakpoint(struct perf_event_attr *attr, perf_overflow_handler_t triggered, struct task_struct *tsk) { - return perf_event_create_kernel_counter(attr, -1, task_pid_vnr(tsk), triggered); + return perf_event_create_kernel_counter(attr, -1, tsk, triggered); } EXPORT_SYMBOL_GPL(register_user_hw_breakpoint); ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [tip:perf/core] perf events: Split out task search into helper 2010-09-13 20:01 ` [PATCH 2/3] perf events: Split out task search into helper Matt Helsley 2010-09-13 20:01 ` [PATCH 3/3] perf events: Cleanup pid passing Matt Helsley @ 2010-09-15 10:03 ` tip-bot for Matt Helsley 1 sibling, 0 replies; 18+ messages in thread From: tip-bot for Matt Helsley @ 2010-09-15 10:03 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, acme, hpa, mingo, will.deacon, a.p.zijlstra, matthltc, mahesh, rostedt, tglx, mingo, greenrd, prasad Commit-ID: 2ebd4ffb6d0cb877787b1e42be8485820158857e Gitweb: http://git.kernel.org/tip/2ebd4ffb6d0cb877787b1e42be8485820158857e Author: Matt Helsley <matthltc@us.ibm.com> AuthorDate: Mon, 13 Sep 2010 13:01:19 -0700 Committer: Ingo Molnar <mingo@elte.hu> CommitDate: Wed, 15 Sep 2010 10:44:00 +0200 perf events: Split out task search into helper Split out the code which searches for non-exiting tasks into its own helper. Creating this helper not only makes the code slightly more readable it prepares to move the search out of find_get_context() in a subsequent commit. Signed-off-by: Matt Helsley <matthltc@us.ibm.com> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Robin Green <greenrd@greenrd.org> Cc: Prasad <prasad@linux.vnet.ibm.com> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Will Deacon <will.deacon@arm.com> Cc: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com> LKML-Reference: <561205417b450b8a4bf7488374541d64b4690431.1284407762.git.matthltc@us.ibm.com> Signed-off-by: Ingo Molnar <mingo@elte.hu> --- kernel/perf_event.c | 63 ++++++++++++++++++++++++++++++++------------------ 1 files changed, 40 insertions(+), 23 deletions(-) diff --git a/kernel/perf_event.c b/kernel/perf_event.c index 440f9ca..3f5309d 100644 --- a/kernel/perf_event.c +++ b/kernel/perf_event.c @@ -2015,6 +2015,43 @@ alloc_perf_context(struct pmu *pmu, struct task_struct *task) return ctx; } +static struct task_struct * +find_lively_task_by_vpid(pid_t vpid) +{ + struct task_struct *task; + int err; + + rcu_read_lock(); + if (!vpid) + task = current; + else + task = find_task_by_vpid(vpid); + if (task) + get_task_struct(task); + rcu_read_unlock(); + + if (!task) + return ERR_PTR(-ESRCH); + + /* + * Can't attach events to a dying task. + */ + err = -ESRCH; + if (task->flags & PF_EXITING) + goto errout; + + /* Reuse ptrace permission checks for now. */ + err = -EACCES; + if (!ptrace_may_access(task, PTRACE_MODE_READ)) + goto errout; + + return task; +errout: + put_task_struct(task); + return ERR_PTR(err); + +} + static struct perf_event_context * find_get_context(struct pmu *pmu, pid_t pid, int cpu) { @@ -2047,29 +2084,9 @@ find_get_context(struct pmu *pmu, pid_t pid, int cpu) return ctx; } - rcu_read_lock(); - if (!pid) - task = current; - else - task = find_task_by_vpid(pid); - if (task) - get_task_struct(task); - rcu_read_unlock(); - - if (!task) - return ERR_PTR(-ESRCH); - - /* - * Can't attach events to a dying task. - */ - err = -ESRCH; - if (task->flags & PF_EXITING) - goto errout; - - /* Reuse ptrace permission checks for now. */ - err = -EACCES; - if (!ptrace_may_access(task, PTRACE_MODE_READ)) - goto errout; + task = find_lively_task_by_vpid(pid); + if (IS_ERR(task)) + return (void*)task; err = -EINVAL; ctxn = pmu->task_ctx_nr; ^ permalink raw reply related [flat|nested] 18+ messages in thread
[parent not found: <f63454af09fb1915717251570423eb9ddd338340.1284407762.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]
* [PATCH 2/3] perf events: Split out task search into helper [not found] ` <f63454af09fb1915717251570423eb9ddd338340.1284407762.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> @ 2010-09-13 20:01 ` Matt Helsley [not found] ` <561205417b450b8a4bf7488374541d64b4690431.1284407762.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> 1 sibling, 0 replies; 18+ messages in thread From: Matt Helsley @ 2010-09-13 20:01 UTC (permalink / raw) To: Frederic Weisbecker Cc: Peter Zijlstra, Mahesh Salgaonkar, containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Will Deacon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steven Rostedt, Arnaldo Carvalho de Melo, Ingo Molnar, Robin Green, Prasad Split out the code which searches for non-exiting tasks into its own helper. Creating this helper not only makes the code slightly more readable it prepares to move the search out of find_get_context() in a subsequent commit. Cc: Robin Green <greenrd-tZez7oB1z75AfugRpC6u6w@public.gmane.org> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Cc: Prasad <prasad-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> Cc: Peter Zijlstra <a.p.zijlstra-/NLkJaSkS4VmR6Xm/wNWPw@public.gmane.org> Cc: Arnaldo Carvalho de Melo <acme-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Cc: Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org> Cc: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> Cc: Mahesh Salgaonkar <mahesh-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> --- kernel/perf_event.c | 63 ++++++++++++++++++++++++++++++++------------------ 1 files changed, 40 insertions(+), 23 deletions(-) diff --git a/kernel/perf_event.c b/kernel/perf_event.c index 440f9ca..3f5309d 100644 --- a/kernel/perf_event.c +++ b/kernel/perf_event.c @@ -2015,6 +2015,43 @@ alloc_perf_context(struct pmu *pmu, struct task_struct *task) return ctx; } +static struct task_struct * +find_lively_task_by_vpid(pid_t vpid) +{ + struct task_struct *task; + int err; + + rcu_read_lock(); + if (!vpid) + task = current; + else + task = find_task_by_vpid(vpid); + if (task) + get_task_struct(task); + rcu_read_unlock(); + + if (!task) + return ERR_PTR(-ESRCH); + + /* + * Can't attach events to a dying task. + */ + err = -ESRCH; + if (task->flags & PF_EXITING) + goto errout; + + /* Reuse ptrace permission checks for now. */ + err = -EACCES; + if (!ptrace_may_access(task, PTRACE_MODE_READ)) + goto errout; + + return task; +errout: + put_task_struct(task); + return ERR_PTR(err); + +} + static struct perf_event_context * find_get_context(struct pmu *pmu, pid_t pid, int cpu) { @@ -2047,29 +2084,9 @@ find_get_context(struct pmu *pmu, pid_t pid, int cpu) return ctx; } - rcu_read_lock(); - if (!pid) - task = current; - else - task = find_task_by_vpid(pid); - if (task) - get_task_struct(task); - rcu_read_unlock(); - - if (!task) - return ERR_PTR(-ESRCH); - - /* - * Can't attach events to a dying task. - */ - err = -ESRCH; - if (task->flags & PF_EXITING) - goto errout; - - /* Reuse ptrace permission checks for now. */ - err = -EACCES; - if (!ptrace_may_access(task, PTRACE_MODE_READ)) - goto errout; + task = find_lively_task_by_vpid(pid); + if (IS_ERR(task)) + return (void*)task; err = -EINVAL; ctxn = pmu->task_ctx_nr; -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
[parent not found: <561205417b450b8a4bf7488374541d64b4690431.1284407762.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]
* [PATCH 3/3] perf events: Cleanup pid passing [not found] ` <561205417b450b8a4bf7488374541d64b4690431.1284407762.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> @ 2010-09-13 20:01 ` Matt Helsley 0 siblings, 0 replies; 18+ messages in thread From: Matt Helsley @ 2010-09-13 20:01 UTC (permalink / raw) To: Frederic Weisbecker Cc: Peter Zijlstra, Mahesh Salgaonkar, containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Will Deacon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steven Rostedt, Arnaldo Carvalho de Melo, Ingo Molnar, Robin Green, Prasad The kernel perf event creation path shouldn't use find_task_by_vpid() because a vpid exists in a specific namespace. find_task_by_vpid() uses current's pid namespace which isn't always the correct namespace to use for the vpid in all the places perf_event_create_kernel_counter() (and thus find_get_context()) is called. The goal is to clean up pid namespace handling and prevent bugs like: https://bugzilla.kernel.org/show_bug.cgi?id=17281 Instead of using pids switch find_get_context() to use task struct pointers directly. The syscall is responsible for resolving the pid to a task struct. This moves the pid namespace resolution into the syscall much like every other syscall that takes pid parameters. Cc: Robin Green <greenrd-tZez7oB1z75AfugRpC6u6w@public.gmane.org> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Cc: Prasad <prasad-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> Cc: Peter Zijlstra <a.p.zijlstra-/NLkJaSkS4VmR6Xm/wNWPw@public.gmane.org> Cc: Arnaldo Carvalho de Melo <acme-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Cc: Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org> Cc: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> Cc: Mahesh Salgaonkar <mahesh-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> --- arch/arm/oprofile/common.c | 2 +- include/linux/perf_event.h | 2 +- kernel/hw_breakpoint.c | 5 ++--- kernel/perf_event.c | 23 ++++++++++++----------- kernel/watchdog.c | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c index 0691176..aad63e6 100644 --- a/arch/arm/oprofile/common.c +++ b/arch/arm/oprofile/common.c @@ -96,7 +96,7 @@ static int op_create_counter(int cpu, int event) return ret; pevent = perf_event_create_kernel_counter(&counter_config[event].attr, - cpu, -1, + cpu, NULL, op_overflow_handler); if (IS_ERR(pevent)) { diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 93bf53a..39d8860 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -902,7 +902,7 @@ extern int perf_event_release_kernel(struct perf_event *event); extern struct perf_event * perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu, - pid_t pid, + struct task_struct *task, perf_overflow_handler_t callback); extern u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running); diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c index 6122f02..3b714e8 100644 --- a/kernel/hw_breakpoint.c +++ b/kernel/hw_breakpoint.c @@ -433,8 +433,7 @@ register_user_hw_breakpoint(struct perf_event_attr *attr, perf_overflow_handler_t triggered, struct task_struct *tsk) { - return perf_event_create_kernel_counter(attr, -1, task_pid_vnr(tsk), - triggered); + return perf_event_create_kernel_counter(attr, -1, tsk, triggered); } EXPORT_SYMBOL_GPL(register_user_hw_breakpoint); @@ -516,7 +515,7 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr, get_online_cpus(); for_each_online_cpu(cpu) { pevent = per_cpu_ptr(cpu_events, cpu); - bp = perf_event_create_kernel_counter(attr, cpu, -1, triggered); + bp = perf_event_create_kernel_counter(attr, cpu, NULL, triggered); *pevent = bp; diff --git a/kernel/perf_event.c b/kernel/perf_event.c index 3f5309d..8477479 100644 --- a/kernel/perf_event.c +++ b/kernel/perf_event.c @@ -2053,15 +2053,14 @@ errout: } static struct perf_event_context * -find_get_context(struct pmu *pmu, pid_t pid, int cpu) +find_get_context(struct pmu *pmu, struct task_struct *task, int cpu) { struct perf_event_context *ctx; struct perf_cpu_context *cpuctx; - struct task_struct *task; unsigned long flags; int ctxn, err; - if (pid == -1 && cpu != -1) { + if (!task && cpu != -1) { /* Must be root to operate on a CPU event: */ if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN)) return ERR_PTR(-EACCES); @@ -2084,10 +2083,6 @@ find_get_context(struct pmu *pmu, pid_t pid, int cpu) return ctx; } - task = find_lively_task_by_vpid(pid); - if (IS_ERR(task)) - return (void*)task; - err = -EINVAL; ctxn = pmu->task_ctx_nr; if (ctxn < 0) @@ -5527,6 +5522,7 @@ SYSCALL_DEFINE5(perf_event_open, struct perf_event_context *ctx; struct file *event_file = NULL; struct file *group_file = NULL; + struct task_struct *task; struct pmu *pmu; int event_fd; int fput_needed = 0; @@ -5584,7 +5580,12 @@ SYSCALL_DEFINE5(perf_event_open, /* * Get the target context (task or percpu): */ - ctx = find_get_context(pmu, pid, cpu); + if (pid == -1 && cpu != -1) + task = NULL; + else + task = find_lively_task_by_vpid(pid); + + ctx = find_get_context(pmu, task, cpu); if (IS_ERR(ctx)) { err = PTR_ERR(ctx); goto err_group_fd; @@ -5666,11 +5667,11 @@ err_fd: * * @attr: attributes of the counter to create * @cpu: cpu in which the counter is bound - * @pid: task to profile + * @task: task to profile (NULL for percpu) */ struct perf_event * perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu, - pid_t pid, + struct task_struct *task, perf_overflow_handler_t overflow_handler) { struct perf_event_context *ctx; @@ -5687,7 +5688,7 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu, goto err; } - ctx = find_get_context(event->pmu, pid, cpu); + ctx = find_get_context(event->pmu, task, cpu); if (IS_ERR(ctx)) { err = PTR_ERR(ctx); goto err_free; diff --git a/kernel/watchdog.c b/kernel/watchdog.c index fa71aeb..8ba21ca 100644 --- a/kernel/watchdog.c +++ b/kernel/watchdog.c @@ -358,7 +358,7 @@ static int watchdog_nmi_enable(int cpu) /* Try to register using hardware perf events */ wd_attr = &wd_hw_attr; wd_attr->sample_period = hw_nmi_get_sample_period(); - event = perf_event_create_kernel_counter(wd_attr, cpu, -1, watchdog_overflow_callback); + event = perf_event_create_kernel_counter(wd_attr, cpu, NULL, watchdog_overflow_callback); if (!IS_ERR(event)) { printk(KERN_INFO "NMI watchdog enabled, takes one hw-pmu counter.\n"); goto out_save; -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [tip:perf/core] hw breakpoints: Fix pid namespace bug 2010-09-13 20:01 ` [PATCH 1/3] hw breakpoints: Fix pid namespace bug Matt Helsley 2010-09-13 20:01 ` [PATCH 2/3] perf events: Split out task search into helper Matt Helsley [not found] ` <f63454af09fb1915717251570423eb9ddd338340.1284407762.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> @ 2010-09-15 10:03 ` tip-bot for Matt Helsley 2010-09-15 11:41 ` Frederic Weisbecker 2010-09-17 8:28 ` [tip:perf/urgent] " tip-bot for Matt Helsley 3 siblings, 1 reply; 18+ messages in thread From: tip-bot for Matt Helsley @ 2010-09-15 10:03 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, acme, hpa, mingo, will.deacon, a.p.zijlstra, matthltc, mahesh, rostedt, tglx, mingo, greenrd, prasad Commit-ID: d958077d007d98125766d11e82da2fd6497b91d6 Gitweb: http://git.kernel.org/tip/d958077d007d98125766d11e82da2fd6497b91d6 Author: Matt Helsley <matthltc@us.ibm.com> AuthorDate: Mon, 13 Sep 2010 13:01:18 -0700 Committer: Ingo Molnar <mingo@elte.hu> CommitDate: Wed, 15 Sep 2010 10:43:59 +0200 hw breakpoints: Fix pid namespace bug Hardware breakpoints can't be registered within pid namespaces because tsk->pid is passed rather than the pid in the current namespace. (See https://bugzilla.kernel.org/show_bug.cgi?id=17281 ) This is a quick fix demonstrating the problem but is not the best method of solving the problem since passing pids internally is not the best way to avoid pid namespace bugs. Subsequent patches will show a better solution. Much thanks to Frederic Weisbecker <fweisbec@gmail.com> for doing the bulk of the work finding this bug. Signed-off-by: Matt Helsley <matthltc@us.ibm.com> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Robin Green <greenrd@greenrd.org> Cc: Prasad <prasad@linux.vnet.ibm.com> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Will Deacon <will.deacon@arm.com> Cc: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com> LKML-Reference: <f63454af09fb1915717251570423eb9ddd338340.1284407762.git.matthltc@us.ibm.com> Signed-off-by: Ingo Molnar <mingo@elte.hu> --- kernel/hw_breakpoint.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c index 3b2aaff..6122f02 100644 --- a/kernel/hw_breakpoint.c +++ b/kernel/hw_breakpoint.c @@ -433,7 +433,8 @@ register_user_hw_breakpoint(struct perf_event_attr *attr, perf_overflow_handler_t triggered, struct task_struct *tsk) { - return perf_event_create_kernel_counter(attr, -1, tsk->pid, triggered); + return perf_event_create_kernel_counter(attr, -1, task_pid_vnr(tsk), + triggered); } EXPORT_SYMBOL_GPL(register_user_hw_breakpoint); ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [tip:perf/core] hw breakpoints: Fix pid namespace bug 2010-09-15 10:03 ` [tip:perf/core] hw breakpoints: Fix pid namespace bug tip-bot for Matt Helsley @ 2010-09-15 11:41 ` Frederic Weisbecker 2010-09-15 11:45 ` Ingo Molnar 0 siblings, 1 reply; 18+ messages in thread From: Frederic Weisbecker @ 2010-09-15 11:41 UTC (permalink / raw) To: mingo, hpa, acme, linux-kernel, a.p.zijlstra, will.deacon, matthltc, mahesh, rostedt, tglx, prasad, greenrd, mingo Cc: linux-tip-commits On Wed, Sep 15, 2010 at 10:03:06AM +0000, tip-bot for Matt Helsley wrote: > Commit-ID: d958077d007d98125766d11e82da2fd6497b91d6 > Gitweb: http://git.kernel.org/tip/d958077d007d98125766d11e82da2fd6497b91d6 > Author: Matt Helsley <matthltc@us.ibm.com> > AuthorDate: Mon, 13 Sep 2010 13:01:18 -0700 > Committer: Ingo Molnar <mingo@elte.hu> > CommitDate: Wed, 15 Sep 2010 10:43:59 +0200 > > hw breakpoints: Fix pid namespace bug > > Hardware breakpoints can't be registered within pid namespaces > because tsk->pid is passed rather than the pid in the current > namespace. > > (See https://bugzilla.kernel.org/show_bug.cgi?id=17281 ) > > This is a quick fix demonstrating the problem but is not the > best method of solving the problem since passing pids internally > is not the best way to avoid pid namespace bugs. Subsequent patches > will show a better solution. > > Much thanks to Frederic Weisbecker <fweisbec@gmail.com> for doing the > bulk of the work finding this bug. > > Signed-off-by: Matt Helsley <matthltc@us.ibm.com> > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> > Cc: Robin Green <greenrd@greenrd.org> > Cc: Prasad <prasad@linux.vnet.ibm.com> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com> > LKML-Reference: <f63454af09fb1915717251570423eb9ddd338340.1284407762.git.matthltc@us.ibm.com> > Signed-off-by: Ingo Molnar <mingo@elte.hu> > --- Aiie, you have been too fast too apply that on -tip, especially it should go to perf/urgent with some stable backport tags. And I need to test them as he said that was only compile tested. Lemme just check it really fixes the issue and I'll package his three patches in according pull requests. Thanks. > kernel/hw_breakpoint.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c > index 3b2aaff..6122f02 100644 > --- a/kernel/hw_breakpoint.c > +++ b/kernel/hw_breakpoint.c > @@ -433,7 +433,8 @@ register_user_hw_breakpoint(struct perf_event_attr *attr, > perf_overflow_handler_t triggered, > struct task_struct *tsk) > { > - return perf_event_create_kernel_counter(attr, -1, tsk->pid, triggered); > + return perf_event_create_kernel_counter(attr, -1, task_pid_vnr(tsk), > + triggered); > } > EXPORT_SYMBOL_GPL(register_user_hw_breakpoint); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-tip-commits" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [tip:perf/core] hw breakpoints: Fix pid namespace bug 2010-09-15 11:41 ` Frederic Weisbecker @ 2010-09-15 11:45 ` Ingo Molnar 0 siblings, 0 replies; 18+ messages in thread From: Ingo Molnar @ 2010-09-15 11:45 UTC (permalink / raw) To: Frederic Weisbecker Cc: mingo, hpa, acme, linux-kernel, a.p.zijlstra, will.deacon, matthltc, mahesh, rostedt, tglx, prasad, greenrd, linux-tip-commits * Frederic Weisbecker <fweisbec@gmail.com> wrote: > On Wed, Sep 15, 2010 at 10:03:06AM +0000, tip-bot for Matt Helsley wrote: > > Commit-ID: d958077d007d98125766d11e82da2fd6497b91d6 > > Gitweb: http://git.kernel.org/tip/d958077d007d98125766d11e82da2fd6497b91d6 > > Author: Matt Helsley <matthltc@us.ibm.com> > > AuthorDate: Mon, 13 Sep 2010 13:01:18 -0700 > > Committer: Ingo Molnar <mingo@elte.hu> > > CommitDate: Wed, 15 Sep 2010 10:43:59 +0200 > > > > hw breakpoints: Fix pid namespace bug > > > > Hardware breakpoints can't be registered within pid namespaces > > because tsk->pid is passed rather than the pid in the current > > namespace. > > > > (See https://bugzilla.kernel.org/show_bug.cgi?id=17281 ) > > > > This is a quick fix demonstrating the problem but is not the > > best method of solving the problem since passing pids internally > > is not the best way to avoid pid namespace bugs. Subsequent patches > > will show a better solution. > > > > Much thanks to Frederic Weisbecker <fweisbec@gmail.com> for doing the > > bulk of the work finding this bug. > > > > Signed-off-by: Matt Helsley <matthltc@us.ibm.com> > > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> > > Cc: Robin Green <greenrd@greenrd.org> > > Cc: Prasad <prasad@linux.vnet.ibm.com> > > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com> > > LKML-Reference: <f63454af09fb1915717251570423eb9ddd338340.1284407762.git.matthltc@us.ibm.com> > > Signed-off-by: Ingo Molnar <mingo@elte.hu> > > --- > > > Aiie, you have been too fast too apply that on -tip, [...] Came from Peter. > [...] especially it should go to perf/urgent with some stable backport > tags. And I need to test them as he said that was only compile tested. No problem, mind cherry-picking it into an urgent pull request? The overlap is not an issue. > Lemme just check it really fixes the issue and I'll package his three > patches in according pull requests. Ok! Thanks, Ingo ^ permalink raw reply [flat|nested] 18+ messages in thread
* [tip:perf/urgent] hw breakpoints: Fix pid namespace bug 2010-09-13 20:01 ` [PATCH 1/3] hw breakpoints: Fix pid namespace bug Matt Helsley ` (2 preceding siblings ...) 2010-09-15 10:03 ` [tip:perf/core] hw breakpoints: Fix pid namespace bug tip-bot for Matt Helsley @ 2010-09-17 8:28 ` tip-bot for Matt Helsley 3 siblings, 0 replies; 18+ messages in thread From: tip-bot for Matt Helsley @ 2010-09-17 8:28 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, acme, hpa, mingo, will.deacon, a.p.zijlstra, matthltc, mahesh, fweisbec, rostedt, stable, tglx, mingo, greenrd, prasad Commit-ID: 068e35eee9ef98eb4cab55181977e24995d273be Gitweb: http://git.kernel.org/tip/068e35eee9ef98eb4cab55181977e24995d273be Author: Matt Helsley <matthltc@us.ibm.com> AuthorDate: Mon, 13 Sep 2010 13:01:18 -0700 Committer: Frederic Weisbecker <fweisbec@gmail.com> CommitDate: Fri, 17 Sep 2010 04:42:59 +0200 hw breakpoints: Fix pid namespace bug Hardware breakpoints can't be registered within pid namespaces because tsk->pid is passed rather than the pid in the current namespace. (See https://bugzilla.kernel.org/show_bug.cgi?id=17281 ) This is a quick fix demonstrating the problem but is not the best method of solving the problem since passing pids internally is not the best way to avoid pid namespace bugs. Subsequent patches will show a better solution. Much thanks to Frederic Weisbecker <fweisbec@gmail.com> for doing the bulk of the work finding this bug. Reported-by: Robin Green <greenrd@greenrd.org> Signed-off-by: Matt Helsley <matthltc@us.ibm.com> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Prasad <prasad@linux.vnet.ibm.com> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Will Deacon <will.deacon@arm.com> Cc: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com> Cc: 2.6.33-2.6.35 <stable@kernel.org> LKML-Reference: <f63454af09fb1915717251570423eb9ddd338340.1284407762.git.matthltc@us.ibm.com> Signed-off-by: Ingo Molnar <mingo@elte.hu> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> --- kernel/hw_breakpoint.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c index d71a987..c7c2aed 100644 --- a/kernel/hw_breakpoint.c +++ b/kernel/hw_breakpoint.c @@ -433,7 +433,8 @@ register_user_hw_breakpoint(struct perf_event_attr *attr, perf_overflow_handler_t triggered, struct task_struct *tsk) { - return perf_event_create_kernel_counter(attr, -1, tsk->pid, triggered); + return perf_event_create_kernel_counter(attr, -1, task_pid_vnr(tsk), + triggered); } EXPORT_SYMBOL_GPL(register_user_hw_breakpoint); ^ permalink raw reply related [flat|nested] 18+ messages in thread
[parent not found: <1284408080-2135-1-git-send-email-matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]
* [PATCH 1/3] hw breakpoints: Fix pid namespace bug [not found] ` <1284408080-2135-1-git-send-email-matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> @ 2010-09-13 20:01 ` Matt Helsley 2010-09-13 20:08 ` [PATCH 0/3] perf_events and hw_breakpoints fix and cleanup Frederic Weisbecker 1 sibling, 0 replies; 18+ messages in thread From: Matt Helsley @ 2010-09-13 20:01 UTC (permalink / raw) To: Frederic Weisbecker Cc: Peter Zijlstra, Mahesh Salgaonkar, containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Will Deacon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steven Rostedt, Arnaldo Carvalho de Melo, Ingo Molnar, Robin Green, Prasad Hardware breakpoints can't be registered within pid namespaces because tsk->pid is passed rather than the pid in the current namespace. (See https://bugzilla.kernel.org/show_bug.cgi?id=17281 ) This is a quick fix demonstrating the problem but is not the best method of solving the problem since passing pids internally is not the best way to avoid pid namespace bugs. Subsequent patches will show a better solution. Much thanks to Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> for doing the bulk of the work finding this bug. Cc: Robin Green <greenrd-tZez7oB1z75AfugRpC6u6w@public.gmane.org> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Cc: Prasad <prasad-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> Cc: Peter Zijlstra <a.p.zijlstra-/NLkJaSkS4VmR6Xm/wNWPw@public.gmane.org> Cc: Arnaldo Carvalho de Melo <acme-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Cc: Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org> Cc: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> Cc: Mahesh Salgaonkar <mahesh-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> --- kernel/hw_breakpoint.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c index 3b2aaff..6122f02 100644 --- a/kernel/hw_breakpoint.c +++ b/kernel/hw_breakpoint.c @@ -433,7 +433,8 @@ register_user_hw_breakpoint(struct perf_event_attr *attr, perf_overflow_handler_t triggered, struct task_struct *tsk) { - return perf_event_create_kernel_counter(attr, -1, tsk->pid, triggered); + return perf_event_create_kernel_counter(attr, -1, task_pid_vnr(tsk), + triggered); } EXPORT_SYMBOL_GPL(register_user_hw_breakpoint); -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] perf_events and hw_breakpoints fix and cleanup [not found] ` <1284408080-2135-1-git-send-email-matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> 2010-09-13 20:01 ` [PATCH 1/3] " Matt Helsley @ 2010-09-13 20:08 ` Frederic Weisbecker 1 sibling, 0 replies; 18+ messages in thread From: Frederic Weisbecker @ 2010-09-13 20:08 UTC (permalink / raw) To: Matt Helsley Cc: Peter Zijlstra, Mahesh Salgaonkar, containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Will Deacon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steven Rostedt, Arnaldo Carvalho de Melo, Ingo Molnar, Prasad On Mon, Sep 13, 2010 at 01:01:17PM -0700, Matt Helsley wrote: > Fixes a bug in the way that hardward breakpoints pass around [v]pids. > > The last two patches move the pid resolution closer to the syscall > and avoid passing pids entirely so that the perf event code does not > need to worry about being in the wrong pid namespace when kernel > counters are created. > > NOTE: Applies to -tip perf/core. Compile-tested only at the moment. Thanks a lot, I'll test that and push for -tip + backport in no big problem arise. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] perf_events and hw_breakpoints fix and cleanup 2010-09-13 20:01 [PATCH 0/3] perf_events and hw_breakpoints fix and cleanup Matt Helsley 2010-09-13 20:01 ` [PATCH 1/3] hw breakpoints: Fix pid namespace bug Matt Helsley [not found] ` <1284408080-2135-1-git-send-email-matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> @ 2010-09-13 20:08 ` Frederic Weisbecker 2 siblings, 0 replies; 18+ messages in thread From: Frederic Weisbecker @ 2010-09-13 20:08 UTC (permalink / raw) To: Matt Helsley Cc: containers, linux-kernel, Prasad, Peter Zijlstra, Arnaldo Carvalho de Melo, Steven Rostedt, Ingo Molnar, Will Deacon, Mahesh Salgaonkar On Mon, Sep 13, 2010 at 01:01:17PM -0700, Matt Helsley wrote: > Fixes a bug in the way that hardward breakpoints pass around [v]pids. > > The last two patches move the pid resolution closer to the syscall > and avoid passing pids entirely so that the perf event code does not > need to worry about being in the wrong pid namespace when kernel > counters are created. > > NOTE: Applies to -tip perf/core. Compile-tested only at the moment. Thanks a lot, I'll test that and push for -tip + backport in no big problem arise. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2010-09-22 12:23 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-13 20:01 [PATCH 0/3] perf_events and hw_breakpoints fix and cleanup Matt Helsley
2010-09-13 20:01 ` [PATCH 1/3] hw breakpoints: Fix pid namespace bug Matt Helsley
2010-09-13 20:01 ` [PATCH 2/3] perf events: Split out task search into helper Matt Helsley
2010-09-13 20:01 ` [PATCH 3/3] perf events: Cleanup pid passing Matt Helsley
2010-09-15 8:18 ` Peter Zijlstra
[not found] ` <a134e5e392ab0204961fd1a62c84a222bf5874a9.1284407763.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
2010-09-15 8:18 ` Peter Zijlstra
2010-09-15 10:03 ` [tip:perf/core] perf events: Clean up " tip-bot for Matt Helsley
2010-09-22 12:22 ` Peter Zijlstra
2010-09-15 10:03 ` [tip:perf/core] perf events: Split out task search into helper tip-bot for Matt Helsley
[not found] ` <f63454af09fb1915717251570423eb9ddd338340.1284407762.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
2010-09-13 20:01 ` [PATCH 2/3] " Matt Helsley
[not found] ` <561205417b450b8a4bf7488374541d64b4690431.1284407762.git.matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
2010-09-13 20:01 ` [PATCH 3/3] perf events: Cleanup pid passing Matt Helsley
2010-09-15 10:03 ` [tip:perf/core] hw breakpoints: Fix pid namespace bug tip-bot for Matt Helsley
2010-09-15 11:41 ` Frederic Weisbecker
2010-09-15 11:45 ` Ingo Molnar
2010-09-17 8:28 ` [tip:perf/urgent] " tip-bot for Matt Helsley
[not found] ` <1284408080-2135-1-git-send-email-matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
2010-09-13 20:01 ` [PATCH 1/3] " Matt Helsley
2010-09-13 20:08 ` [PATCH 0/3] perf_events and hw_breakpoints fix and cleanup Frederic Weisbecker
2010-09-13 20:08 ` Frederic Weisbecker
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.