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]
Date:   Wed, 25 May 2022 09:33:57 +0800
From:   Linyu Yuan <quic_linyyuan@...cinc.com>
To:     Steven Rostedt <rostedt@...dmis.org>
CC:     Ingo Molnar <mingo@...hat.com>, <linux-kernel@...r.kernel.org>,
        "Masami Hiramatsu" <mhiramat@...nel.org>,
        Tom Zanussi <zanussi@...nel.org>
Subject: Re: [PATCH 1/2] tracing/probes: auto generate events name when create
 a group of events


On 5/25/2022 6:31 AM, Steven Rostedt wrote:
> [ Adding Masami (kprobe maintainer) and Tom (dynamic event maintainer) ]
>
> On Sun, 1 May 2022 17:34:10 +0800
> Linyu Yuan <quic_linyyuan@...cinc.com> wrote:
>
>> change traceprobe_parse_event_name() and return GRP/EVENT types,
>> TP_ENAME_GROUP_EVENT means user input GRP/EVENT format,
>> TP_ENAME_EVENT means user only input EVENT format,
>> TP_ENAME_GROUP means user only input GRP/ format,
>>
>> it allow no event name input when create a group of events,
>> and will auto generate event name according second argument.
> Sorry, but I can't understand the above. Can you reword it, and perhaps
> include an example of what this patch does.

thanks, will try to update after Masai and Tom comment

>
> After reading the code, I guess you are trying to make it so that you can
> leave out the event name and only have the group name, if you add the slash.
>
>   echo 'p:group/  schedule' > kprobe_events
>
> And that will create a kprobe "schedule" event under the "group" system?

that's right

