linux-kernel - Re: [RFC PATCH perf/core v2 14/16] perf probe: Add group name support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55AC7DA4.80301@hitachi.com>
Date:	Mon, 20 Jul 2015 13:48:36 +0900
From:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To:	Namhyung Kim <namhyung@...nel.org>
CC:	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	linux-kernel@...r.kernel.org,
	Adrian Hunter <adrian.hunter@...el.com>,
	Ingo Molnar <mingo@...hat.com>,
	Paul Mackerras <paulus@...ba.org>,
	Jiri Olsa <jolsa@...nel.org>, Borislav Petkov <bp@...e.de>,
	Hemant Kumar <hemant@...ux.vnet.ibm.com>
Subject: Re: [RFC PATCH perf/core v2 14/16] perf probe: Add group name support

On 2015/07/19 19:16, Namhyung Kim wrote:
> On Wed, Jul 15, 2015 at 06:15:30PM +0900, Masami Hiramatsu wrote:
>> Allow user to set group name for adding new event.
>> Note that this can easily shot yourself in the foot.
>> E.g. Existing group name can conflict with other events.
>> Especially, using the group name reserved for kernel
>> modules can break something when loading/unloading
>> modules.
> 
> Yes, I agree that this can be dangerous.  How about enforcing
> [ku]probes to make the directory of dynamic events safely?

What the safety issue would you afraid?

>  I think
> it'd be better putting all dynamic events in a single directory -
> e.g. $tracefs/events/probe/.  Any events lack group name are created
> in the directory.  Any events have group name create subdirectories as
> group name under the directory.  The perf tools (and others too)
> should be changed to lookup the directory after the usual location.

That will be possible, but includes a big change on event namespace,
e.g. how we'll show the events by perf-list? Even if we can avoid
namespace conflict on tracefs, perf-list event namespace is still
fragile.

> What do you think?

I think there are 2 purposes of probe-event, one is just additional
debug points, another is an extensible event-set. The former will not
any namespace problem, we just add it into new namespace. But latter
requires to be treated as a part of existing (in-kernel) events.
And (userspace)SDT is clearly the latter one.

However, avoiding the conflict of namespace is also important, how
about simply using sdt_<PROVIDER>:<NAME> ?

- Give just a name on a userspace binary
  perf probe -x <BIN> --add <NAME>=<PROBEDEF>
   -> probe_<BIN>:<NAME>
- Give a pair of group and name on a userspace binary
  perf probe -x <BIN> --add <GRP>:<NAME>=<PROBEDEF>
   -> probe_<GRP>:<NAME>
- Set an sdt event on a userspace binary
  perf probe -x <BIN> --add %<PROV>:<NAME>   # or %sdt_<PROV> ?
   -> sdt_<PROV>:<NAME>
- Set an cached event on a userspace binary
  perf probe -x <BIN> --add %<GRP>:<NAME>    # or %probe_<GRP> ?
   -> probe_<GRP>:<NAME>

Thank you,

> 
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
>> ---
>>  tools/perf/util/probe-event.c |   23 ++++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index 262f9d3..c19a380 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -1141,10 +1141,8 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
>>  	bool file_spec = false;
>>  	/*
>>  	 * <Syntax>
>> -	 * perf probe [EVENT=]SRC[:LN|;PTN]
>> -	 * perf probe [EVENT=]FUNC[@SRC][+OFFS|%return|:LN|;PAT]
>> -	 *
>> -	 * TODO:Group name support
>> +	 * perf probe [GRP:][EVENT=]SRC[:LN|;PTN]
> 
> Shouldn't it be
>                       [[GRP:]EVENT=]
> 
> ?
> 
>> +	 * perf probe [GRP:][EVENT=]FUNC[@SRC][+OFFS|%return|:LN|;PAT]
> 
> Ditto.
> 
> Thanks,
> Namhyung
> 
> 
>>  	 */
>>  	if (!arg)
>>  		return -EINVAL;
>> @@ -1153,11 +1151,19 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
>>  	if (ptr && *ptr == '=') {	/* Event name */
>>  		*ptr = '\0';
>>  		tmp = ptr + 1;
>> -		if (strchr(arg, ':')) {
>> -			semantic_error("Group name is not supported yet.\n");
>> -			return -ENOTSUP;
>> -		}
>> +		ptr = strchr(arg, ':');
>> +		if (ptr) {
>> +			*ptr = '\0';
>> +			if (!is_c_func_name(arg))
>> +				goto not_fname;
>> +			pev->group = strdup(arg);
>> +			if (!pev->group)
>> +				return -ENOMEM;
>> +			arg = ptr + 1;
>> +		} else
>> +			pev->group = NULL;
>>  		if (!is_c_func_name(arg)) {
>> +not_fname:
>>  			semantic_error("%s is bad for event name -it must "
>>  				       "follow C symbol-naming rule.\n", arg);
>>  			return -EINVAL;
>> @@ -1165,7 +1171,6 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
>>  		pev->event = strdup(arg);
>>  		if (pev->event == NULL)
>>  			return -ENOMEM;
>> -		pev->group = NULL;
>>  		arg = tmp;
>>  	}
>>  
>>
>>
> 


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@...achi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/