Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1676304 > unrolled thread

Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the  kernel in the "skid" region Kyle Huey <me@kylehuey.com> - 2017-06-28 03:10 +0200
    Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the  kernel in the "skid" region "Jin, Yao" <yao.jin@linux.intel.com> - 2017-06-28 04:10 +0200
      Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the  kernel in the "skid" region Kyle Huey <me@kylehuey.com> - 2017-06-28 07:00 +0200
        Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the  kernel in the "skid" region "Jin, Yao" <yao.jin@linux.intel.com> - 2017-06-28 07:40 +0200
          Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the  kernel in the "skid" region Kyle Huey <me@kylehuey.com> - 2017-06-28 09:40 +0200
        Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the  kernel in the "skid" region Mark Rutland <mark.rutland@arm.com> - 2017-06-28 12:20 +0200
          [PATCH] perf/core: generate overflow signal when samples are dropped  (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the  kernel in the "skid" region) Mark Rutland <mark.rutland@arm.com> - 2017-06-28 13:00 +0200
            Re: [PATCH] perf/core: generate overflow signal when samples are  dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we  entered the kernel in the "skid" region) Vince Weaver <vincent.weaver@maine.edu> - 2017-06-28 14:50 +0200
              Re: [PATCH] perf/core: generate overflow signal when samples are  dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we  entered the kernel in the "skid" region) Mark Rutland <mark.rutland@arm.com> - 2017-06-28 15:10 +0200
                Re: [PATCH] perf/core: generate overflow signal when samples are  dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we  entered the kernel in the "skid" region) Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-06-29 10:20 +0200
                  Re: [PATCH] perf/core: generate overflow signal when samples are  dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we  entered the kernel in the "skid" region) Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-06-29 10:30 +0200
            Re: [PATCH] perf/core: generate overflow signal when samples are  dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we  entered the kernel in the "skid" region) Mark Rutland <mark.rutland@arm.com> - 2017-06-28 20:00 +0200
              Re: [PATCH] perf/core: generate overflow signal when samples are  dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we  entered the kernel in the "skid" region) Kyle Huey <me@kylehuey.com> - 2017-06-29 01:00 +0200
                Re: [PATCH] perf/core: generate overflow signal when samples are  dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we  entered the kernel in the "skid" region) "Jin, Yao" <yao.jin@linux.intel.com> - 2017-06-29 02:30 +0200
                Re: [PATCH] perf/core: generate overflow signal when samples are  dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we  entered the kernel in the "skid" region) Kyle Huey <me@kylehuey.com> - 2017-06-30 19:50 +0200
              Re: [PATCH] perf/core: generate overflow signal when samples are  dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we  entered the kernel in the "skid" region) Ingo Molnar <mingo@kernel.org> - 2017-06-29 10:20 +0200
            Re: [PATCH] perf/core: generate overflow signal when samples are  dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we  entered the kernel in the "skid" region) Kyle Huey <me@kylehuey.com> - 2017-06-28 20:00 +0200
          Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the  kernel in the "skid" region "Robert O'Callahan" <robert@ocallahan.org> - 2017-06-28 20:00 +0200
          Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the  kernel in the "skid" region Kyle Huey <me@kylehuey.com> - 2017-06-28 20:00 +0200
            Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the  kernel in the "skid" region Mark Rutland <mark.rutland@arm.com> - 2017-06-28 20:00 +0200
          Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the  kernel in the "skid" region Mark Rutland <mark.rutland@arm.com> - 2017-06-28 20:00 +0200
          Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the  kernel in the "skid" region Kyle Huey <me@kylehuey.com> - 2017-06-28 20:00 +0200

Page 1 of 2  [1] 2  Next page →

#1676304 — Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region

Sent again with LKML CCd, sorry for the noise.

- Kyle

On Tue, Jun 27, 2017 at 5:38 PM, Kyle Huey <me@kylehuey.com> wrote:
> cc1582c231ea introduced a regression in v4.12.0-rc5, and appears to be
> a candidate for backporting to stable branches.
>
> rr, a userspace record and replay debugger[0], uses the PMU interrupt
> to stop a program during replay to inject asynchronous events such as
> signals.  We are counting retired conditional branches in userspace
> only.  This changeset causes the kernel to drop interrupts on the
> floor if, during the PMU interrupt's "skid" region, the CPU enters
> kernel mode for whatever reason.  When replaying traces of complex
> programs such as Firefox, we intermittently fail to deliver
> asynchronous events on time, leading the replay to diverge from the
> recorded state.
>
> It seems like this change should, at a bare minimum, be limited to
> counters that actually perform sampling of register state when the
> interrupt fires.  In our case, with the retired conditional branches
> counter restricted to counting userspace events only, it makes no
> difference that the PMU interrupt happened to be delivered in the
> kernel.
>
> As this makes rr unusable on complex applications and cannot be
> efficiently worked around, we would appreciate this being addressed
> before 4.12 is finalized, and the regression not being introduced to
> stable branches.
>
> Thanks,
>
> - Kyle
>
> [0] http://rr-project.org/

[toc] | [next] | [standalone]

#1676318

Hi,

In theory, the PMI interrupts in skid region should be dropped, right?

For a userspace debugger, is it the only choice that relies on the 
*skid* PMI interrupt?

Thanks
Jin Yao

On 6/28/2017 9:01 AM, Kyle Huey wrote:
> Sent again with LKML CCd, sorry for the noise.
>
> - Kyle
>
> On Tue, Jun 27, 2017 at 5:38 PM, Kyle Huey <me@kylehuey.com> wrote:
>> cc1582c231ea introduced a regression in v4.12.0-rc5, and appears to be
>> a candidate for backporting to stable branches.
>>
>> rr, a userspace record and replay debugger[0], uses the PMU interrupt
>> to stop a program during replay to inject asynchronous events such as
>> signals.  We are counting retired conditional branches in userspace
>> only.  This changeset causes the kernel to drop interrupts on the
>> floor if, during the PMU interrupt's "skid" region, the CPU enters
>> kernel mode for whatever reason.  When replaying traces of complex
>> programs such as Firefox, we intermittently fail to deliver
>> asynchronous events on time, leading the replay to diverge from the
>> recorded state.
>>
>> It seems like this change should, at a bare minimum, be limited to
>> counters that actually perform sampling of register state when the
>> interrupt fires.  In our case, with the retired conditional branches
>> counter restricted to counting userspace events only, it makes no
>> difference that the PMU interrupt happened to be delivered in the
>> kernel.
>>
>> As this makes rr unusable on complex applications and cannot be
>> efficiently worked around, we would appreciate this being addressed
>> before 4.12 is finalized, and the regression not being introduced to
>> stable branches.
>>
>> Thanks,
>>
>> - Kyle
>>
>> [0] http://rr-project.org/

[toc] | [prev] | [next] | [standalone]

#1676372

On Tue, Jun 27, 2017 at 7:09 PM, Jin, Yao <yao.jin@linux.intel.com> wrote:
> Hi,
>
> In theory, the PMI interrupts in skid region should be dropped, right?

No, why would they be dropped?

My understanding of the situation is as follows:

There is some time, call it t_0, where the hardware counter overflows.
The PMU triggers an interrupt, but this is not instantaneous.  Call
the time when the interrupt is actually delivered t_1.  Then t_1 - t_0
is the "skid".

