Return EADDRNOTAVAIL when func matches several symbols during kprobe creation

Previously to this commit, if func matches several symbols, a kprobe, being either sysfs or PMU, would only be installed for the first matching address. This could lead to some misunderstanding when some BPF code was never called because it was attached to a function which was indeed not called, because the effectively called one has no kprobes attached.

So, this commit returns EADDRNOTAVAIL when func matches several symbols. This way, user needs to use address to remove the ambiguity.

Suggested-by Masami Hiramatsu <[email protected]>

Signed-off-by Francis Laniel <[email protected]>

Link https://lore.kernel.org/lkml/[email protected]/ open here

---
 kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

kernel/trace/trace_kprobe.c +61 -0

index 23dba01831f7..2f393739e8cf 100644
@@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb = {
 	.priority = 1	/* Invoked after kprobe module callback */
 };
 
+static int count_symbols(void *data, unsigned long unused)
+{
+	unsigned int *count = data;
+
+	(*count)++;
+
+	return 0;
+}
+
+static unsigned int number_of_same_symbols(char *func_name)
+{
+	unsigned int count;
+
+	count = 0;
+	kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
+
+	return count;
+}
+
 static int __trace_kprobe_create(int argc, const char *argv[])
 {
 	/*

@@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 		}
 	}
 
+	if (symbol) {
+		unsigned int count;
+
+		count = number_of_same_symbols(symbol);
+		if (count > 1) {
+			/*
+			 * Users should use ADDR to remove the ambiguity of
+			 * using KSYM only.
+			 */
+			ret = -EADDRNOTAVAIL;
+
+			goto error;
+		} else if (count == 0) {
+			/*
+			 * We can return ENOENT earlier than when register the
+			 * kprobe.
+			 */
+			ret = -ENOENT;
+
+			goto error;
+		}
+	}
+
 	trace_probe_log_set_index(0);
 	if (event) {
 		ret = traceprobe_parse_event_name(&event, &group, gbuf,

@@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct trace_kprobe *tk)
 }
 
 #ifdef CONFIG_PERF_EVENTS
+
 /* create a trace_kprobe, but don't add it to global lists */
 struct trace_event_call *
 create_local_trace_kprobe(char *func, void *addr, unsigned long offs,

@@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
 	int ret;
 	char *event;
 
+	if (func) {
+		unsigned int count;
+
+		count = number_of_same_symbols(func);
+		if (count > 1)
+			/*
+			 * Users should use addr to remove the ambiguity of
+			 * using func only.
+			 */
+			return ERR_PTR(-EADDRNOTAVAIL);
+		else if (count == 0)
+			/*
+			 * We can return ENOENT earlier than when register the
+			 * kprobe.
+			 */
+			return ERR_PTR(-ENOENT);
+	}
+
 	/*
 	 * local trace_kprobes are not added to dyn_event, so they are never
 	 * searched in find_trace_kprobe(). Therefore, there is no concern of

Signature · 1 line

2.34.1

Previously to this commit, if func matches several symbols, a kprobe, being either sysfs or PMU, would only be installed for the first matching address. This could lead to some misunderstanding when some BPF code was never called because it was attached to a function which was indeed not called, because the effectively called one has no kprobes attached.

So, this commit returns EADDRNOTAVAIL when func matches several symbols. This way, user needs to use address to remove the ambiguity.

Suggested-by Masami Hiramatsu <[email protected]>

Signed-off-by Francis Laniel <[email protected]>

Link https://lore.kernel.org/lkml/[email protected]/ open here

---

 kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

kernel/trace/trace_kprobe.c +42 -0

index 23dba01831f7..2f393739e8cf 100644
@@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb = {
 	.priority = 1	/* Invoked after kprobe module callback */
 };
 
+static int count_symbols(void *data, unsigned long unused)
+{
+	unsigned int *count = data;
+
+	(*count)++;
+
+	return 0;
+}
+
+static unsigned int number_of_same_symbols(char *func_name)
+{
+	unsigned int count;
+
+	count = 0;
+	kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
+
+	return count;
+}
+
 static int __trace_kprobe_create(int argc, const char *argv[])
 {
 	/*

@@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 		}
 	}
 
+	if (symbol) {
+		unsigned int count;
+
+		count = number_of_same_symbols(symbol);
+		if (count > 1) {
+			/*
+			 * Users should use ADDR to remove the ambiguity of
+			 * using KSYM only.
+			 */
 
 
 
+			ret = -EADDRNOTAVAIL;
+
+			goto error;
+		} else if (count == 0) {
+			/*
+			 * We can return ENOENT earlier than when register the
+			 * kprobe.
+			 */
+			ret = -ENOENT;
+
+			goto error;
+		}
+	}
+