>
> Also, for some reason, patch 2 of the series never came to me. But it is in
> my LKML folder :-/
>
>> Signed-off-by: Linyu Yuan <quic_linyyuan@...cinc.com>
>> ---
>>   Documentation/trace/kprobetrace.rst  |  8 ++++----
>>   Documentation/trace/uprobetracer.rst |  8 ++++----
>>   kernel/trace/trace_dynevent.c        |  2 +-
>>   kernel/trace/trace_eprobe.c          | 19 +++++++++++--------
>>   kernel/trace/trace_kprobe.c          | 18 +++++++++++-------
>>   kernel/trace/trace_probe.c           |  9 ++++++++-
>>   kernel/trace/trace_probe.h           |  4 ++++
>>   kernel/trace/trace_uprobe.c          | 14 +++++++++-----
>>   8 files changed, 52 insertions(+), 30 deletions(-)
>>
>> diff --git a/Documentation/trace/kprobetrace.rst b/Documentation/trace/kprobetrace.rst
>> index b175d88..4274cc6 100644
>> --- a/Documentation/trace/kprobetrace.rst
>> +++ b/Documentation/trace/kprobetrace.rst
>> @@ -28,10 +28,10 @@ Synopsis of kprobe_events
>>   -------------------------
>>   ::
>>   
>> -  p[:[GRP/]EVENT] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS]	: Set a probe
>> -  r[MAXACTIVE][:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS]	: Set a return probe
>> -  p:[GRP/]EVENT] [MOD:]SYM[+0]%return [FETCHARGS]	: Set a return probe
>> -  -:[GRP/]EVENT						: Clear a probe
>> +  p[:[GRP/][EVENT]] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS]	: Set a probe
>> +  r[MAXACTIVE][:[GRP/][EVENT]] [MOD:]SYM[+0] [FETCHARGS]	: Set a return probe
>> +  p[:[GRP/][EVENT]] [MOD:]SYM[+0]%return [FETCHARGS]	: Set a return probe
>> +  -:[GRP/][EVENT]						: Clear a probe
>>   
>>    GRP		: Group name. If omitted, use "kprobes" for it.
>>    EVENT		: Event name. If omitted, the event name is generated
>> diff --git a/Documentation/trace/uprobetracer.rst b/Documentation/trace/uprobetracer.rst
>> index a8e5938..3a1797d7 100644
>> --- a/Documentation/trace/uprobetracer.rst
>> +++ b/Documentation/trace/uprobetracer.rst
>> @@ -26,10 +26,10 @@ Synopsis of uprobe_tracer
>>   -------------------------
>>   ::
>>   
>> -  p[:[GRP/]EVENT] PATH:OFFSET [FETCHARGS] : Set a uprobe
>> -  r[:[GRP/]EVENT] PATH:OFFSET [FETCHARGS] : Set a return uprobe (uretprobe)
>> -  p[:[GRP/]EVENT] PATH:OFFSET%return [FETCHARGS] : Set a return uprobe (uretprobe)
>> -  -:[GRP/]EVENT                           : Clear uprobe or uretprobe event
>> +  p[:[GRP/][EVENT]] PATH:OFFSET [FETCHARGS] : Set a uprobe
>> +  r[:[GRP/][EVENT]] PATH:OFFSET [FETCHARGS] : Set a return uprobe (uretprobe)
>> +  p[:[GRP/][EVENT]] PATH:OFFSET%return [FETCHARGS] : Set a return uprobe (uretprobe)
>> +  -:[GRP/][EVENT]                           : Clear uprobe or uretprobe event
>>   
>>     GRP           : Group name. If omitted, "uprobes" is the default value.
>>     EVENT         : Event name. If omitted, the event name is generated based
>> diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
>> index e34e8182..033248d 100644
>> --- a/kernel/trace/trace_dynevent.c
>> +++ b/kernel/trace/trace_dynevent.c
>> @@ -101,7 +101,7 @@ int dyn_event_release(const char *raw_command, struct dyn_event_operations *type
>>   		event = p + 1;
>>   		*p = '\0';
>>   	}
>> -	if (event[0] == '\0') {
>> +	if (!system && event[0] == '\0') {
>>   		ret = -EINVAL;
>>   		goto out;
>>   	}
>> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
>> index 7d44785..b16e067 100644
>> --- a/kernel/trace/trace_eprobe.c
>> +++ b/kernel/trace/trace_eprobe.c
>> @@ -125,6 +125,7 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
>>   	 * We match the following:
>>   	 *  event only			- match all eprobes with event name
>>   	 *  system and event only	- match all system/event probes
>> +	 *  system only			- match all system probes
>>   	 *
>>   	 * The below has the above satisfied with more arguments:
>>   	 *
>> @@ -143,7 +144,7 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
>>   		return false;
>>   
>>   	/* Must match the event name */
>> -	if (strcmp(trace_probe_name(&ep->tp), event) != 0)
>> +	if (event[0] != '\0' && strcmp(trace_probe_name(&ep->tp), event) != 0)
>>   		return false;
>>   
>>   	/* No arguments match all */
>> @@ -848,7 +849,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>>   {
>>   	/*
>>   	 * Argument syntax:
>> -	 *      e[:[GRP/]ENAME] SYSTEM.EVENT [FETCHARGS]
>> +	 *      e[:[GRP/][ENAME]] SYSTEM.EVENT [FETCHARGS]
>>   	 * Fetch args:
>>   	 *  <name>=$<field>[:TYPE]
>>   	 */
>> @@ -858,6 +859,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>>   	struct trace_eprobe *ep = NULL;
>>   	char buf1[MAX_EVENT_NAME_LEN];
>>   	char buf2[MAX_EVENT_NAME_LEN];
>> +	char grp_buf[MAX_EVENT_NAME_LEN];
>>   	int ret = 0;
>>   	int i;
>>   
>> @@ -866,14 +868,16 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>>   
>>   	trace_probe_log_init("event_probe", argc, argv);
>>   
>> +	ret = TP_ENAME_EMPTTY;
> EMPTY is spelled wrong.


sorry, will wait comment from  maintainer of V1,  will fix it in V2 if 
V1 is not reject.


>>   	event = strchr(&argv[0][1], ':');
>>   	if (event) {
>>   		event++;
>> -		ret = traceprobe_parse_event_name(&event, &group, buf1,
>> +		ret = traceprobe_parse_event_name(&event, &group, grp_buf,
>>   						  event - argv[0]);
>> -		if (ret)
>> +		if (ret < 0)
>>   			goto parse_error;
>> -	} else {
>> +	}
> add space.

sure, thanks.

>
>> +	if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) {
>>   		strscpy(buf1, argv[1], MAX_EVENT_NAME_LEN);
>>   		sanitize_event_name(buf1);
>>   		event = buf1;
> I'd like to see Masami's and Tom's comments on this one.
>
> -- Steve
>
>> @@ -882,9 +886,8 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>>   		goto parse_error;
>>   
>>   	sys_event = argv[1];
>> -	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2,
>> -					  sys_event - argv[1]);
>> -	if (ret || !sys_name)
>> +	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
>> +	if (ret != TP_ENAME_GROUP_EVENT)
>>   		goto parse_error;
>>   	if (!is_good_name(sys_event) || !is_good_name(sys_name))
>>   		goto parse_error;
>> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
>> index 47cebef..2cd8ef9 100644
>> --- a/kernel/trace/trace_kprobe.c
>> +++ b/kernel/trace/trace_kprobe.c
>> @@ -163,7 +163,8 @@ static bool trace_kprobe_match(const char *system, const char *event,
>>   {
>>   	struct trace_kprobe *tk = to_trace_kprobe(ev);
>>   
>> -	return strcmp(trace_probe_name(&tk->tp), event) == 0 &&
>> +	return (event[0] == '\0' ||
>> +		strcmp(trace_probe_name(&tk->tp), event) == 0) &&
>>   	    (!system || strcmp(trace_probe_group_name(&tk->tp), system) == 0) &&
>>   	    trace_kprobe_match_command_head(tk, argc, argv);
>>   }
>> @@ -708,11 +709,11 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>>   	/*
>>   	 * Argument syntax:
>>   	 *  - Add kprobe:
>> -	 *      p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
>> +	 *      p[:[GRP/][EVENT]] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
>>   	 *  - Add kretprobe:
>> -	 *      r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS]
>> +	 *      r[MAXACTIVE][:[GRP/][EVENT]] [MOD:]KSYM[+0] [FETCHARGS]
>>   	 *    Or
>> -	 *      p:[GRP/]EVENT] [MOD:]KSYM[+0]%return [FETCHARGS]
>> +	 *      p[:[GRP/][EVENT]] [MOD:]KSYM[+0]%return [FETCHARGS]
>>   	 *
>>   	 * Fetch args:
>>   	 *  $retval	: fetch return value
>> @@ -739,6 +740,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>>   	long offset = 0;
>>   	void *addr = NULL;
>>   	char buf[MAX_EVENT_NAME_LEN];
>> +	char grp_buf[MAX_EVENT_NAME_LEN];
>>   	unsigned int flags = TPARG_FL_KERNEL;
>>   
>>   	switch (argv[0][0]) {
>> @@ -832,12 +834,14 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>>   	}
>>   
>>   	trace_probe_log_set_index(0);
>> +	ret = TP_ENAME_EMPTTY;
>>   	if (event) {
>> -		ret = traceprobe_parse_event_name(&event, &group, buf,
>> +		ret = traceprobe_parse_event_name(&event, &group, grp_buf,
>>   						  event - argv[0]);
>> -		if (ret)
>> +		if (ret < 0)
>>   			goto parse_error;
>> -	} else {
>> +	}
>> +	if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) {
>>   		/* Make a new event name */
>>   		if (symbol)
>>   			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
>> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
>> index 80863c6..7fd50ab 100644
>> --- a/kernel/trace/trace_probe.c
>> +++ b/kernel/trace/trace_probe.c
>> @@ -257,6 +257,9 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
>>   	}
>>   	len = strlen(event);
>>   	if (len == 0) {
>> +		if (slash)
>> +			return TP_ENAME_GROUP;
>> +
>>   		trace_probe_log_err(offset, NO_EVENT_NAME);
>>   		return -EINVAL;
>>   	} else if (len > MAX_EVENT_NAME_LEN) {
>> @@ -267,7 +270,11 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
>>   		trace_probe_log_err(offset, BAD_EVENT_NAME);
>>   		return -EINVAL;
>>   	}
>> -	return 0;
>> +
>> +	if (slash)
>> +		return TP_ENAME_GROUP_EVENT;
>> +
>> +	return TP_ENAME_EVENT;
>>   }
>>   
>>   #define PARAM_MAX_STACK (THREAD_SIZE / sizeof(unsigned long))
>> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
>> index 92cc149..d726769 100644
>> --- a/kernel/trace/trace_probe.h
>> +++ b/kernel/trace/trace_probe.h
>> @@ -364,6 +364,10 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg);
>>   extern int traceprobe_split_symbol_offset(char *symbol, long *offset);
>>   int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
>>   				char *buf, int offset);
>> +#define TP_ENAME_GROUP_EVENT	0
>> +#define TP_ENAME_EVENT		1
>> +#define TP_ENAME_GROUP		2
>> +#define TP_ENAME_EMPTTY		3
>>   
>>   enum probe_print_type {
>>   	PROBE_PRINT_NORMAL,
>> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
>> index 9711589..a935c08 100644
>> --- a/kernel/trace/trace_uprobe.c
>> +++ b/kernel/trace/trace_uprobe.c
>> @@ -312,7 +312,8 @@ static bool trace_uprobe_match(const char *system, const char *event,
>>   {
>>   	struct trace_uprobe *tu = to_trace_uprobe(ev);
>>   
>> -	return strcmp(trace_probe_name(&tu->tp), event) == 0 &&
>> +	return (event[0] == '\0' ||
>> +		strcmp(trace_probe_name(&tu->tp), event) == 0) &&
>>   	   (!system || strcmp(trace_probe_group_name(&tu->tp), system) == 0) &&
>>   	   trace_uprobe_match_command_head(tu, argc, argv);
>>   }
>> @@ -532,7 +533,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
>>   
>>   /*
>>    * Argument syntax:
>> - *  - Add uprobe: p|r[:[GRP/]EVENT] PATH:OFFSET[%return][(REF)] [FETCHARGS]
>> + *  - Add uprobe: p|r[:[GRP/][EVENT]] PATH:OFFSET[%return][(REF)] [FETCHARGS]
>>    */
>>   static int __trace_uprobe_create(int argc, const char **argv)
>>   {
>> @@ -540,6 +541,7 @@ static int __trace_uprobe_create(int argc, const char **argv)
>>   	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
>>   	char *arg, *filename, *rctr, *rctr_end, *tmp;
>>   	char buf[MAX_EVENT_NAME_LEN];
>> +	char grp_buf[MAX_EVENT_NAME_LEN];
>>   	enum probe_print_type ptype;
>>   	struct path path;
>>   	unsigned long offset, ref_ctr_offset;
>> @@ -644,12 +646,14 @@ static int __trace_uprobe_create(int argc, const char **argv)
>>   
>>   	/* setup a probe */
>>   	trace_probe_log_set_index(0);
>> +	ret = TP_ENAME_EMPTTY;
>>   	if (event) {
>> -		ret = traceprobe_parse_event_name(&event, &group, buf,
>> +		ret = traceprobe_parse_event_name(&event, &group, grp_buf,
>>   						  event - argv[0]);
>> -		if (ret)
>> +		if (ret < 0)
>>   			goto fail_address_parse;
>> -	} else {
>> +	}
>> +	if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) {
>>   		char *tail;
>>   		char *ptr;
>>   

Powered by blists - more mailing lists