Note that if the counter is `exclude_kernel`, then at t_0 the CPU
*must* be running a userspace program.  But by t_1, the CPU may be
doing something else.  Your patch changed things so that if at t_1 the
CPU is in the kernel, then the interrupt is discarded.  But rr has
programmed the counter to deliver a signal on overflow (via F_SETSIG
on the fd returned by perf_event_open).  This change results in the
signal never being delivered, because the interrupt was ignored.
(More accurately, the signal is delivered the *next* time the counter
overflows, which is far past where we wanted to inject our
asynchronous event into our tracee.

It seems to me that it might be reasonable to ignore the interrupt if
the purpose of the interrupt is to trigger sampling of the CPUs
register state.  But if the interrupt will trigger some other
operation, such as a signal on an fd, then there's no reason to drop
it.

> For a userspace debugger, is it the only choice that relies on the *skid*
> PMI interrupt?

I don't understand this question, but hopefully the above clarified things.

- Kyle

> Thanks
> Jin Yao
>
>
> On 6/28/2017 9:01 AM, Kyle Huey wrote:
>>
>> Sent again with LKML CCd, sorry for the noise.
>>
>> - Kyle
>>
>> On Tue, Jun 27, 2017 at 5:38 PM, Kyle Huey <me@kylehuey.com> wrote:
>>>
>>> cc1582c231ea introduced a regression in v4.12.0-rc5, and appears to be
>>> a candidate for backporting to stable branches.
>>>
>>> rr, a userspace record and replay debugger[0], uses the PMU interrupt
>>> to stop a program during replay to inject asynchronous events such as
>>> signals.  We are counting retired conditional branches in userspace
>>> only.  This changeset causes the kernel to drop interrupts on the
>>> floor if, during the PMU interrupt's "skid" region, the CPU enters
>>> kernel mode for whatever reason.  When replaying traces of complex
>>> programs such as Firefox, we intermittently fail to deliver
>>> asynchronous events on time, leading the replay to diverge from the
>>> recorded state.
>>>
>>> It seems like this change should, at a bare minimum, be limited to
>>> counters that actually perform sampling of register state when the
>>> interrupt fires.  In our case, with the retired conditional branches
>>> counter restricted to counting userspace events only, it makes no
>>> difference that the PMU interrupt happened to be delivered in the
>>> kernel.
>>>
>>> As this makes rr unusable on complex applications and cannot be
>>> efficiently worked around, we would appreciate this being addressed
>>> before 4.12 is finalized, and the regression not being introduced to
>>> stable branches.
>>>
>>> Thanks,
>>>
>>> - Kyle
>>>
>>> [0] http://rr-project.org/
>
>

[toc] | [prev] | [next] | [standalone]

#1676383

Hi Kyle,

I understand your requirement. Sorry I don't know the detail of rr 
debugger, but I guess if it just uses counter overflow to deliver a 
signal, could it set the counter without "exclude_kernel"?

Another consideration is, I'm not sure if the kernel can accurately 
realize that if the interrupt is to trigger sampling or just deliver a 
signal. Userspace may know that but kernel may not.

Anyway let's listen to more comments from community.

Thanks

Jin Yao


On 6/28/2017 12:51 PM, Kyle Huey wrote:
> On Tue, Jun 27, 2017 at 7:09 PM, Jin, Yao <yao.jin@linux.intel.com> wrote:
>> Hi,
>>
>> In theory, the PMI interrupts in skid region should be dropped, right?
> No, why would they be dropped?
>
> My understanding of the situation is as follows:
>
> There is some time, call it t_0, where the hardware counter overflows.
> The PMU triggers an interrupt, but this is not instantaneous.  Call
> the time when the interrupt is actually delivered t_1.  Then t_1 - t_0
> is the "skid".
>
> Note that if the counter is `exclude_kernel`, then at t_0 the CPU
> *must* be running a userspace program.  But by t_1, the CPU may be
> doing something else.  Your patch changed things so that if at t_1 the
> CPU is in the kernel, then the interrupt is discarded.  But rr has
> programmed the counter to deliver a signal on overflow (via F_SETSIG
> on the fd returned by perf_event_open).  This change results in the
> signal never being delivered, because the interrupt was ignored.
> (More accurately, the signal is delivered the *next* time the counter
> overflows, which is far past where we wanted to inject our
> asynchronous event into our tracee.
>
> It seems to me that it might be reasonable to ignore the interrupt if
> the purpose of the interrupt is to trigger sampling of the CPUs
> register state.  But if the interrupt will trigger some other
> operation, such as a signal on an fd, then there's no reason to drop
> it.
>
>> For a userspace debugger, is it the only choice that relies on the *skid*
>> PMI interrupt?
> I don't understand this question, but hopefully the above clarified things.
>
> - Kyle
>
>> Thanks
>> Jin Yao
>>
>>
>> On 6/28/2017 9:01 AM, Kyle Huey wrote:
>>> Sent again with LKML CCd, sorry for the noise.
>>>
>>> - Kyle
>>>
>>> On Tue, Jun 27, 2017 at 5:38 PM, Kyle Huey <me@kylehuey.com> wrote:
>>>> cc1582c231ea introduced a regression in v4.12.0-rc5, and appears to be
>>>> a candidate for backporting to stable branches.
>>>>
>>>> rr, a userspace record and replay debugger[0], uses the PMU interrupt
>>>> to stop a program during replay to inject asynchronous events such as
>>>> signals.  We are counting retired conditional branches in userspace
>>>> only.  This changeset causes the kernel to drop interrupts on the
>>>> floor if, during the PMU interrupt's "skid" region, the CPU enters
>>>> kernel mode for whatever reason.  When replaying traces of complex
>>>> programs such as Firefox, we intermittently fail to deliver
>>>> asynchronous events on time, leading the replay to diverge from the
>>>> recorded state.
>>>>
>>>> It seems like this change should, at a bare minimum, be limited to
>>>> counters that actually perform sampling of register state when the
>>>> interrupt fires.  In our case, with the retired conditional branches
>>>> counter restricted to counting userspace events only, it makes no
>>>> difference that the PMU interrupt happened to be delivered in the
>>>> kernel.
>>>>
>>>> As this makes rr unusable on complex applications and cannot be
>>>> efficiently worked around, we would appreciate this being addressed
>>>> before 4.12 is finalized, and the regression not being introduced to
>>>> stable branches.
>>>>
>>>> Thanks,
>>>>
>>>> - Kyle
>>>>
>>>> [0] http://rr-project.org/
>>

[toc] | [prev] | [next] | [standalone]

#1676426

On Tue, Jun 27, 2017 at 10:35 PM, Jin, Yao <yao.jin@linux.intel.com> wrote:
> Hi Kyle,
>
> I understand your requirement. Sorry I don't know the detail of rr debugger,
> but I guess if it just uses counter overflow to deliver a signal, could it
> set the counter without "exclude_kernel"?

Unfortunately we cannot.  We depend on the counter value being exactly
the same between recording and replay, and dropping `exclude_kernel`
would introduce non-determinism.

> Another consideration is, I'm not sure if the kernel can accurately realize
> that if the interrupt is to trigger sampling or just deliver a signal.
> Userspace may know that but kernel may not.

After looking at this code a bit more, I think that changing the
`is_sample_allowed` check from an early return to a guard around the
invocation of `overflow_handler` would fix this.  I believe, but have
not tested, that `perf_event_fasync` is what must run to deliver our
signal, while the `overflow_handler` is what copies the kernel RIP/etc
into the output buffer that you want to skip.

- Kyle

> Anyway let's listen to more comments from community.
>
> Thanks
>
> Jin Yao
>
>
>
> On 6/28/2017 12:51 PM, Kyle Huey wrote:
>>
>> On Tue, Jun 27, 2017 at 7:09 PM, Jin, Yao <yao.jin@linux.intel.com> wrote:
>>>
>>> Hi,
>>>
>>> In theory, the PMI interrupts in skid region should be dropped, right?
>>
>> No, why would they be dropped?
>>
>> My understanding of the situation is as follows:
>>
>> There is some time, call it t_0, where the hardware counter overflows.
>> The PMU triggers an interrupt, but this is not instantaneous.  Call
>> the time when the interrupt is actually delivered t_1.  Then t_1 - t_0
>> is the "skid".
>>
>> Note that if the counter is `exclude_kernel`, then at t_0 the CPU
>> *must* be running a userspace program.  But by t_1, the CPU may be
>> doing something else.  Your patch changed things so that if at t_1 the
>> CPU is in the kernel, then the interrupt is discarded.  But rr has
>> programmed the counter to deliver a signal on overflow (via F_SETSIG
>> on the fd returned by perf_event_open).  This change results in the
>> signal never being delivered, because the interrupt was ignored.
>> (More accurately, the signal is delivered the *next* time the counter
>> overflows, which is far past where we wanted to inject our
>> asynchronous event into our tracee.
>>
>> It seems to me that it might be reasonable to ignore the interrupt if
>> the purpose of the interrupt is to trigger sampling of the CPUs
>> register state.  But if the interrupt will trigger some other
>> operation, such as a signal on an fd, then there's no reason to drop
>> it.
>>
>>> For a userspace debugger, is it the only choice that relies on the *skid*
>>> PMI interrupt?
>>
>> I don't understand this question, but hopefully the above clarified
>> things.
>>
>> - Kyle
>>
>>> Thanks
>>> Jin Yao
>>>
>>>
>>> On 6/28/2017 9:01 AM, Kyle Huey wrote:
>>>>
>>>> Sent again with LKML CCd, sorry for the noise.
>>>>
>>>> - Kyle
>>>>
>>>> On Tue, Jun 27, 2017 at 5:38 PM, Kyle Huey <me@kylehuey.com> wrote:
>>>>>
>>>>> cc1582c231ea introduced a regression in v4.12.0-rc5, and appears to be
>>>>> a candidate for backporting to stable branches.
>>>>>
>>>>> rr, a userspace record and replay debugger[0], uses the PMU interrupt
>>>>> to stop a program during replay to inject asynchronous events such as
>>>>> signals.  We are counting retired conditional branches in userspace
>>>>> only.  This changeset causes the kernel to drop interrupts on the
>>>>> floor if, during the PMU interrupt's "skid" region, the CPU enters
>>>>> kernel mode for whatever reason.  When replaying traces of complex
>>>>> programs such as Firefox, we intermittently fail to deliver
>>>>> asynchronous events on time, leading the replay to diverge from the
>>>>> recorded state.
>>>>>
>>>>> It seems like this change should, at a bare minimum, be limited to
>>>>> counters that actually perform sampling of register state when the
>>>>> interrupt fires.  In our case, with the retired conditional branches
>>>>> counter restricted to counting userspace events only, it makes no
>>>>> difference that the PMU interrupt happened to be delivered in the
>>>>> kernel.
>>>>>
>>>>> As this makes rr unusable on complex applications and cannot be
>>>>> efficiently worked around, we would appreciate this being addressed
>>>>> before 4.12 is finalized, and the regression not being introduced to
>>>>> stable branches.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> - Kyle
>>>>>
>>>>> [0] http://rr-project.org/
>>>
>>>
>

[toc] | [prev] | [next] | [standalone]

#1676513

On Tue, Jun 27, 2017 at 09:51:00PM -0700, Kyle Huey wrote:
> On Tue, Jun 27, 2017 at 7:09 PM, Jin, Yao <yao.jin@linux.intel.com> wrote:
> > Hi,
> >
> > In theory, the PMI interrupts in skid region should be dropped, right?
> 
> No, why would they be dropped?
> 
> My understanding of the situation is as follows:
> 
> There is some time, call it t_0, where the hardware counter overflows.
> The PMU triggers an interrupt, but this is not instantaneous.  Call
> the time when the interrupt is actually delivered t_1.  Then t_1 - t_0
> is the "skid".
> 
> Note that if the counter is `exclude_kernel`, then at t_0 the CPU
> *must* be running a userspace program.  But by t_1, the CPU may be
> doing something else.  Your patch changed things so that if at t_1 the
> CPU is in the kernel, then the interrupt is discarded.  But rr has
> programmed the counter to deliver a signal on overflow (via F_SETSIG
> on the fd returned by perf_event_open).  This change results in the
> signal never being delivered, because the interrupt was ignored.
> (More accurately, the signal is delivered the *next* time the counter
> overflows, which is far past where we wanted to inject our
> asynchronous event into our tracee.

Yes, this is a bug.

As we're trying to avoid smapling state, I think we can move the check
into perf_prepare_sample() or __perf_event_output(), where that state is
actually sampled. I'll take a look at that momentarily.

Just to clarify, you don't care about the sample state at all? i.e. you
don't need the user program counter?

Is that signal delivered to the tracee, or to a different process that
traces it? If the latter, what ensures that the task is stopped
sufficiently quickly?

> It seems to me that it might be reasonable to ignore the interrupt if
> the purpose of the interrupt is to trigger sampling of the CPUs
> register state.  But if the interrupt will trigger some other
> operation, such as a signal on an fd, then there's no reason to drop
> it.

Agreed. I'll try to have a patch for this soon.

I just need to figure out exactly where that overflow signal is
generated by the perf core.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1676554 — [PATCH] perf/core: generate overflow signal when samples are dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region)

On Wed, Jun 28, 2017 at 11:12:48AM +0100, Mark Rutland wrote:
> On Tue, Jun 27, 2017 at 09:51:00PM -0700, Kyle Huey wrote:

> As we're trying to avoid smapling state, I think we can move the check
> into perf_prepare_sample() or __perf_event_output(), where that state is
> actually sampled. I'll take a look at that momentarily.
> 
> Just to clarify, you don't care about the sample state at all? i.e. you
> don't need the user program counter?
> 
> Is that signal delivered to the tracee, or to a different process that
> traces it? If the latter, what ensures that the task is stopped
> sufficiently quickly?
> 
> > It seems to me that it might be reasonable to ignore the interrupt if
> > the purpose of the interrupt is to trigger sampling of the CPUs
> > register state.  But if the interrupt will trigger some other
> > operation, such as a signal on an fd, then there's no reason to drop
> > it.
> 
> Agreed. I'll try to have a patch for this soon.
> 
> I just need to figure out exactly where that overflow signal is
> generated by the perf core.

I've figured that out now. That's handled by perf_pending_event(), whcih
is the irq_work we schedule in __perf_event_overflow().

Does the below patch work for you?

---->8----
From bb1f99dace508ce34ab0818f91d59e73450fa142 Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Wed, 28 Jun 2017 11:39:25 +0100
Subject: [PATCH] perf/core: generate overflow signal when samples are dropped

We recently tried to kill an information leak where users could receive
kernel samples due to skid on the PMU interrupt. To block this, we
bailed out early in perf_event_overflow(), as we do for non-sampling
events.

This broke rr, which uses sampling events to receive a signal on
overflow (but does not care about the contents of the sample). These
signals are critical to the correct operation of rr.

Instead of bailing out early in perf_event_overflow, we can bail prior
to performing the actual sampling in __perf_event_output(). This avoids
the information leak, but preserves the generation of the signal.

Since we don't place any sample data into the ring buffer, the signal is
arguably spurious. However, a userspace ringbuffer consumer can already
consume data prior to taking the associated signals, and therefore must
handle spurious signals to operate correctly. Thus, this signal
shouldn't be harmful.

Fixes: cc1582c231ea041f ("perf/core: Drop kernel samples even though :u is specified")
Reported-by: Kyle Huey <me@kylehuey.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c | 46 +++++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 6c4e523..6b263f3 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6090,6 +6090,21 @@ void perf_prepare_sample(struct perf_event_header *header,
 	}
 }
 
+static bool sample_is_allowed(struct perf_event *event, struct pt_regs *regs)
+{
+	/*
+	 * Due to interrupt latency (AKA "skid"), we may enter the
+	 * kernel before taking an overflow, even if the PMU is only
+	 * counting user events.
+	 * To avoid leaking information to userspace, we must always
+	 * reject kernel samples when exclude_kernel is set.
+	 */
+	if (event->attr.exclude_kernel && !user_mode(regs))
+		return false;
+
+	return true;
+}
+
 static void __always_inline
 __perf_event_output(struct perf_event *event,
 		    struct perf_sample_data *data,
@@ -6101,6 +6116,12 @@ void perf_prepare_sample(struct perf_event_header *header,
 	struct perf_output_handle handle;
 	struct perf_event_header header;
 
+	/*
+	 * For security, drop the skid kernel samples if necessary.
+	 */
+	if (!sample_is_allowed(event, regs))
+		return ret;
+
 	/* protect the callchain buffers */
 	rcu_read_lock();
 
@@ -7316,21 +7337,6 @@ int perf_event_account_interrupt(struct perf_event *event)
 	return __perf_event_account_interrupt(event, 1);
 }
 
-static bool sample_is_allowed(struct perf_event *event, struct pt_regs *regs)
-{
-	/*
-	 * Due to interrupt latency (AKA "skid"), we may enter the
-	 * kernel before taking an overflow, even if the PMU is only
-	 * counting user events.
-	 * To avoid leaking information to userspace, we must always
-	 * reject kernel samples when exclude_kernel is set.
-	 */
-	if (event->attr.exclude_kernel && !user_mode(regs))
-		return false;
-
-	return true;
-}
-
 /*
  * Generic event overflow handling, sampling.
  */
@@ -7352,12 +7358,6 @@ static int __perf_event_overflow(struct perf_event *event,
 	ret = __perf_event_account_interrupt(event, throttle);
 
 	/*
-	 * For security, drop the skid kernel samples if necessary.
-	 */
-	if (!sample_is_allowed(event, regs))
-		return ret;
-
-	/*
 	 * XXX event_limit might not quite work as expected on inherited
 	 * events
 	 */
@@ -7372,6 +7372,10 @@ static int __perf_event_overflow(struct perf_event *event,
 
 	READ_ONCE(event->overflow_handler)(event, data, regs);
 
+	/*
+	 * We must generate a wakeup regardless of whether we actually
+	 * generated a sample. This is relied upon by rr.
+	 */
 	if (*perf_event_fasync(event) && event->pending_kill) {
 		event->pending_wakeup = 1;
 		irq_work_queue(&event->pending);
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1676628 — Re: [PATCH] perf/core: generate overflow signal when samples are dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region)

On Wed, 28 Jun 2017, Mark Rutland wrote:

> On Wed, Jun 28, 2017 at 11:12:48AM +0100, Mark Rutland wrote:

> Instead of bailing out early in perf_event_overflow, we can bail prior
> to performing the actual sampling in __perf_event_output(). This avoids
> the information leak, but preserves the generation of the signal.
> 
> Since we don't place any sample data into the ring buffer, the signal is
> arguably spurious. However, a userspace ringbuffer consumer can already
> consume data prior to taking the associated signals, and therefore must
> handle spurious signals to operate correctly. Thus, this signal
> shouldn't be harmful.

this could still break some of my perf_event validation tests.

Ones that set up a sampling event for every 1M instructions, run for 100M 
instructions, and expect there to be 100 samples received.

If we're so worried about info leakage, can't we just zero-out the problem 
address (or randomize the kernel address) rather than just pretending the 
interrupt didn't happen?

Vince

[toc] | [prev] | [next] | [standalone]

#1676644 — Re: [PATCH] perf/core: generate overflow signal when samples are dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region)

On Wed, Jun 28, 2017 at 08:40:30AM -0400, Vince Weaver wrote:
> On Wed, 28 Jun 2017, Mark Rutland wrote:
> 
> > On Wed, Jun 28, 2017 at 11:12:48AM +0100, Mark Rutland wrote:
> 
> > Instead of bailing out early in perf_event_overflow, we can bail prior
> > to performing the actual sampling in __perf_event_output(). This avoids
> > the information leak, but preserves the generation of the signal.
> > 
> > Since we don't place any sample data into the ring buffer, the signal is
> > arguably spurious. However, a userspace ringbuffer consumer can already
> > consume data prior to taking the associated signals, and therefore must
> > handle spurious signals to operate correctly. Thus, this signal
> > shouldn't be harmful.
> 
> this could still break some of my perf_event validation tests.
> 
> Ones that set up a sampling event for every 1M instructions, run for 100M 
> instructions, and expect there to be 100 samples received.

Is that test reliable today?

I'd expect that at least on ARM it's not, given that events can be
counted imprecisely, and mode filters can be applied imprecisely. So you
might get fewer (or more) samples. I'd imagine similar is true on other
archtiectures.

If sampling took long enough, the existing ratelimiting could come into
effect, too.

Surely that already has some error margin?

> If we're so worried about info leakage, can't we just zero-out the problem 
> address (or randomize the kernel address) rather than just pretending the 
> interrupt didn't happen?

Making up zeroed or randomized data is going to confuse users. I can't
imagine that real users are going to want bogus samples that they have
to identify (somehow) in order to skip when processing the data.

I can see merit in signalling "lost" samples to userspace, so long as
they're easily distinguished from real samples.

One option is to fake up a sample using the user regs regardless, but
that's both fragile and surprising in other cases.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1677510 — Re: [PATCH] perf/core: generate overflow signal when samples are dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region)

Hi Folks,

On 28.06.2017 16:07, Mark Rutland wrote:
> On Wed, Jun 28, 2017 at 08:40:30AM -0400, Vince Weaver wrote:
>> On Wed, 28 Jun 2017, Mark Rutland wrote:
>>
>>> On Wed, Jun 28, 2017 at 11:12:48AM +0100, Mark Rutland wrote:
>>
>>> Instead of bailing out early in perf_event_overflow, we can bail prior
>>> to performing the actual sampling in __perf_event_output(). This avoids
>>> the information leak, but preserves the generation of the signal.
>>>
>>> Since we don't place any sample data into the ring buffer, the signal is
>>> arguably spurious. However, a userspace ringbuffer consumer can already
>>> consume data prior to taking the associated signals, and therefore must
>>> handle spurious signals to operate correctly. Thus, this signal
>>> shouldn't be harmful.
>>
>> this could still break some of my perf_event validation tests.
>>
>> Ones that set up a sampling event for every 1M instructions, run for 100M 
>> instructions, and expect there to be 100 samples received.
> 
> Is that test reliable today?
> 
> I'd expect that at least on ARM it's not, given that events can be
> counted imprecisely, and mode filters can be applied imprecisely. So you
> might get fewer (or more) samples. I'd imagine similar is true on other
> archtiectures.
> 
> If sampling took long enough, the existing ratelimiting could come into
> effect, too.
> 
> Surely that already has some error margin?

FYI.

From my recent experience and observation (on Intel Xeon Phi) 
wakeup_events_overflow and overflow_poll tests may fail if 
/proc/sys/kernel/perf_event_max_sample_rate is low enough:

# cat /proc/sys/kernel/perf_event_max_sample_rate
6000
# abudanko/perf_event_tests/tests/overflow/wakeup_events_overflow
This tests wakeup event overflows.
Testing with wakeup_events=1.
Counts, using mmap buffer 0x7f707b0dc000
	POLL_IN : 10
	POLL_OUT: 0
	POLL_MSG: 0
	POLL_ERR: 0
	POLL_PRI: 0
	POLL_HUP: 0
	UNKNOWN : 0
Testing wakeup events overflow...                            PASSED
# abudanko/perf_event_tests/tests/overflow/overflow_poll
This tests using poll() to catch overflow.
Monitoring pid 131412 status 1407
Child has stopped due to signal 5 (Trace/breakpoint trap)
Continuing child
Returned HUP!
Counts, using mmap buffer 0x7fb3bfe04000
	POLL_IN : 10
	POLL_OUT: 0
	POLL_MSG: 0
	POLL_ERR: 0
	POLL_PRI: 0
	POLL_HUP: 1
	UNKNOWN : 0
Testing catching overflow with poll()...                     PASSED

# echo 1000 > /proc/sys/kernel/perf_event_max_sample_rate
# abudanko/perf_event_tests/tests/overflow/overflow_poll
This tests using poll() to catch overflow.
Monitoring pid 131551 status 1407
Child has stopped due to signal 5 (Trace/breakpoint trap)
Continuing child
Returned HUP!
Counts, using mmap buffer 0x7f80532df000
	POLL_IN : 9
	POLL_OUT: 0
	POLL_MSG: 0
	POLL_ERR: 0
	POLL_PRI: 0
	POLL_HUP: 1
	UNKNOWN : 0
Unexpected POLL_IN interrupt.
Testing catching overflow with poll()...                     FAILED
[root@nntpdsd52-210 ~]# abudanko/perf_event_tests/tests/overflow/overflow_poll
This tests using poll() to catch overflow.
Monitoring pid 131553 status 1407
Child has stopped due to signal 5 (Trace/breakpoint trap)
Continuing child
Returned HUP!
Counts, using mmap buffer 0x7f650952c000
	POLL_IN : 9
	POLL_OUT: 0
	POLL_MSG: 0
	POLL_ERR: 0
	POLL_PRI: 0
	POLL_HUP: 1
	UNKNOWN : 0
Unexpected POLL_IN interrupt.
Testing catching overflow with poll()...                     FAILED

> 
>> If we're so worried about info leakage, can't we just zero-out the problem 
>> address (or randomize the kernel address) rather than just pretending the 
>> interrupt didn't happen?
> 
> Making up zeroed or randomized data is going to confuse users. I can't
> imagine that real users are going to want bogus samples that they have
> to identify (somehow) in order to skip when processing the data.
> 
> I can see merit in signalling "lost" samples to userspace, so long as
> they're easily distinguished from real samples.
> 
> One option is to fake up a sample using the user regs regardless, but
> that's both fragile and surprising in other cases.
> 
> Thanks,
> Mark.
> 

Thanks,
Alexey

[toc] | [prev] | [next] | [standalone]

#1677518 — Re: [PATCH] perf/core: generate overflow signal when samples are dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region)

On 29.06.2017 11:13, Alexey Budankov wrote:
> Hi Folks,
> 
> On 28.06.2017 16:07, Mark Rutland wrote:
>> On Wed, Jun 28, 2017 at 08:40:30AM -0400, Vince Weaver wrote:
>>> On Wed, 28 Jun 2017, Mark Rutland wrote:
>>>
>>>> On Wed, Jun 28, 2017 at 11:12:48AM +0100, Mark Rutland wrote:
>>>
>>>> Instead of bailing out early in perf_event_overflow, we can bail prior
>>>> to performing the actual sampling in __perf_event_output(). This avoids
>>>> the information leak, but preserves the generation of the signal.
>>>>
>>>> Since we don't place any sample data into the ring buffer, the signal is
>>>> arguably spurious. However, a userspace ringbuffer consumer can already
>>>> consume data prior to taking the associated signals, and therefore must
>>>> handle spurious signals to operate correctly. Thus, this signal
>>>> shouldn't be harmful.
>>>
>>> this could still break some of my perf_event validation tests.
>>>
>>> Ones that set up a sampling event for every 1M instructions, run for 100M 
>>> instructions, and expect there to be 100 samples received.
>>
>> Is that test reliable today?
>>
>> I'd expect that at least on ARM it's not, given that events can be
>> counted imprecisely, and mode filters can be applied imprecisely. So you
>> might get fewer (or more) samples. I'd imagine similar is true on other
>> archtiectures.
>>
>> If sampling took long enough, the existing ratelimiting could come into
>> effect, too.
>>
>> Surely that already has some error margin?
> 
> FYI.
> 
>>From my recent experience and observation (on Intel Xeon Phi) 
> wakeup_events_overflow and overflow_poll tests may fail if 
> /proc/sys/kernel/perf_event_max_sample_rate is low enough:
> 
> # cat /proc/sys/kernel/perf_event_max_sample_rate
> 6000
> # abudanko/perf_event_tests/tests/overflow/wakeup_events_overflow
> This tests wakeup event overflows.
> Testing with wakeup_events=1.
> Counts, using mmap buffer 0x7f707b0dc000
> 	POLL_IN : 10
> 	POLL_OUT: 0
> 	POLL_MSG: 0
> 	POLL_ERR: 0
> 	POLL_PRI: 0
> 	POLL_HUP: 0
> 	UNKNOWN : 0
> Testing wakeup events overflow...                            PASSED
> # abudanko/perf_event_tests/tests/overflow/overflow_poll
> This tests using poll() to catch overflow.
> Monitoring pid 131412 status 1407
> Child has stopped due to signal 5 (Trace/breakpoint trap)
> Continuing child
> Returned HUP!
> Counts, using mmap buffer 0x7fb3bfe04000
> 	POLL_IN : 10
> 	POLL_OUT: 0
> 	POLL_MSG: 0
> 	POLL_ERR: 0
> 	POLL_PRI: 0
> 	POLL_HUP: 1
> 	UNKNOWN : 0
> Testing catching overflow with poll()...                     PASSED
> 
> # echo 1000 > /proc/sys/kernel/perf_event_max_sample_rate
> # abudanko/perf_event_tests/tests/overflow/overflow_poll
> This tests using poll() to catch overflow.
> Monitoring pid 131551 status 1407
> Child has stopped due to signal 5 (Trace/breakpoint trap)
> Continuing child
> Returned HUP!
> Counts, using mmap buffer 0x7f80532df000
> 	POLL_IN : 9
> 	POLL_OUT: 0
> 	POLL_MSG: 0
> 	POLL_ERR: 0
> 	POLL_PRI: 0
> 	POLL_HUP: 1
> 	UNKNOWN : 0
> Unexpected POLL_IN interrupt.
> Testing catching overflow with poll()...                     FAILED
> [root@nntpdsd52-210 ~]# abudanko/perf_event_tests/tests/overflow/overflow_poll
> This tests using poll() to catch overflow.
> Monitoring pid 131553 status 1407
> Child has stopped due to signal 5 (Trace/breakpoint trap)
> Continuing child
> Returned HUP!
> Counts, using mmap buffer 0x7f650952c000
> 	POLL_IN : 9
> 	POLL_OUT: 0
> 	POLL_MSG: 0
> 	POLL_ERR: 0
> 	POLL_PRI: 0
> 	POLL_HUP: 1
> 	UNKNOWN : 0
> Unexpected POLL_IN interrupt.
> Testing catching overflow with poll()...                     FAILED

More of the other test:

# echo 1000 > /proc/sys/kernel/perf_event_max_sample_rate
[ ~]# abudanko/perf_event_tests/tests/overflow/wakeup_events_overflow
This tests wakeup event overflows.
Testing with wakeup_events=1.
Counts, using mmap buffer 0x7fe65deff000
	POLL_IN : 4
	POLL_OUT: 0
	POLL_MSG: 0
	POLL_ERR: 0
	POLL_PRI: 0
	POLL_HUP: 0
	UNKNOWN : 0
POLL_IN value 4, expected 10.
Testing wakeup events overflow...                            FAILED
[ ~]# abudanko/perf_event_tests/tests/overflow/wakeup_events_overflow
This tests wakeup event overflows.
Testing with wakeup_events=1.
Counts, using mmap buffer 0x7f818a732000
	POLL_IN : 2
	POLL_OUT: 0
	POLL_MSG: 0
	POLL_ERR: 0
	POLL_PRI: 0
	POLL_HUP: 0
	UNKNOWN : 0
POLL_IN value 2, expected 10.
Testing wakeup events overflow...                            FAILED
[ ~]# abudanko/perf_event_tests/tests/overflow/wakeup_events_overflow
This tests wakeup event overflows.
Testing with wakeup_events=1.
Counts, using mmap buffer 0x7fb3675c5000
	POLL_IN : 4
	POLL_OUT: 0
	POLL_MSG: 0
	POLL_ERR: 0
	POLL_PRI: 0
	POLL_HUP: 0
	UNKNOWN : 0
POLL_IN value 4, expected 10.
Testing wakeup events overflow...                            FAILED

> 
>>
>>> If we're so worried about info leakage, can't we just zero-out the problem 
>>> address (or randomize the kernel address) rather than just pretending the 
>>> interrupt didn't happen?
>>
>> Making up zeroed or randomized data is going to confuse users. I can't
>> imagine that real users are going to want bogus samples that they have
>> to identify (somehow) in order to skip when processing the data.
>>
>> I can see merit in signalling "lost" samples to userspace, so long as
>> they're easily distinguished from real samples.
>>
>> One option is to fake up a sample using the user regs regardless, but
>> that's both fragile and surprising in other cases.
>>
>> Thanks,
>> Mark.
>>
> 
> Thanks,
> Alexey
> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1676914 — Re: [PATCH] perf/core: generate overflow signal when samples are dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region)

On Wed, Jun 28, 2017 at 09:48:27AM -0700, Kyle Huey wrote:
> On Wed, Jun 28, 2017 at 3:56 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > @@ -6101,6 +6116,12 @@ void perf_prepare_sample(struct perf_event_header *header,
> >         struct perf_output_handle handle;
> >         struct perf_event_header header;
> >
> > +       /*
> > +        * For security, drop the skid kernel samples if necessary.
> > +        */
> > +       if (!sample_is_allowed(event, regs))
> > +               return ret;
> 
> Just a bare return here.

Ugh, yes. Sorry about that. I'll fix that up.

[...]

> I can confirm that with that fixed to compile, this patch fixes rr.

Thanks for giving this a go.

Having thought about this some more, I think Vince does make a good
point that throwing away samples is liable to break stuff, e.g. that
which only relies on (non-sensitive) samples.

It still seems wrong to make up data, though.

Maybe for exclude_kernel && !exclude_user events we can always generate
samples from the user regs, rather than the exception regs. That's going
to be closer to what the user wants, regardless. I'll take a look
tomorrow.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1677258 — Re: [PATCH] perf/core: generate overflow signal when samples are dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region)

On Wed, Jun 28, 2017 at 10:49 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jun 28, 2017 at 09:48:27AM -0700, Kyle Huey wrote:
>> On Wed, Jun 28, 2017 at 3:56 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > @@ -6101,6 +6116,12 @@ void perf_prepare_sample(struct perf_event_header *header,
>> >         struct perf_output_handle handle;
>> >         struct perf_event_header header;
>> >
>> > +       /*
>> > +        * For security, drop the skid kernel samples if necessary.
>> > +        */
>> > +       if (!sample_is_allowed(event, regs))
>> > +               return ret;
>>
>> Just a bare return here.
>
> Ugh, yes. Sorry about that. I'll fix that up.
>
> [...]
>
>> I can confirm that with that fixed to compile, this patch fixes rr.
>
> Thanks for giving this a go.
>
> Having thought about this some more, I think Vince does make a good
> point that throwing away samples is liable to break stuff, e.g. that
> which only relies on (non-sensitive) samples.
>
> It still seems wrong to make up data, though.
>
> Maybe for exclude_kernel && !exclude_user events we can always generate
> samples from the user regs, rather than the exception regs. That's going
> to be closer to what the user wants, regardless. I'll take a look
> tomorrow.

I'm not very familiar with the kernel internals, but the reason I
didn't suggest this originally is it seems like it will be difficult
to determine what the "correct" userspace registers are.  For example,
what happens if a performance counter is fixed to a given tid, the
interrupt fires during a context switch from that task to another that
is not being monitored, and the kernel is far enough along in the
context switch that the current task struct has been switched out?
Reporting the new task's registers seems as bad as reporting the
kernel's registers.  But maybe this is easier than I imagine for
whatever reason.

Something to think about.

- Kyle

[toc] | [prev] | [next] | [standalone]

#1677286 — Re: [PATCH] perf/core: generate overflow signal when samples are dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region)

On 6/29/2017 6:55 AM, Kyle Huey wrote:
> On Wed, Jun 28, 2017 at 10:49 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Wed, Jun 28, 2017 at 09:48:27AM -0700, Kyle Huey wrote:
>>> On Wed, Jun 28, 2017 at 3:56 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>> @@ -6101,6 +6116,12 @@ void perf_prepare_sample(struct perf_event_header *header,
>>>>          struct perf_output_handle handle;
>>>>          struct perf_event_header header;
>>>>
>>>> +       /*
>>>> +        * For security, drop the skid kernel samples if necessary.
>>>> +        */
>>>> +       if (!sample_is_allowed(event, regs))
>>>> +               return ret;
>>> Just a bare return here.
>> Ugh, yes. Sorry about that. I'll fix that up.
>>
>> [...]
>>
>>> I can confirm that with that fixed to compile, this patch fixes rr.
>> Thanks for giving this a go.
>>
>> Having thought about this some more, I think Vince does make a good
>> point that throwing away samples is liable to break stuff, e.g. that
>> which only relies on (non-sensitive) samples.
>>
>> It still seems wrong to make up data, though.
>>
>> Maybe for exclude_kernel && !exclude_user events we can always generate
>> samples from the user regs, rather than the exception regs. That's going
>> to be closer to what the user wants, regardless. I'll take a look
>> tomorrow.
> I'm not very familiar with the kernel internals, but the reason I
> didn't suggest this originally is it seems like it will be difficult
> to determine what the "correct" userspace registers are.  For example,
> what happens if a performance counter is fixed to a given tid, the
> interrupt fires during a context switch from that task to another that
> is not being monitored, and the kernel is far enough along in the
> context switch that the current task struct has been switched out?
> Reporting the new task's registers seems as bad as reporting the
> kernel's registers.  But maybe this is easier than I imagine for
> whatever reason.
>
> Something to think about.
>
> - Kyle

Yes, I think so.

The skid interrupt may be triggered at a wrong context and return wrong 
indications (e.g. wrong regs) to userspace.

So that's why I think the *skid* interrupt had better be dropped.

Thanks
Jin Yao

[toc] | [prev] | [next] | [standalone]

#1679037 — Re: [PATCH] perf/core: generate overflow signal when samples are dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region)

On Wed, Jun 28, 2017 at 3:55 PM, Kyle Huey <me@kylehuey.com> wrote:
> On Wed, Jun 28, 2017 at 10:49 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Wed, Jun 28, 2017 at 09:48:27AM -0700, Kyle Huey wrote:
>>> On Wed, Jun 28, 2017 at 3:56 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> > @@ -6101,6 +6116,12 @@ void perf_prepare_sample(struct perf_event_header *header,
>>> >         struct perf_output_handle handle;
>>> >         struct perf_event_header header;
>>> >
>>> > +       /*
>>> > +        * For security, drop the skid kernel samples if necessary.
>>> > +        */
>>> > +       if (!sample_is_allowed(event, regs))
>>> > +               return ret;
>>>
>>> Just a bare return here.
>>
>> Ugh, yes. Sorry about that. I'll fix that up.
>>
>> [...]
>>
>>> I can confirm that with that fixed to compile, this patch fixes rr.
>>
>> Thanks for giving this a go.
>>
>> Having thought about this some more, I think Vince does make a good
>> point that throwing away samples is liable to break stuff, e.g. that
>> which only relies on (non-sensitive) samples.
>>
>> It still seems wrong to make up data, though.
>>
>> Maybe for exclude_kernel && !exclude_user events we can always generate
>> samples from the user regs, rather than the exception regs. That's going
>> to be closer to what the user wants, regardless. I'll take a look
>> tomorrow.
>
> I'm not very familiar with the kernel internals, but the reason I
> didn't suggest this originally is it seems like it will be difficult
> to determine what the "correct" userspace registers are.  For example,
> what happens if a performance counter is fixed to a given tid, the
> interrupt fires during a context switch from that task to another that
> is not being monitored, and the kernel is far enough along in the
> context switch that the current task struct has been switched out?
> Reporting the new task's registers seems as bad as reporting the
> kernel's registers.  But maybe this is easier than I imagine for
> whatever reason.
>
> Something to think about.
>
> - Kyle

We've noticed that the regressing changeset made it into stable
branches that distros are shipping now[0], and we're starting to
receive bug reports from our users.  We would really like to get this
patch or something similar into 4.12 and the next stable releases if
at all possible.

Thanks!

- Kyle

[0] Full list in https://mail.mozilla.org/pipermail/rr-dev/2017-June/000500.html

[toc] | [prev] | [next] | [standalone]

#1677509 — Re: [PATCH] perf/core: generate overflow signal when samples are dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region)

* Mark Rutland <mark.rutland@arm.com> wrote:

> It still seems wrong to make up data, though.

So what we have here is a hardware quirk: we asked for user-space samples, but 
didn't get them and we cannot expose the kernel-internal address.

The question is, how do we handle the hardware quirk. Since we cannot fix the 
hardware on existing systems there's really just two choices:

 - Lose the sample (and signal it as a lost sample)

 - Keep the sample but change the sensitive kernel-internal address to something 
   that is not sensitive: 0 or -1 works, but we could perhaps also return a 
   well-known user-space address such as the vDSO syscall trampoline or such?

there's no other option really.

I'd lean towards Vince's take: losing samples is more surprising than getting the 
occasional sample with some sanitized data in it.

If we make the artificial data still a meaningful user-space address, related to 
kernel entries, then it might even be a bonus, as users would learn to recognize 
it as: 'oh, skid artifact, I know about that'.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1676960 — Re: [PATCH] perf/core: generate overflow signal when samples are dropped (WAS: Re: [REGRESSION] perf/core: PMU interrupts dropped if we entered the kernel in the "skid" region)

On Wed, Jun 28, 2017 at 3:56 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jun 28, 2017 at 11:12:48AM +0100, Mark Rutland wrote:
>> On Tue, Jun 27, 2017 at 09:51:00PM -0700, Kyle Huey wrote:
>
>> As we're trying to avoid smapling state, I think we can move the check
>> into perf_prepare_sample() or __perf_event_output(), where that state is
>> actually sampled. I'll take a look at that momentarily.
>>
>> Just to clarify, you don't care about the sample state at all? i.e. you
>> don't need the user program counter?
>>
>> Is that signal delivered to the tracee, or to a different process that
>> traces it? If the latter, what ensures that the task is stopped
>> sufficiently quickly?
>>
>> > It seems to me that it might be reasonable to ignore the interrupt if
>> > the purpose of the interrupt is to trigger sampling of the CPUs
>> > register state.  But if the interrupt will trigger some other
>> > operation, such as a signal on an fd, then there's no reason to drop
>> > it.
>>
>> Agreed. I'll try to have a patch for this soon.
>>
>> I just need to figure out exactly where that overflow signal is
>> generated by the perf core.
>
> I've figured that out now. That's handled by perf_pending_event(), whcih
> is the irq_work we schedule in __perf_event_overflow().
>
> Does the below patch work for you?
>
> ---->8----
> From bb1f99dace508ce34ab0818f91d59e73450fa142 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland@arm.com>
> Date: Wed, 28 Jun 2017 11:39:25 +0100
> Subject: [PATCH] perf/core: generate overflow signal when samples are dropped
>
> We recently tried to kill an information leak where users could receive
> kernel samples due to skid on the PMU interrupt. To block this, we
> bailed out early in perf_event_overflow(), as we do for non-sampling
> events.
>
> This broke rr, which uses sampling events to receive a signal on
> overflow (but does not care about the contents of the sample). These
> signals are critical to the correct operation of rr.
>
> Instead of bailing out early in perf_event_overflow, we can bail prior
> to performing the actual sampling in __perf_event_output(). This avoids
> the information leak, but preserves the generation of the signal.
>
> Since we don't place any sample data into the ring buffer, the signal is
> arguably spurious. However, a userspace ringbuffer consumer can already
> consume data prior to taking the associated signals, and therefore must
> handle spurious signals to operate correctly. Thus, this signal
> shouldn't be harmful.
>
> Fixes: cc1582c231ea041f ("perf/core: Drop kernel samples even though :u is specified")
> Reported-by: Kyle Huey <me@kylehuey.com>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jin Yao <yao.jin@linux.intel.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/events/core.c | 46 +++++++++++++++++++++++++---------------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 6c4e523..6b263f3 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6090,6 +6090,21 @@ void perf_prepare_sample(struct perf_event_header *header,
>         }
>  }
>
> +static bool sample_is_allowed(struct perf_event *event, struct pt_regs *regs)
> +{
> +       /*
> +        * Due to interrupt latency (AKA "skid"), we may enter the
> +        * kernel before taking an overflow, even if the PMU is only
> +        * counting user events.
> +        * To avoid leaking information to userspace, we must always
> +        * reject kernel samples when exclude_kernel is set.
> +        */
> +       if (event->attr.exclude_kernel && !user_mode(regs))
> +               return false;
> +
> +       return true;
> +}
> +
>  static void __always_inline
>  __perf_event_output(struct perf_event *event,
>                     struct perf_sample_data *data,
> @@ -6101,6 +6116,12 @@ void perf_prepare_sample(struct perf_event_header *header,
>         struct perf_output_handle handle;
>         struct perf_event_header header;
>
> +       /*
> +        * For security, drop the skid kernel samples if necessary.
> +        */
> +       if (!sample_is_allowed(event, regs))
> +               return ret;

Just a bare return here.

> +
>         /* protect the callchain buffers */
>         rcu_read_lock();
>
> @@ -7316,21 +7337,6 @@ int perf_event_account_interrupt(struct perf_event *event)
>         return __perf_event_account_interrupt(event, 1);
>  }
>
> -static bool sample_is_allowed(struct perf_event *event, struct pt_regs *regs)
> -{
> -       /*
> -        * Due to interrupt latency (AKA "skid"), we may enter the
> -        * kernel before taking an overflow, even if the PMU is only
> -        * counting user events.
> -        * To avoid leaking information to userspace, we must always
> -        * reject kernel samples when exclude_kernel is set.
> -        */
> -       if (event->attr.exclude_kernel && !user_mode(regs))
> -               return false;
> -
> -       return true;
> -}
> -
>  /*
>   * Generic event overflow handling, sampling.
>   */
> @@ -7352,12 +7358,6 @@ static int __perf_event_overflow(struct perf_event *event,
>         ret = __perf_event_account_interrupt(event, throttle);
>
>         /*
> -        * For security, drop the skid kernel samples if necessary.
> -        */
> -       if (!sample_is_allowed(event, regs))
> -               return ret;
> -
> -       /*
>          * XXX event_limit might not quite work as expected on inherited
>          * events
>          */
> @@ -7372,6 +7372,10 @@ static int __perf_event_overflow(struct perf_event *event,
>
>         READ_ONCE(event->overflow_handler)(event, data, regs);
>
> +       /*
> +        * We must generate a wakeup regardless of whether we actually
> +        * generated a sample. This is relied upon by rr.
> +        */
>         if (*perf_event_fasync(event) && event->pending_kill) {
>                 event->pending_wakeup = 1;
>                 irq_work_queue(&event->pending);
> --
> 1.9.1
>

I can confirm that with that fixed to compile, this patch fixes rr.

Thanks!

- Kyle

[toc] | [prev] | [next] | [standalone]

#1676915

On Wed, Jun 28, 2017 at 10:19 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> It seems odd that an event without any samples to take has a sample
> period. I'm surprised that there's not *some* sample_type set.

Yes, it is a bit odd :-). Rather than trying to gather some limited
set of information about the target process, we want to interrupt its
execution after a particular number of events have occurred.

The bigger picture here is that we're replaying a previously recorded
process execution. We want to the process to run until it reaches a
particular state that occurred during recording; that state is
identified by a value for the performance counter (retired conditional
branches) plus the values of the general-purpose registers. So our
first step is to run the process until the performance counter reaches
a value less than but "close to" the target value. (We know the
process will be interrupted after a number of extra events have
occurred, so we set the sample period to a value less than the target
by a heuristic "maximum skid size".)

Rob
-- 
lbir ye,ea yer.tnietoehr  rdn rdsme,anea lurpr  edna e hnysnenh hhe uresyf toD
selthor  stor  edna  siewaoeodm  or v sstvr  esBa  kbvted,t rdsme,aoreseoouoto
o l euetiuruewFa  kbn e hnystoivateweh uresyf tulsa rehr  rdm  or rnea lurpr
.a war hsrer holsa rodvted,t  nenh hneireseoouot.tniesiewaoeivatewt sstvr  esn

[toc] | [prev] | [next] | [standalone]

#1676931

On Wed, Jun 28, 2017 at 10:19 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jun 28, 2017 at 09:46:43AM -0700, Kyle Huey wrote:
>> On Wed, Jun 28, 2017 at 3:12 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Tue, Jun 27, 2017 at 09:51:00PM -0700, Kyle Huey wrote:
>> >> My understanding of the situation is as follows:
>> >>
>> >> There is some time, call it t_0, where the hardware counter overflows.
>> >> The PMU triggers an interrupt, but this is not instantaneous.  Call
>> >> the time when the interrupt is actually delivered t_1.  Then t_1 - t_0
>> >> is the "skid".
>> >>
>> >> Note that if the counter is `exclude_kernel`, then at t_0 the CPU
>> >> *must* be running a userspace program.  But by t_1, the CPU may be
>> >> doing something else.  Your patch changed things so that if at t_1 the
>> >> CPU is in the kernel, then the interrupt is discarded.  But rr has
>> >> programmed the counter to deliver a signal on overflow (via F_SETSIG
>> >> on the fd returned by perf_event_open).  This change results in the
>> >> signal never being delivered, because the interrupt was ignored.
>> >> (More accurately, the signal is delivered the *next* time the counter
>> >> overflows, which is far past where we wanted to inject our
>> >> asynchronous event into our tracee.
>> >
>> > Yes, this is a bug.
>> >
>> > As we're trying to avoid smapling state, I think we can move the check
>> > into perf_prepare_sample() or __perf_event_output(), where that state is
>> > actually sampled. I'll take a look at that momentarily.
>> >
>> > Just to clarify, you don't care about the sample state at all? i.e. you
>> > don't need the user program counter?
>>
>> Right. `sample_regs_user`, `sample_star_user`, `branch_sample_type`,
>> etc are all 0.
>> https://github.com/mozilla/rr/blob/cf594dd01f07d96a61409e9f41a29f78c8c51693/src/PerfCounters.cc#L194
>> is what we do use.
>
> Given that, I must be missing something.
>
> In __perf_event_overflow(), we already bail out early if
> !is_sampling_event(event), i.e. when the sample_period is 0.
>
> Your attr has a sample_period of zero, so something must be initialising
> that.
>
> Do you always call PERF_EVENT_IOC_PERIOD, or is something in the core
> fiddling with the sample period behind your back?

We always either set sample_period or call PERF_EVENT_IOC_PERIOD (with
an enormous number if we don't actually want an interrupt.  See
`PerfCounters::reset`, line 446.

> It seems odd that an event without any samples to take has a sample
> period. I'm surprised that there's not *some* sample_type set.

Perhaps sample_period is misleadingly named :)  Alternatively, you
could imagine it as sampling where we're only interested in whether
the counter passed the sampling value or not.

>> > Is that signal delivered to the tracee, or to a different process that
>> > traces it? If the latter, what ensures that the task is stopped
>> > sufficiently quickly?
>>
>> It's delivered to the tracee (via an F_SETOWN_EX with the tracee tid).
>> In practice we've found that on modern Intel hardware that the
>> interrupt and resulting signal delivery delay is bounded by a
>> relatively small number of counter events.
>
> Ok.
>
> Thanks,
> Mark.

- Kyle

[toc] | [prev] | [next] | [standalone]

#1676959

On Wed, Jun 28, 2017 at 10:36:20AM -0700, Kyle Huey wrote:
> On Wed, Jun 28, 2017 at 10:19 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Wed, Jun 28, 2017 at 09:46:43AM -0700, Kyle Huey wrote:
> >> On Wed, Jun 28, 2017 at 3:12 AM, Mark Rutland <mark.rutland@arm.com> wrote:

> >> > Just to clarify, you don't care about the sample state at all? i.e. you
> >> > don't need the user program counter?
> >>
> >> Right. `sample_regs_user`, `sample_star_user`, `branch_sample_type`,
> >> etc are all 0.
> >> https://github.com/mozilla/rr/blob/cf594dd01f07d96a61409e9f41a29f78c8c51693/src/PerfCounters.cc#L194
> >> is what we do use.
> >
> > Given that, I must be missing something.
> >
> > In __perf_event_overflow(), we already bail out early if
> > !is_sampling_event(event), i.e. when the sample_period is 0.
> >
> > Your attr has a sample_period of zero, so something must be initialising
> > that.
> >
> > Do you always call PERF_EVENT_IOC_PERIOD, or is something in the core
> > fiddling with the sample period behind your back?
> 
> We always either set sample_period or call PERF_EVENT_IOC_PERIOD (with
> an enormous number if we don't actually want an interrupt.  See
> `PerfCounters::reset`, line 446.

Ah, thanks for the pointer.

> > It seems odd that an event without any samples to take has a sample
> > period. I'm surprised that there's not *some* sample_type set.
> 
> Perhaps sample_period is misleadingly named :)  Alternatively, you
> could imagine it as sampling where we're only interested in whether
> the counter passed the sampling value or not.

Sure; it's just that I suspect the existing kernel behviour isn't
*quite* intentional, and I could easily see it getting broken in future,
e.g. if someone were to make is_sampling_event() check the attr for
sample types.

So we need to keep an eye on that, regardless.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web