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

Groups > linux.kernel > #1214644 > unrolled thread

[PATCH 1/2] perf/x86/intel/ds: Work around BTS leaking kernel addresses

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

  [PATCH 1/2] perf/x86/intel/ds: Work around BTS leaking kernel addresses Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-08-27 17:20 +0200
    Re: [PATCH 1/2] perf/x86/intel/ds: Work around BTS leaking kernel  addresses Ingo Molnar <mingo@kernel.org> - 2015-08-28 07:40 +0200
      Re: [PATCH 1/2] perf/x86/intel/ds: Work around BTS leaking kernel addresses Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-08-28 08:10 +0200

#1214644 — [PATCH 1/2] perf/x86/intel/ds: Work around BTS leaking kernel addresses

BTS leaks kernel addresses even in userspace-only mode due to imprecise IP
sampling, so sometimes syscall entry points or page fault handler addresses
end up in a userspace trace.

Since this driver uses a relatively small buffer for BTS records and it has
to iterate through them anyway, it can also take on the additional job of
filtering out the records that contain kernel addresses when kernel space
tracing is not enabled.

This patch changes the bts code to skip the offending records from perf
output. In order to request the exact amount of space on the ring buffer,
we need to do an extra pass through the records to know how many there are
of the valid ones, but considering the small size of the buffer, this extra
pass adds very little overhead to the nmi handler. This way we won't end
up with awkward IP samples with zero IPs in the perf stream.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event_intel_ds.c | 40 ++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c b/arch/x86/kernel/cpu/perf_event_intel_ds.c
index 84f236ab96..79175a2e5a 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_ds.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c
@@ -510,10 +510,11 @@ int intel_pmu_drain_bts_buffer(void)
 		u64	flags;
 	};
 	struct perf_event *event = cpuc->events[INTEL_PMC_IDX_FIXED_BTS];
-	struct bts_record *at, *top;
+	struct bts_record *at, *base, *top;
 	struct perf_output_handle handle;
 	struct perf_event_header header;
 	struct perf_sample_data data;
+	unsigned long skip = 0;
 	struct pt_regs regs;
 
 	if (!event)
@@ -522,10 +523,10 @@ int intel_pmu_drain_bts_buffer(void)
 	if (!x86_pmu.bts_active)
 		return 0;
 
-	at  = (struct bts_record *)(unsigned long)ds->bts_buffer_base;
-	top = (struct bts_record *)(unsigned long)ds->bts_index;
+	base = (struct bts_record *)(unsigned long)ds->bts_buffer_base;
+	top  = (struct bts_record *)(unsigned long)ds->bts_index;
 
-	if (top <= at)
+	if (top <= base)
 		return 0;
 
 	memset(&regs, 0, sizeof(regs));
@@ -535,16 +536,43 @@ int intel_pmu_drain_bts_buffer(void)
 	perf_sample_data_init(&data, 0, event->hw.last_period);
 
 	/*
+	 * BTS leaks kernel addresses in branches across the cpl boundary,
+	 * such as traps or system calls, so unless the user is asking for
+	 * kernel tracing (and right now it's not possible), we'd need to
+	 * filter them out. But first we need to count how many of those we
+	 * have in the current batch. This is an extra O(n) pass, however,
+	 * it's much faster than the other one especially considering that
+	 * n <= 2560 (BTS_BUFFER_SIZE / BTS_RECORD_SIZE * 15/16; see the
+	 * alloc_bts_buffer()).
+	 */
+	for (at = base; at < top; at++) {
+		/*
+		 * Note that right now *this* BTS code only works if
+		 * attr::exclude_kernel is set, but let's keep this extra
+		 * check here in case that changes.
+		 */
+		if (event->attr.exclude_kernel &&
+		    (at->from >= PAGE_OFFSET || at->to >= PAGE_OFFSET))
+			skip++;
+	}
+
+	/*
 	 * Prepare a generic sample, i.e. fill in the invariant fields.
 	 * We will overwrite the from and to address before we output
 	 * the sample.
 	 */
 	perf_prepare_sample(&header, &data, event, &regs);
 
-	if (perf_output_begin(&handle, event, header.size * (top - at)))
+	if (perf_output_begin(&handle, event, header.size *
+			      (top - base - skip)))
 		return 1;
 
-	for (; at < top; at++) {
+	for (at = base; at < top; at++) {
+		/* Filter out any records that contain kernel addresses. */
+		if (event->attr.exclude_kernel &&
+		    (at->from >= PAGE_OFFSET || at->to >= PAGE_OFFSET))
+			continue;
+
 		data.ip		= at->from;
 		data.addr	= at->to;
 
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1215100 — Re: [PATCH 1/2] perf/x86/intel/ds: Work around BTS leaking kernel addresses

* Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:

> +	for (at = base; at < top; at++) {
> +		/*
> +		 * Note that right now *this* BTS code only works if
> +		 * attr::exclude_kernel is set, but let's keep this extra
> +		 * check here in case that changes.
> +		 */
> +		if (event->attr.exclude_kernel &&
> +		    (at->from >= PAGE_OFFSET || at->to >= PAGE_OFFSET))
> +			skip++;

Yeah, so that only works on 32-bit kernels, on 64-bit kernels the check for kernel 
addresses is to see whether it's a negative address. PAGE_OFFSET points to above 
any hypervisor's address, so even with your fix we could still leak hypervisor 
addresses.

I.e. use the kernel_ip() primitive instead.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1215107

Ingo Molnar <mingo@kernel.org> writes:

> * Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:
>
>> +	for (at = base; at < top; at++) {
>> +		/*
>> +		 * Note that right now *this* BTS code only works if
>> +		 * attr::exclude_kernel is set, but let's keep this extra
>> +		 * check here in case that changes.
>> +		 */
>> +		if (event->attr.exclude_kernel &&
>> +		    (at->from >= PAGE_OFFSET || at->to >= PAGE_OFFSET))
>> +			skip++;
>
> Yeah, so that only works on 32-bit kernels, on 64-bit kernels the check for kernel 
> addresses is to see whether it's a negative address. PAGE_OFFSET points to above 
> any hypervisor's address, so even with your fix we could still leak hypervisor 
> addresses.
>
> I.e. use the kernel_ip() primitive instead.

That's what I've been looking for, thanks!

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web