kernel/trace/trace_kprobe.c +19 -0

 	trace_probe_log_set_index(0);
 	if (event) {
 		ret = traceprobe_parse_event_name(&event, &group, gbuf,

@@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct trace_kprobe *tk)
 }
 
 #ifdef CONFIG_PERF_EVENTS
+
 /* create a trace_kprobe, but don't add it to global lists */
 struct trace_event_call *
 create_local_trace_kprobe(char *func, void *addr, unsigned long offs,

@@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
 	int ret;
 	char *event;
 
+	if (func) {
+		unsigned int count;
+
+		count = number_of_same_symbols(func);
+		if (count > 1)
+			/*
+			 * Users should use addr to remove the ambiguity of
+			 * using func only.
+			 */
+			return ERR_PTR(-EADDRNOTAVAIL);
+		else if (count == 0)
+			/*
+			 * We can return ENOENT earlier than when register the
+			 * kprobe.
+			 */
+			return ERR_PTR(-ENOENT);
+	}
+
 	/*
 	 * local trace_kprobes are not added to dyn_event, so they are never
 	 * searched in find_trace_kprobe(). Therefore, there is no concern of

Signature · 1 line

2.34.1

On Thu, 24 Aug 2023 18:08:59 +0200

Quoted 7 lines · Francis Laniel

Previously to this commit, if func matches several symbols, a kprobe, being
either sysfs or PMU, would only be installed for the first matching address. This could lead to some misunderstanding when some BPF code was never called because it was attached to a function which was indeed not called, because the effectively called one has no kprobes attached.

Quoted 8 lines

So, this commit returns EADDRNOTAVAIL when func matches several symbols. This way, user needs to use address to remove the ambiguity.

Suggested-by Masami Hiramatsu <[email protected]>

Signed-off-by Francis Laniel <[email protected]>

Link:
https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kern
el.org/ ---

Ah, this should be fine, but selftest (tools/testing/selftests/ftrace) fails.

 # tail 60-kprobe_module.tc-log.vsOHnF
...
+ :
+ : 'Add an event on a module function without specifying event name'
+ :
+ echo 'p trace_printk:trace_printk_irq_work'
sh: write error: No such file or directory

Ah, the function on non-exist module should be checked too.

# tail 63-kprobe_syntax_errors.tc-log.mMLwIQ
+ + printfwc '%s' -c
 'p '
+ pos=2
+ printf+  '%s'tr 'p ^non_exist_func'
 -d ^
+ command='p non_exist_func'
+ echo 'Test command: p non_exist_func'
Test command: p non_exist_func
+ echo
+ grep 'trace_kprobe: error:' -A 3 error_log

Also, this doesn't leave a syntax error message.

So, the below changes are needed.

kernel/trace/trace_kprobe.c +3 -1

index 8ab46a2a446d..1e57bc896952 100644
@@ -855,7 +855,7 @@ static int __trace_kprobe_create(int argc, const char
 *argv[]) }
 	}
 
-	if (symbol) {
+	if (symbol && !strchr(symbol, ':')) {
 		unsigned int count;
 
 		count = number_of_same_symbols(symbol);

@@ -864,6 +864,7 @@ static int __trace_kprobe_create(int argc, const char
 *argv[]) * Users should use ADDR to remove the ambiguity of
 			 * using KSYM only.
 			 */
+			trace_probe_log_err(0, NON_UNIQ_SYMBOL);
 			ret = -EADDRNOTAVAIL;
 
 			goto error;

@@ -872,6 +873,7 @@ static int __trace_kprobe_create(int argc, const char
 *argv[]) * We can return ENOENT earlier than when register the
 			 * kprobe.
 			 */
+			trace_probe_log_err(0, BAD_PROBE_ADDR);
 			ret = -ENOENT;
 
 			goto error;

kernel/trace/trace_probe.h +0 -0

index 7f929482e8d4..a4f478448eef 100644
@@ -450,6 +450,7 @@ extern int traceprobe_define_arg_fields(struct

trace_event_call *event_call, C(BAD_MAXACT, "Invalid maxactive
number"), \

kernel/trace/trace_probe.h +0 -0

 	C(NO_TRACEPOINT,	"Tracepoint is not found"),		\
 	C(BAD_ADDR_SUFFIX,	"Invalid probed address suffix"), \

Thank you,

Quoted 112 lines

 kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

kernel/trace/trace_kprobe.c +0 -0

index 23dba01831f7..2f393739e8cf 100644
@@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb =

{>

kernel/trace/trace_kprobe.c +42 -0

 	.priority = 1	/* Invoked after kprobe module callback */
 
 };
 
+static int count_symbols(void *data, unsigned long unused)
+{
+	unsigned int *count = data;
+
+	(*count)++;
+
+	return 0;
+}
+
+static unsigned int number_of_same_symbols(char *func_name)
+{
+	unsigned int count;
+
+	count = 0;
+	kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
+
+	return count;
+}
+
 
 static int __trace_kprobe_create(int argc, const char *argv[])
 {
 
 	/*

@@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char
 *argv[])> 
 		}
 
 	}
 
+	if (symbol) {
+		unsigned int count;
+
+		count = number_of_same_symbols(symbol);
+		if (count > 1) {
+			/*
+			 * Users should use ADDR to remove the ambiguity of
+			 * using KSYM only.
+			 */
 
 
 
+			ret = -EADDRNOTAVAIL;
+
+			goto error;
+		} else if (count == 0) {
+			/*
+			 * We can return ENOENT earlier than when register the
+			 * kprobe.
+			 */
+			ret = -ENOENT;
+
+			goto error;
+		}
+	}
+
 
 	trace_probe_log_set_index(0);
 	if (event) {
 
 		ret = traceprobe_parse_event_name(&event, &group, gbuf,

@@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct

trace_kprobe *tk)>
}

#ifdef CONFIG_PERF_EVENTS

kernel/trace/trace_kprobe.c +1 -0

+
 
 /* create a trace_kprobe, but don't add it to global lists */
 struct trace_event_call *
 create_local_trace_kprobe(char *func, void *addr, unsigned long offs,

@@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr,

unsigned long offs,>

kernel/trace/trace_kprobe.c +18 -0

 	int ret;
 	char *event;
 
+	if (func) {
+		unsigned int count;
+
+		count = number_of_same_symbols(func);
+		if (count > 1)
+			/*
+			 * Users should use addr to remove the ambiguity of
+			 * using func only.
+			 */
+			return ERR_PTR(-EADDRNOTAVAIL);
+		else if (count == 0)
+			/*
+			 * We can return ENOENT earlier than when register the
+			 * kprobe.
+			 */
+			return ERR_PTR(-ENOENT);
+	}
+

kernel/trace/trace_kprobe.c

Quoted 4 lines

kernel/trace/trace_kprobe.c +0 -0

 	/*
 
 	 * local trace_kprobes are not added to dyn_event, so they are never
 	 * searched in find_trace_kprobe(). Therefore, there is no concern of

Hi.

Quoted 44 lines · Le vendredi 25 août 2023, 14:16:49 CEST Masami Hiramatsu a écrit :

On Thu, 24 Aug 2023 18:08:59 +0200

Quoted 7 lines · Francis Laniel

Previously to this commit, if func matches several symbols, a kprobe, being
either sysfs or PMU, would only be installed for the first matching address. This could lead to some misunderstanding when some BPF code was never called because it was attached to a function which was indeed not called, because the effectively called one has no kprobes attached.

Quoted 8 lines

So, this commit returns EADDRNOTAVAIL when func matches several symbols. This way, user needs to use address to remove the ambiguity.

Suggested-by Masami Hiramatsu <[email protected]>

Signed-off-by Francis Laniel <[email protected]>

Link:
https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kern
el.org/ ---

Ah, this should be fine, but selftest (tools/testing/selftests/ftrace) fails.

 # tail 60-kprobe_module.tc-log.vsOHnF
...
+ :
+ : 'Add an event on a module function without specifying event name'
+ :
+ echo 'p trace_printk:trace_printk_irq_work'
sh: write error: No such file or directory

Ah, the function on non-exist module should be checked too.

# tail 63-kprobe_syntax_errors.tc-log.mMLwIQ
+ + printfwc '%s' -c
 'p '
+ pos=2
+ printf+  '%s'tr 'p ^non_exist_func'
 -d ^
+ command='p non_exist_func'
+ echo 'Test command: p non_exist_func'
Test command: p non_exist_func
+ echo
+ grep 'trace_kprobe: error:' -A 3 error_log

Quoted 3 lines

Also, this doesn't leave a syntax error message.

So, the below changes are needed.

Excellent catch! Thank you, I will apply this patch and send v4 right after. Regarding test, do you think I can add a test for the EADDRNOTAVAIL case?

Maybe it should go inside LTP? As this would need having a kernel compiled with a name pointing to several symbols?

Also, should some man pages somewhere be updated to reflect the case kprobe can return EADDRNOTAVAIL?

Quoted 37 lines

kernel/trace/trace_kprobe.c +3 -1

index 8ab46a2a446d..1e57bc896952 100644
@@ -855,7 +855,7 @@ static int __trace_kprobe_create(int argc, const char
 *argv[]) }
 	}
 
-	if (symbol) {
+	if (symbol && !strchr(symbol, ':')) {
 		unsigned int count;
 
 		count = number_of_same_symbols(symbol);

@@ -864,6 +864,7 @@ static int __trace_kprobe_create(int argc, const char
 *argv[]) * Users should use ADDR to remove the ambiguity of
 			 * using KSYM only.
 			 */
+			trace_probe_log_err(0, NON_UNIQ_SYMBOL);
 			ret = -EADDRNOTAVAIL;
 
 			goto error;

@@ -872,6 +873,7 @@ static int __trace_kprobe_create(int argc, const char
 *argv[]) * We can return ENOENT earlier than when register the
 			 * kprobe.
 			 */
+			trace_probe_log_err(0, BAD_PROBE_ADDR);
 			ret = -ENOENT;
 
 			goto error;

kernel/trace/trace_probe.h +0 -0

index 7f929482e8d4..a4f478448eef 100644
@@ -450,6 +450,7 @@ extern int traceprobe_define_arg_fields(struct

trace_event_call *event_call, C(BAD_MAXACT, "Invalid maxactive
number"), \

kernel/trace/trace_probe.h extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,

Quoted 4 lines

kernel/trace/trace_probe.h +1 -0

 	C(MAXACT_TOO_BIG,	"Maxactive is too big"),		\
 	C(BAD_PROBE_ADDR,	"Invalid probed address or symbol"),	\
+	C(NON_UNIQ_SYMBOL,	"The symbol is not unique"),		\
 	C(BAD_RETPROBE,		"Retprobe address must be an function 

entry"), \

Quoted 117 lines

kernel/trace/trace_probe.h +0 -0

 	C(NO_TRACEPOINT,	"Tracepoint is not found"),		\
 	C(BAD_ADDR_SUFFIX,	"Invalid probed address suffix"), \

Thank you,

Quoted 112 lines

 kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

kernel/trace/trace_kprobe.c +0 -0

index 23dba01831f7..2f393739e8cf 100644
@@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb =

{>

kernel/trace/trace_kprobe.c +42 -0

 	.priority = 1	/* Invoked after kprobe module callback */
 
 };
 
+static int count_symbols(void *data, unsigned long unused)
+{
+	unsigned int *count = data;
+
+	(*count)++;
+
+	return 0;
+}
+
+static unsigned int number_of_same_symbols(char *func_name)
+{
+	unsigned int count;
+
+	count = 0;
+	kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
+
+	return count;
+}
+
 
 static int __trace_kprobe_create(int argc, const char *argv[])
 {
 
 	/*

@@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char
 *argv[])> 
 		}
 
 	}
 
+	if (symbol) {
+		unsigned int count;
+
+		count = number_of_same_symbols(symbol);
+		if (count > 1) {
+			/*
+			 * Users should use ADDR to remove the ambiguity of
+			 * using KSYM only.
+			 */
 
 
 
+			ret = -EADDRNOTAVAIL;
+
+			goto error;
+		} else if (count == 0) {
+			/*
+			 * We can return ENOENT earlier than when register the
+			 * kprobe.
+			 */
+			ret = -ENOENT;
+
+			goto error;
+		}
+	}
+
 
 	trace_probe_log_set_index(0);
 	if (event) {
 
 		ret = traceprobe_parse_event_name(&event, &group, gbuf,

@@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct

trace_kprobe *tk)>
}

#ifdef CONFIG_PERF_EVENTS

kernel/trace/trace_kprobe.c +1 -0

+
 
 /* create a trace_kprobe, but don't add it to global lists */
 struct trace_event_call *
 create_local_trace_kprobe(char *func, void *addr, unsigned long offs,

@@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr,

unsigned long offs,>

kernel/trace/trace_kprobe.c +18 -0

 	int ret;
 	char *event;
 
+	if (func) {
+		unsigned int count;
+
+		count = number_of_same_symbols(func);
+		if (count > 1)
+			/*
+			 * Users should use addr to remove the ambiguity of
+			 * using func only.
+			 */
+			return ERR_PTR(-EADDRNOTAVAIL);
+		else if (count == 0)
+			/*
+			 * We can return ENOENT earlier than when register the
+			 * kprobe.
+			 */
+			return ERR_PTR(-ENOENT);
+	}
+

Quoted 4 lines

kernel/trace/trace_kprobe.c

Quoted 4 lines

kernel/trace/trace_kprobe.c +0 -0

 	/*
 
 	 * local trace_kprobes are not added to dyn_event, so they are never
 	 * searched in find_trace_kprobe(). Therefore, there is no concern of

Best regards.

On Fri, 25 Aug 2023 14:34:49 +0200

Quoted 58 lines · Francis Laniel

Hi.

Quoted 55 lines · Le vendredi 25 août 2023, 14:16:49 CEST Masami Hiramatsu a écrit :

On Thu, 24 Aug 2023 18:08:59 +0200

Quoted 13 lines · Francis Laniel

Previously to this commit, if func matches several symbols, a kprobe, being
either sysfs or PMU, would only be installed for the first matching address. This could lead to some misunderstanding when some BPF code was
never called because it was attached to a function which was indeed not
called, because the effectively called one has no kprobes attached.

So, this commit returns EADDRNOTAVAIL when func matches several symbols.
This way, user needs to use address to remove the ambiguity.

Quoted 6 lines

Suggested-by Masami Hiramatsu <[email protected]>

Signed-off-by Francis Laniel <[email protected]>

Link:
https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@k
ern
el.org/ ---

Ah, this should be fine, but selftest (tools/testing/selftests/ftrace) fails.

 # tail 60-kprobe_module.tc-log.vsOHnF

...
+ :
+ : 'Add an event on a module function without specifying event name'
+ :
+ echo 'p trace_printk:trace_printk_irq_work'
sh: write error: No such file or directory

Ah, the function on non-exist module should be checked too.

# tail 63-kprobe_syntax_errors.tc-log.mMLwIQ
+ + printfwc '%s' -c

'p '

+ pos=2
+ printf+ '%s'tr 'p ^non_exist_func'

-d ^

+ command='p non_exist_func'
+ echo 'Test command: p non_exist_func'
Test command: p non_exist_func
+ echo
+ grep 'trace_kprobe: error:' -A 3 error_log

Also, this doesn't leave a syntax error message.

Quoted 5 lines

Quoted 1 line

So, the below changes are needed.

Excellent catch! Thank you, I will apply this patch and send v4 right after. Regarding test, do you think I can add a test for the EADDRNOTAVAIL case?

Hmm, in that case, you need to change something in tracefs/README so that we can identify the kernel has different behavior. Or we have to change this is a "Fix" for backporting.

Quoted 2 lines

Maybe it should go inside LTP? As this would need having a kernel compiled with a name pointing to several symbols?

For this tracing feature, I rather like to use tools/testing/selftests/ftrace to test it. And it is used on all stable kernel, that is why we need to add some change on tracefs/README or something.

But I would like to wait for Alessandro's work. After his work, in this time we need to probe all the same-name symbols as your original patch does. This is because 1:n mapping can happen as Alessandro pointed in

https://lore.kernel.org/all/CAPp5cGQsRdB0+KHR1wX2bDDdc5sTzSNPA417PNJb0ypmV=y
[email protected]/

But if his feature is configurable (and maybe so), we need to keep this version... We have many options.

- this normal kallsyms: the same-name symbols should not be used.
- enhanced kallsyms (if 1:n symbol has the same suffix): the same name symbols should be probed at once.
- enhanced kallsyms (if 1:n symbol has different suffix): the same-name symbol must not exist.

Quoted 2 lines

Also, should some man pages somewhere be updated to reflect the case kprobe can return EADDRNOTAVAIL?

No, it is a tracefs interface and we don't have man pages yet.

Thank you,

Quoted 192 lines

Quoted 51 lines

kernel/trace/trace_kprobe.c +0 -0

index 8ab46a2a446d..1e57bc896952 100644
@@ -855,7 +855,7 @@ static int __trace_kprobe_create(int argc, const

char
*argv[]) }

kernel/trace/trace_kprobe.c +1 -1

 	}
 
-	if (symbol) {
+	if (symbol && !strchr(symbol, ':')) {
 
 		unsigned int count;
 
 		count = number_of_same_symbols(symbol);

@@ -864,6 +864,7 @@ static int __trace_kprobe_create(int argc, const

char
*argv[]) * Users should use ADDR to remove the ambiguity of

kernel/trace/trace_kprobe.c +1 -0

 			 * using KSYM only.
 			 */
 
+			trace_probe_log_err(0, NON_UNIQ_SYMBOL);
 
 			ret = -EADDRNOTAVAIL;
 
 			goto error;

@@ -872,6 +873,7 @@ static int __trace_kprobe_create(int argc, const

char
*argv[]) * We can return ENOENT earlier than when register the

kernel/trace/trace_kprobe.c +1 -0

 			 * kprobe.
 			 */
 
+			trace_probe_log_err(0, BAD_PROBE_ADDR);
 
 			ret = -ENOENT;
 
 			goto error;

kernel/trace/trace_probe.h +0 -0

index 7f929482e8d4..a4f478448eef 100644
@@ -450,6 +450,7 @@ extern int traceprobe_define_arg_fields(struct

trace_event_call *event_call, C(BAD_MAXACT, "Invalid maxactive
number"), \

kernel/trace/trace_probe.h extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,

Quoted 6 lines

kernel/trace/trace_probe.h +1 -0

 	C(MAXACT_TOO_BIG,	"Maxactive is too big"),		\
 	C(BAD_PROBE_ADDR,	"Invalid probed address or symbol"),	\
 
+	C(NON_UNIQ_SYMBOL,	"The symbol is not unique"),		\
 
 	C(BAD_RETPROBE,		"Retprobe address must be an function

entry"), \

Quoted 125 lines

kernel/trace/trace_probe.h +0 -0

 	C(NO_TRACEPOINT,	"Tracepoint is not found"),		\
 	C(BAD_ADDR_SUFFIX,	"Invalid probed address suffix"), \

Thank you,

Quoted 120 lines

 kernel/trace/trace_kprobe.c | 61
 +++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

kernel/trace/trace_kprobe.c +0 -0

index 23dba01831f7..2f393739e8cf 100644
@@ -705,6 +705,25 @@ static struct notifier_block

trace_kprobe_module_nb =
{>

kernel/trace/trace_kprobe.c +19 -0

 	.priority = 1	/* Invoked after kprobe module callback */
 
 };
 
+static int count_symbols(void *data, unsigned long unused)
+{
+	unsigned int *count = data;
+
+	(*count)++;
+
+	return 0;
+}
+
+static unsigned int number_of_same_symbols(char *func_name)
+{
+	unsigned int count;
+
+	count = 0;
+	kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
+
+	return count;
+}
+
 
 static int __trace_kprobe_create(int argc, const char *argv[])
 {
 
 	/*

@@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const

char
*argv[])>

kernel/trace/trace_kprobe.c +23 -0

 		}
 
 	}
 
+	if (symbol) {
+		unsigned int count;
+
+		count = number_of_same_symbols(symbol);
+		if (count > 1) {
+			/*
+			 * Users should use ADDR to remove the ambiguity of
+			 * using KSYM only.
+			 */
 
 
 
+			ret = -EADDRNOTAVAIL;
+
+			goto error;
+		} else if (count == 0) {
+			/*
+			 * We can return ENOENT earlier than when register the
+			 * kprobe.
+			 */
+			ret = -ENOENT;
+
+			goto error;
+		}
+	}
+
 
 	trace_probe_log_set_index(0);
 	if (event) {
 
 		ret = traceprobe_parse_event_name(&event, &group, gbuf,

@@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct

trace_kprobe *tk)>

}

#ifdef CONFIG_PERF_EVENTS

kernel/trace/trace_kprobe.c +1 -0

+
 
 /* create a trace_kprobe, but don't add it to global lists */
 struct trace_event_call *
 create_local_trace_kprobe(char *func, void *addr, unsigned long offs,

@@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void
 *addr,

unsigned long offs,>

kernel/trace/trace_kprobe.c +18 -0

 	int ret;
 	char *event;
 
+	if (func) {
+		unsigned int count;
+
+		count = number_of_same_symbols(func);
+		if (count > 1)
+			/*
+			 * Users should use addr to remove the ambiguity of
+			 * using func only.
+			 */
+			return ERR_PTR(-EADDRNOTAVAIL);
+		else if (count == 0)
+			/*
+			 * We can return ENOENT earlier than when register the
+			 * kprobe.
+			 */
+			return ERR_PTR(-ENOENT);
+	}
+

Quoted 5 lines

kernel/trace/trace_kprobe.c

Quoted 5 lines

kernel/trace/trace_kprobe.c +0 -0

 	/*
 
 	 * local trace_kprobes are not added to dyn_event, so they are never
 	 * searched in find_trace_kprobe(). Therefore, there is no concern
 	 of

Best regards.

Quoted 2 lines

Excellent catch! Thank you, I will apply this patch and send v4 right after. Regarding test, do you think I can add a test for the EADDRNOTAVAIL case?

Hmm, in that case, you need to change something in tracefs/README so that we can identify the kernel has different behavior. Or we have to change this is a "Fix" for backporting.

On Fri, 25 Aug 2023 22:13:21 +0900

Quoted 6 lines · Masami Hiramatsu (Google)

Quoted 3 lines

Excellent catch! Thank you, I will apply this patch and send v4 right after. Regarding test, do you think I can add a test for the EADDRNOTAVAIL case?>

Hmm, in that case, you need to change something in tracefs/README so that we can identify the kernel has different behavior. Or we have to change this is a "Fix" for backporting.

I prefer this to be a Fix and backported.

Thanks,

-- Steve

Hi.

Quoted 6 lines · Le mercredi 30 août 2023, 01:57:19 CEST Steven Rostedt a écrit :

On Fri, 25 Aug 2023 22:13:21 +0900

Quoted 3 lines · Masami Hiramatsu (Google)

Quoted 3 lines

Excellent catch! Thank you, I will apply this patch and send v4 right after. Regarding test, do you think I can add a test for the EADDRNOTAVAIL case?>

Quoted 5 lines

Quoted 3 lines

Hmm, in that case, you need to change something in tracefs/README so that we can identify the kernel has different behavior. Or we have to change this is a "Fix" for backporting.

I prefer this to be a Fix and backported.

This makes sense, I will send v5 to stable mailing list too!

Quoted 4 lines

Thanks,

-- Steve

Best regards.

Hi Francis,

On Thu, 31 Aug 2023 09:14:55 +0200

Quoted 17 lines · Francis Laniel

Hi.

Quoted 14 lines · Le mercredi 30 août 2023, 01:57:19 CEST Steven Rostedt a écrit :

On Fri, 25 Aug 2023 22:13:21 +0900

Quoted 10 lines · Masami Hiramatsu (Google)

Quoted 4 lines

Excellent catch! Thank you, I will apply this patch and send v4 right
after. Regarding test, do you think I can add a test for the EADDRNOTAVAIL case?>

Hmm, in that case, you need to change something in tracefs/README so that
we can identify the kernel has different behavior. Or we have to change
this is a "Fix" for backporting.

Quoted 3 lines

Quoted 1 line

I prefer this to be a Fix and backported.

This makes sense, I will send v5 to stable mailing list too!

I missed this a while. did you send v5 ? I could not find in my mbox.

Thank you,

Quoted 5 lines

Quoted 3 lines

Thanks,

-- Steve

Best regards.