| 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: <20220208130047.GA273989@leoy-ThinkPad-X240s>
Date: Tue, 8 Feb 2022 21:00:47 +0800
From: Leo Yan <leo.yan@...aro.org>
To: German Gomez <german.gomez@....com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
will@...nel.org, mark.rutland@....com, james.clark@....com
Subject: Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register
bit CX
Hi German,
On Mon, Feb 07, 2022 at 12:06:14PM +0000, German Gomez wrote:
[...]
> > I reviewed the code and also traced the backtrace for the function
> > arm_spe_pmu_start(), I can confirm that every time perf session will
> > execute below flow:
> >
> > evlist__enable()
> > __evlist__enable()
> > evlist__for_each_cpu() { -> call affinity__set()
> > evsel__enable_cpu()
> > }
> >
> > We can see the macro evlist__for_each_cpu() will extend to invoke
> > evlist__cpu_begin() and affinity__set(); affinity__set() will set CPU
> > affinity to the target CPU, thus perf process will firstly migrate to
> > the target CPU and enable event on the target CPU. This means perf
> > will not send remote IPI and it directly runs on target CPU, and the
> > dd program will not interfere capabilities for perf session.
>
> Thank you for looking at this,
>
> I re-tested on the N1SDP (previously I was using a graviton2 instance).
> I had to adjust the command slightly with "-m,2" to get it consistently
> this time:
>
> $ taskset --cpu-list 0 sudo dd if=/dev/random of=/dev/null &
> $ perf record -e arm_spe_0// -C0 -m,2 -- sleep 1
> $ perf report -D | grep CONTEXT | head
> . 0000000e: 65 b5 6e 00 00 CONTEXT 0x6eb5 el2
> . 0000004e: 65 b5 6e 00 00 CONTEXT 0x6eb5 el2
> . 0000008e: 65 b5 6e 00 00 CONTEXT 0x6eb5 el2
> [...]
Indeed! I can reproduce the issue now. And I can capture backtrace
for arm_spe_pmu_start() with below commands:
# cd /home/leoy/linux/tools/perf
# ./perf probe --add "arm_spe_pmu_start" -s /home/leoy/linux/ -k /home/leoy/linux/vmlinux
# echo 1 > /sys/kernel/debug/tracing/events/probe/arm_spe_pmu_start/enable
# echo stacktrace > /sys/kernel/debug/tracing/events/probe/arm_spe_pmu_start/trigger
... run your commands with non-root user ...
# cat /sys/kernel/debug/tracing/trace
dd-7697 [000] d.h2. 506.068700: arm_spe_pmu_start: (arm_spe_pmu_start+0x8/0xe0)
dd-7697 [000] d.h3. 506.068701: <stack trace>
=> kprobe_dispatcher
=> kprobe_breakpoint_handler
=> call_break_hook
=> brk_handler
=> do_debug_exception
=> el1_dbg
=> el1h_64_sync_handler
=> el1h_64_sync
=> arm_spe_pmu_start
=> event_sched_in.isra.0
=> merge_sched_in
=> visit_groups_merge.constprop.0
=> ctx_sched_in
=> perf_event_sched_in
=> ctx_resched
=> __perf_event_enable
=> event_function
=> remote_function
=> flush_smp_call_function_queue
=> generic_smp_call_function_single_interrupt
=> ipi_handler
=> handle_percpu_devid_irq
=> generic_handle_domain_irq
=> gic_handle_irq
=> call_on_irq_stack
=> do_interrupt_handler
=> el1_interrupt
=> el1h_64_irq_handler
=> el1h_64_irq
=> _raw_spin_unlock_irqrestore
=> urandom_read_nowarn.isra.0
=> random_read
=> vfs_read
=> ksys_read
=> __arm64_sys_read
=> invoke_syscall
=> el0_svc_common.constprop.0
=> do_el0_svc
=> el0_svc
=> el0t_64_sync_handler
=> el0t_64_sync
The backtrace clearly shows the function arm_spe_pmu_start() is
invoked in the 'dd' process (dd-7697); the flow is:
- perf program sends IPI to CPU0;
- 'dd' process is running on CPU0 and it's interrupted to handle IPI;
- 'dd' process has root capabilities, so it will enable context
tracing for non-root perf session.
> >> One way to fix this is by caching the value of the CX bit during the
> >> initialization of the PMU event, so that it remains consistent for the
> >> duration of the session.
> >>
> >> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?h=v5.16#n275
> >> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?h=v5.16#n713
> >> [3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?h=v5.16#n751
> >>
> >> Signed-off-by: German Gomez <german.gomez@....com>
> >> ---
> >> drivers/perf/arm_spe_pmu.c | 6 ++++--
> >> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> >> index d44bcc29d..8515bf85c 100644
> >> --- a/drivers/perf/arm_spe_pmu.c
> >> +++ b/drivers/perf/arm_spe_pmu.c
> >> @@ -57,6 +57,7 @@ struct arm_spe_pmu {
> >> u16 pmsver;
> >> u16 min_period;
> >> u16 counter_sz;
> >> + bool pmscr_cx;
So the patch makes sense to me. Just a minor comment:
Here we can define a u64 for recording pmscr value rather than a
bool value.
struct arm_spe_pmu {
...
u64 pmscr;
};
> >>
> >> #define SPE_PMU_FEAT_FILT_EVT (1UL << 0)
> >> #define SPE_PMU_FEAT_FILT_TYP (1UL << 1)
> >> @@ -260,6 +261,7 @@ static const struct attribute_group *arm_spe_pmu_attr_groups[] = {
> >> static u64 arm_spe_event_to_pmscr(struct perf_event *event)
> >> {
> >> struct perf_event_attr *attr = &event->attr;
> >> + struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu);
> >> u64 reg = 0;
> >>
> >> reg |= ATTR_CFG_GET_FLD(attr, ts_enable) << SYS_PMSCR_EL1_TS_SHIFT;
> >> @@ -272,7 +274,7 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event)
> >> if (!attr->exclude_kernel)
> >> reg |= BIT(SYS_PMSCR_EL1_E1SPE_SHIFT);
> >>
> >> - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
> >> + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && spe_pmu->pmscr_cx)
> >> reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
> >>
> >> return reg;
> >> @@ -709,10 +711,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
> >> !(spe_pmu->features & SPE_PMU_FEAT_FILT_LAT))
> >> return -EOPNOTSUPP;
> >>
> >> + spe_pmu->pmscr_cx = perfmon_capable();
> >> reg = arm_spe_event_to_pmscr(event);
Thus here we can change as:
spe_pmu->pmscr = arm_spe_event_to_pmscr(event);
And then in the function arm_spe_pmu_start(), we can skip calling
arm_spe_event_to_pmscr() and directly set PMSCR register:
static void arm_spe_pmu_start(struct perf_event *event, int flags)
{
...
isb();
write_sysreg_s(spe_pmu->pmscr, SYS_PMSCR_EL1);
}
Thanks,
Leo
> >> if (!perfmon_capable() &&
> >> (reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) |
> >> - BIT(SYS_PMSCR_EL1_CX_SHIFT) |
> >> BIT(SYS_PMSCR_EL1_PCT_SHIFT))))
> >> return -EACCES;
> >>
> >> --
> >> 2.25.1
> >>
Powered by blists - more mailing lists