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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 15 Jan 2019 19:58:07 +0000
From:   Song Liu <songliubraving@...com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
CC:     lkml <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>,
        "dsahern@...il.com" <dsahern@...il.com>
Subject: Re: [PATCH v7 perf, bpf-next 6/8] perf util: handle
 PERF_RECORD_KSYMBOL



> On Jan 15, 2019, at 9:56 AM, Arnaldo Carvalho de Melo <acme@...nel.org> wrote:
> 
> Em Thu, Jan 10, 2019 at 04:19:31PM -0800, Song Liu escreveu:
>> This patch handles PERF_RECORD_KSYMBOL in perf record/report.
>> Specifically, map and symbol are created for ksymbol register, and
>> removed for ksymbol unregister.
>> 
>> This patch also set perf_event_attr.ksymbol properly. The flag is
>> ON by default.
>> 
>> Signed-off-by: Song Liu <songliubraving@...com>
>> ---
>> tools/perf/util/event.c   | 21 +++++++++++++++
>> tools/perf/util/event.h   | 20 ++++++++++++++
>> tools/perf/util/evsel.c   |  9 +++++++
>> tools/perf/util/evsel.h   |  1 +
>> tools/perf/util/machine.c | 57 +++++++++++++++++++++++++++++++++++++++
>> tools/perf/util/machine.h |  3 +++
>> tools/perf/util/session.c |  4 +++
>> tools/perf/util/tool.h    |  4 ++-
>> 8 files changed, 118 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
>> index 937a5a4f71cc..3c8a6a8dd260 100644
>> --- a/tools/perf/util/event.c
>> +++ b/tools/perf/util/event.c
>> @@ -24,6 +24,7 @@
>> #include "symbol/kallsyms.h"
>> #include "asm/bug.h"
>> #include "stat.h"
>> +#include "session.h"
>> 
>> #define DEFAULT_PROC_MAP_PARSE_TIMEOUT 500
>> 
>> @@ -45,6 +46,7 @@ static const char *perf_event__names[] = {
>> 	[PERF_RECORD_SWITCH]			= "SWITCH",
>> 	[PERF_RECORD_SWITCH_CPU_WIDE]		= "SWITCH_CPU_WIDE",
>> 	[PERF_RECORD_NAMESPACES]		= "NAMESPACES",
>> +	[PERF_RECORD_KSYMBOL]			= "KSYMBOL",
>> 	[PERF_RECORD_HEADER_ATTR]		= "ATTR",
>> 	[PERF_RECORD_HEADER_EVENT_TYPE]		= "EVENT_TYPE",
>> 	[PERF_RECORD_HEADER_TRACING_DATA]	= "TRACING_DATA",
>> @@ -1329,6 +1331,14 @@ int perf_event__process_switch(struct perf_tool *tool __maybe_unused,
>> 	return machine__process_switch_event(machine, event);
>> }
>> 
>> +int perf_event__process_ksymbol(struct perf_tool *tool __maybe_unused,
>> +				union perf_event *event,
>> +				struct perf_sample *sample __maybe_unused,
>> +				struct machine *machine)
>> +{
>> +	return machine__process_ksymbol(machine, event, sample);
>> +}
>> +
>> size_t perf_event__fprintf_mmap(union perf_event *event, FILE *fp)
>> {
>> 	return fprintf(fp, " %d/%d: [%#" PRIx64 "(%#" PRIx64 ") @ %#" PRIx64 "]: %c %s\n",
>> @@ -1461,6 +1471,14 @@ static size_t perf_event__fprintf_lost(union perf_event *event, FILE *fp)
>> 	return fprintf(fp, " lost %" PRIu64 "\n", event->lost.lost);
>> }
>> 
>> +size_t perf_event__fprintf_ksymbol(union perf_event *event, FILE *fp)
>> +{
>> +	return fprintf(fp, " ksymbol event with addr %lx len %u type %u flags 0x%x name %s\n",
>> +		       event->ksymbol_event.addr, event->ksymbol_event.len,
>> +		       event->ksymbol_event.ksym_type,
>> +		       event->ksymbol_event.flags, event->ksymbol_event.name);
>> +}
>> +
>> size_t perf_event__fprintf(union perf_event *event, FILE *fp)
>> {
>> 	size_t ret = fprintf(fp, "PERF_RECORD_%s",
>> @@ -1496,6 +1514,9 @@ size_t perf_event__fprintf(union perf_event *event, FILE *fp)
>> 	case PERF_RECORD_LOST:
>> 		ret += perf_event__fprintf_lost(event, fp);
>> 		break;
>> +	case PERF_RECORD_KSYMBOL:
>> +		ret += perf_event__fprintf_ksymbol(event, fp);
>> +		break;
>> 	default:
>> 		ret += fprintf(fp, "\n");
>> 	}
>> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
>> index eb95f3384958..018322f2a13e 100644
>> --- a/tools/perf/util/event.h
>> +++ b/tools/perf/util/event.h
>> @@ -5,6 +5,7 @@
>> #include <limits.h>
>> #include <stdio.h>
>> #include <linux/kernel.h>
>> +#include <linux/bpf.h>
>> 
>> #include "../perf.h"
>> #include "build-id.h"
>> @@ -84,6 +85,19 @@ struct throttle_event {
>> 	u64 stream_id;
>> };
>> 
>> +#ifndef KSYM_NAME_LEN
>> +#define KSYM_NAME_LEN 256
>> +#endif
>> +
>> +struct ksymbol_event {
>> +	struct perf_event_header header;
>> +	u64 addr;
>> +	u32 len;
>> +	u16 ksym_type;
>> +	u16 flags;
>> +	char name[KSYM_NAME_LEN];
>> +};
>> +
>> #define PERF_SAMPLE_MASK				\
>> 	(PERF_SAMPLE_IP | PERF_SAMPLE_TID |		\
>> 	 PERF_SAMPLE_TIME | PERF_SAMPLE_ADDR |		\
>> @@ -651,6 +665,7 @@ union perf_event {
>> 	struct stat_round_event		stat_round;
>> 	struct time_conv_event		time_conv;
>> 	struct feature_event		feat;
>> +	struct ksymbol_event		ksymbol_event;
>> };
>> 
>> void perf_event__print_totals(void);
>> @@ -748,6 +763,10 @@ int perf_event__process_exit(struct perf_tool *tool,
>> 			     union perf_event *event,
>> 			     struct perf_sample *sample,
>> 			     struct machine *machine);
>> +int perf_event__process_ksymbol(struct perf_tool *tool,
>> +				union perf_event *event,
>> +				struct perf_sample *sample,
>> +				struct machine *machine);
>> int perf_tool__process_synth_event(struct perf_tool *tool,
>> 				   union perf_event *event,
>> 				   struct machine *machine,
>> @@ -811,6 +830,7 @@ size_t perf_event__fprintf_switch(union perf_event *event, FILE *fp);
>> size_t perf_event__fprintf_thread_map(union perf_event *event, FILE *fp);
>> size_t perf_event__fprintf_cpu_map(union perf_event *event, FILE *fp);
>> size_t perf_event__fprintf_namespaces(union perf_event *event, FILE *fp);
>> +size_t perf_event__fprintf_ksymbol(union perf_event *event, FILE *fp);
>> size_t perf_event__fprintf(union perf_event *event, FILE *fp);
>> 
>> int kallsyms__get_function_start(const char *kallsyms_filename,
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index dbc0466db368..de34ce875648 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -1035,6 +1035,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
>> 	attr->mmap  = track;
>> 	attr->mmap2 = track && !perf_missing_features.mmap2;
>> 	attr->comm  = track;
>> +	attr->ksymbol = track && !perf_missing_features.ksymbol;
>> 
>> 	if (opts->record_namespaces)
>> 		attr->namespaces  = track;
>> @@ -1652,6 +1653,7 @@ int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
>> 	PRINT_ATTRf(context_switch, p_unsigned);
>> 	PRINT_ATTRf(write_backward, p_unsigned);
>> 	PRINT_ATTRf(namespaces, p_unsigned);
>> +	PRINT_ATTRf(ksymbol, p_unsigned);
>> 
>> 	PRINT_ATTRn("{ wakeup_events, wakeup_watermark }", wakeup_events, p_unsigned);
>> 	PRINT_ATTRf(bp_type, p_unsigned);
>> @@ -1811,6 +1813,8 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>> 				     PERF_SAMPLE_BRANCH_NO_CYCLES);
>> 	if (perf_missing_features.group_read && evsel->attr.inherit)
>> 		evsel->attr.read_format &= ~(PERF_FORMAT_GROUP|PERF_FORMAT_ID);
>> +	if (perf_missing_features.ksymbol)
>> +		evsel->attr.ksymbol = 0;
>> retry_sample_id:
>> 	if (perf_missing_features.sample_id_all)
>> 		evsel->attr.sample_id_all = 0;
>> @@ -1955,6 +1959,11 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>> 		perf_missing_features.exclude_guest = true;
>> 		pr_debug2("switching off exclude_guest, exclude_host\n");
>> 		goto fallback_missing_features;
>> +	} else if (!perf_missing_features.ksymbol &&
>> +		   evsel->attr.ksymbol) {
>> +		perf_missing_features.ksymbol = true;
>> +		pr_debug2("switching off ksymbol\n");
>> +		goto fallback_missing_features;
> 
> 
> Please move this to the top, see the comment there:
> 
> 	if (err != -EINVAL || cpu > 0 || thread > 0)
> 		goto out_close;
> 
> 	/*
> 	 * Must probe features in the order they were added to the
> 	 * perf_event_attr interface.
> 	 */
> 	if (!perf_missing_features.write_backward && evsel->attr.write_backward) {
> 		perf_missing_features.write_backward = true;
> 		pr_debug2("switching off write_backward\n");
> 		goto out_close;
> 	} else if (!perf_missing_features.clockid_wrong && evsel->attr.use_clockid) {
> 		perf_missing_features.clockid_wrong = true;
> 		pr_debug2("switching off clockid\n");
> 		goto fallback_missing_features;
> 
> 
> So that when one asks for, say, attr.write_backward _and_ attr.ksymbol,
> we first disable attr.ksymbol, to try the fallback, as it was added
> after attr.write_backward.
> 
> Think about an older kernel where attr.write_backward is present but not
> attr.ksymbol.
> 
> Ditto for the attr.bpf_event.
> 

Thanks Arnaldo!

I will include the fix in next version.

Song

Powered by blists - more mailing lists