| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <594C6746.2060401@huawei.com>
Date: Fri, 23 Jun 2017 08:56:38 +0800
From: zhouchengming <zhouchengming1@...wei.com>
To: Mark Rutland <mark.rutland@....com>
CC: <linux-kernel@...r.kernel.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Li Bin <huawei.libin@...wei.com>
Subject: Re: [PATCH] perf/core: fix group {cpu,task} validation
On 2017/6/22 22:41, Mark Rutland wrote:
> Regardless of which events form a group, it does not make sense for the
> events to target different tasks and/or CPUs, as this leaves the group
> inconsistent and impossible to schedule. The core perf code assumes that
> these are consistent across (successfully intialised) groups.
>
> Core perf code only verifies this when moving SW events into a HW
> context. Thus, we can violate this requirement for pure SW groups and
> pure HW groups, unless the relevant PMU driver happens to perform this
> verification itself. These mismatched groups subsequently wreak havoc
> elsewhere.
>
> For example, we handle watchpoints as SW events, and reserve watchpoint
> HW on a per-cpu basis at pmu::event_init() time to ensure that any event
> that is initialised is guaranteed to have a slot at pmu::add() time.
> However, the core code only checks the group leader's cpu filter (via
> event_filter_match()), and can thus install follower events onto CPUs
> violating thier (mismatched) CPU filters, potentially installing them
> into a CPU without sufficient reserved slots.
>
> This can be triggered with the below test case, resulting in warnings
> from arch backends.
>
> #define _GNU_SOURCE
> #include<linux/hw_breakpoint.h>
> #include<linux/perf_event.h>
> #include<sched.h>
> #include<stdio.h>
> #include<sys/prctl.h>
> #include<sys/syscall.h>
> #include<unistd.h>
>
> static int perf_event_open(struct perf_event_attr *attr, pid_t pid, int cpu,
> int group_fd, unsigned long flags)
> {
> return syscall(__NR_perf_event_open, attr, pid, cpu, group_fd, flags);
> }
>
> char watched_char;
>
> struct perf_event_attr wp_attr = {
> .type = PERF_TYPE_BREAKPOINT,
> .bp_type = HW_BREAKPOINT_RW,
> .bp_addr = (unsigned long)&watched_char,
> .bp_len = 1,
> .size = sizeof(wp_attr),
> };
>
> int main(int argc, char *argv[])
> {
> int leader, ret;
> cpu_set_t cpus;
>
> /*
> * Force use of CPU0 to ensure our CPU0-bound events get scheduled.
> */
> CPU_ZERO(&cpus);
> CPU_SET(0,&cpus);
> ret = sched_setaffinity(0, sizeof(cpus),&cpus);
> if (ret) {
> printf("Unable to set cpu affinity\n");
> return 1;
> }
>
> /* open leader event, bound to this task, CPU0 only */
> leader = perf_event_open(&wp_attr, 0, 0, -1, 0);
> if (leader< 0) {
> printf("Couldn't open leader: %d\n", leader);
> return 1;
> }
>
> /*
> * Open a follower event that is bound to the same task, but a
> * different CPU. This means that the group should never be possible to
> * schedule.
> */
> ret = perf_event_open(&wp_attr, 0, 1, leader, 0);
> if (ret< 0) {
> printf("Couldn't open mismatched follower: %d\n", ret);
> return 1;
> } else {
> printf("Opened leader/follower with mismastched CPUs\n");
> }
>
> /*
> * Open as many independent events as we can, all bound to the same
> * task, CPU0 only.
> */
> do {
> ret = perf_event_open(&wp_attr, 0, 0, -1, 0);
> } while (ret>= 0);
>
> /*
> * Force enable/disble all events to trigger the erronoeous
> * installation of the follower event.
> */
> printf("Opened all events. Toggling..\n");
> for (;;) {
> prctl(PR_TASK_PERF_EVENTS_DISABLE, 0, 0, 0, 0);
> prctl(PR_TASK_PERF_EVENTS_ENABLE, 0, 0, 0, 0);
> }
>
> return 0;
> }
Very good example!
>
> Fix this by validating this requirement regardless of whether we're
> moving events.
>
> Signed-off-by: Mark Rutland<mark.rutland@....com>
> Cc: Alexander Shishkin<alexander.shishkin@...ux.intel.com>
> Cc: Arnaldo Carvalho de Melo<acme@...nel.org>
> Cc: Ingo Molnar<mingo@...hat.com>
> Cc: Peter Zijlstra<peterz@...radead.org>
> Cc: Zhou Chengming<zhouchengming1@...wei.com>
> Cc: linux-kernel@...r.kernel.org
> ---
> kernel/events/core.c | 39 +++++++++++++++++++--------------------
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 6c4e523..1dca484 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10010,28 +10010,27 @@ static int perf_event_set_clock(struct perf_event *event, clockid_t clk_id)
> goto err_context;
>
> /*
> - * Do not allow to attach to a group in a different
> - * task or CPU context:
> + * Make sure we're both events for the same CPU;
> + * grouping events for different CPUs is broken; since
> + * you can never concurrently schedule them anyhow.
> */
> - if (move_group) {
> - /*
> - * Make sure we're both on the same task, or both
> - * per-cpu events.
> - */
> - if (group_leader->ctx->task != ctx->task)
> - goto err_context;
> + if (group_leader->cpu != event->cpu)
> + goto err_context;
>
> - /*
> - * Make sure we're both events for the same CPU;
> - * grouping events for different CPUs is broken; since
> - * you can never concurrently schedule them anyhow.
> - */
> - if (group_leader->cpu != event->cpu)
> - goto err_context;
> - } else {
> - if (group_leader->ctx != ctx)
> - goto err_context;
> - }
> + /*
> + * Make sure we're both on the same task, or both
> + * per-cpu events.
> + */
> + if (group_leader->ctx->task != ctx->task)
> + goto err_context;
> +
> + /*
> + * Do not allow to attach to a group in a different task
> + * or CPU context. If we're moving SW events, we'll fix
> + * this up later, so allow that.
> + */
> + if (!move_group&& group_leader->ctx != ctx)
> + goto err_context;
We don't need to check move_group here, the previous two checks already make sure
the events are on the same task and the same cpu. So when move_group needed, they
will be moved to the same taskctx or cpuctx then.
Thanks.
>
> /*
> * Only a group leader can be exclusive or pinned
Powered by blists - more mailing lists