[PATCH for-next] tracing/kprobes: Add symbol counting check when module loads

[PATCH for-next] tracing/kprobes: Add symbol counting check when module loads

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Check the number of probe target symbols in the target module when
the module is loaded. If the probe is not on the unique name symbols
in the module, it will be rejected at that point.

Note that the symbol which has a unique name in the target module,
it will be accepted even if there are same-name symbols in the
kernel or other modules,

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_kprobe.c |  112 ++++++++++++++++++++++++++-----------------
 1 file changed, 68 insertions(+), 44 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index e834f149695b..90cf2219adb4 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -670,6 +670,21 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
 	return ret;
 }
 
+static int validate_module_probe_symbol(const char *modname, const char *symbol);
+
+static int register_module_trace_kprobe(struct module *mod, struct trace_kprobe *tk)
+{
+	const char *p;
+	int ret = 0;
+
+	p = strchr(trace_kprobe_symbol(tk), ':');
+	if (p)
+		ret = validate_module_probe_symbol(module_name(mod), p++);
+	if (!ret)
+		ret = register_trace_kprobe(tk);
+	return ret;
+}
+
 /* Module notifier call back, checking event on the module */
 static int trace_kprobe_module_callback(struct notifier_block *nb,
 				       unsigned long val, void *data)
@@ -688,7 +703,7 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
 		if (trace_kprobe_within_module(tk, mod)) {
 			/* Don't need to check busy - this should have gone. */
 			__unregister_trace_kprobe(tk);
-			ret = __register_trace_kprobe(tk);
+			ret = register_module_trace_kprobe(mod, tk);
 			if (ret)
 				pr_warn("Failed to re-register probe %s on %s: %d\n",
 					trace_probe_name(&tk->tp),
@@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const char *name, unsigned long unused)
 	return 0;
 }
 
-static unsigned int number_of_same_symbols(char *func_name)
+static unsigned int number_of_same_symbols(const char *mod, const char *func_name)
 {
 	struct sym_count_ctx ctx = { .count = 0, .name = func_name };
 
-	kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count);
+	if (!mod)
+		kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count);
 
-	module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx);
+	module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx);
 
 	return ctx.count;
 }
 
+static int validate_module_probe_symbol(const char *modname, const char *symbol)
+{
+	unsigned int count = number_of_same_symbols(modname, symbol);
+
+	if (count > 1) {
+		/*
+		 * Users should use ADDR to remove the ambiguity of
+		 * using KSYM only.
+		 */
+		return -EADDRNOTAVAIL;
+	} else if (count == 0) {
+		/*
+		 * We can return ENOENT earlier than when register the
+		 * kprobe.
+		 */
+		return -ENOENT;
+	}
+	return 0;
+}
+
+static int validate_probe_symbol(char *symbol)
+{
+	char *mod = NULL, *p;
+	int ret;
+
+	p = strchr(symbol, ':');
+	if (p) {
+		mod = symbol;
+		symbol = p + 1;
+		*p = '\0';
+	}
+	ret = validate_module_probe_symbol(mod, symbol);
+	if (p)
+		*p = ':';
+	return ret;
+}
+
 static int __trace_kprobe_create(int argc, const char *argv[])
 {
 	/*
@@ -859,6 +912,14 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 			trace_probe_log_err(0, BAD_PROBE_ADDR);
 			goto parse_error;
 		}
+		ret = validate_probe_symbol(symbol);
+		if (ret) {
+			if (ret == -EADDRNOTAVAIL)
+				trace_probe_log_err(0, NON_UNIQ_SYMBOL);
+			else
+				trace_probe_log_err(0, BAD_PROBE_ADDR);
+			goto parse_error;
+		}
 		if (is_return)
 			ctx.flags |= TPARG_FL_RETURN;
 		ret = kprobe_on_func_entry(NULL, symbol, offset);
@@ -871,31 +932,6 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 		}
 	}
 
-	if (symbol && !strchr(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.
-			 */
-			trace_probe_log_err(0, NON_UNIQ_SYMBOL);
-			ret = -EADDRNOTAVAIL;
-
-			goto error;
-		} else if (count == 0) {
-			/*
-			 * We can return ENOENT earlier than when register the
-			 * kprobe.
-			 */
-			trace_probe_log_err(0, BAD_PROBE_ADDR);
-			ret = -ENOENT;
-
-			goto error;
-		}
-	}
-
 	trace_probe_log_set_index(0);
 	if (event) {
 		ret = traceprobe_parse_event_name(&event, &group, gbuf,
@@ -1767,21 +1803,9 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
 	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);
+		ret = validate_probe_symbol(func);
+		if (ret)
+			return ERR_PTR(ret);
 	}
 
 	/*

Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads

[Steven Rostedt]

On Sun, 29 Oct 2023 12:10:46 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Check the number of probe target symbols in the target module when
> the module is loaded. If the probe is not on the unique name symbols
> in the module, it will be rejected at that point.
> 
> Note that the symbol which has a unique name in the target module,
> it will be accepted even if there are same-name symbols in the
> kernel or other modules,
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_kprobe.c |  112 ++++++++++++++++++++++++++-----------------
>  1 file changed, 68 insertions(+), 44 deletions(-)

Reviewed-by: Steven Rosted (Google) <rostedt@goodmis.org>

-- Steve

Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads

[Andrii Nakryiko]

On Sat, Oct 28, 2023 at 8:10 PM Masami Hiramatsu (Google)
<mhiramat@kernel.org> wrote:
>
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Check the number of probe target symbols in the target module when
> the module is loaded. If the probe is not on the unique name symbols
> in the module, it will be rejected at that point.
>
> Note that the symbol which has a unique name in the target module,
> it will be accepted even if there are same-name symbols in the
> kernel or other modules,
>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_kprobe.c |  112 ++++++++++++++++++++++++++-----------------
>  1 file changed, 68 insertions(+), 44 deletions(-)
>

LGTM.

Acked-by: Andrii Nakryiko <andrii@kernel.org>


> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index e834f149695b..90cf2219adb4 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -670,6 +670,21 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
>         return ret;
>  }
>
> +static int validate_module_probe_symbol(const char *modname, const char *symbol);
> +
> +static int register_module_trace_kprobe(struct module *mod, struct trace_kprobe *tk)
> +{
> +       const char *p;
> +       int ret = 0;
> +
> +       p = strchr(trace_kprobe_symbol(tk), ':');
> +       if (p)
> +               ret = validate_module_probe_symbol(module_name(mod), p++);
> +       if (!ret)
> +               ret = register_trace_kprobe(tk);
> +       return ret;
> +}
> +
>  /* Module notifier call back, checking event on the module */
>  static int trace_kprobe_module_callback(struct notifier_block *nb,
>                                        unsigned long val, void *data)
> @@ -688,7 +703,7 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
>                 if (trace_kprobe_within_module(tk, mod)) {
>                         /* Don't need to check busy - this should have gone. */
>                         __unregister_trace_kprobe(tk);
> -                       ret = __register_trace_kprobe(tk);
> +                       ret = register_module_trace_kprobe(mod, tk);
>                         if (ret)
>                                 pr_warn("Failed to re-register probe %s on %s: %d\n",
>                                         trace_probe_name(&tk->tp),
> @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const char *name, unsigned long unused)
>         return 0;
>  }
>
> -static unsigned int number_of_same_symbols(char *func_name)
> +static unsigned int number_of_same_symbols(const char *mod, const char *func_name)
>  {
>         struct sym_count_ctx ctx = { .count = 0, .name = func_name };
>
> -       kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count);
> +       if (!mod)
> +               kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count);
>
> -       module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx);
> +       module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx);
>
>         return ctx.count;
>  }
>
> +static int validate_module_probe_symbol(const char *modname, const char *symbol)
> +{
> +       unsigned int count = number_of_same_symbols(modname, symbol);
> +
> +       if (count > 1) {
> +               /*
> +                * Users should use ADDR to remove the ambiguity of
> +                * using KSYM only.
> +                */
> +               return -EADDRNOTAVAIL;
> +       } else if (count == 0) {
> +               /*
> +                * We can return ENOENT earlier than when register the
> +                * kprobe.
> +                */
> +               return -ENOENT;
> +       }
> +       return 0;
> +}
> +
> +static int validate_probe_symbol(char *symbol)
> +{
> +       char *mod = NULL, *p;
> +       int ret;
> +
> +       p = strchr(symbol, ':');
> +       if (p) {
> +               mod = symbol;
> +               symbol = p + 1;
> +               *p = '\0';
> +       }
> +       ret = validate_module_probe_symbol(mod, symbol);
> +       if (p)
> +               *p = ':';
> +       return ret;
> +}
> +
>  static int __trace_kprobe_create(int argc, const char *argv[])
>  {
>         /*
> @@ -859,6 +912,14 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>                         trace_probe_log_err(0, BAD_PROBE_ADDR);
>                         goto parse_error;
>                 }
> +               ret = validate_probe_symbol(symbol);
> +               if (ret) {
> +                       if (ret == -EADDRNOTAVAIL)
> +                               trace_probe_log_err(0, NON_UNIQ_SYMBOL);
> +                       else
> +                               trace_probe_log_err(0, BAD_PROBE_ADDR);
> +                       goto parse_error;
> +               }
>                 if (is_return)
>                         ctx.flags |= TPARG_FL_RETURN;
>                 ret = kprobe_on_func_entry(NULL, symbol, offset);
> @@ -871,31 +932,6 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>                 }
>         }
>
> -       if (symbol && !strchr(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.
> -                        */
> -                       trace_probe_log_err(0, NON_UNIQ_SYMBOL);
> -                       ret = -EADDRNOTAVAIL;
> -
> -                       goto error;
> -               } else if (count == 0) {
> -                       /*
> -                        * We can return ENOENT earlier than when register the
> -                        * kprobe.
> -                        */
> -                       trace_probe_log_err(0, BAD_PROBE_ADDR);
> -                       ret = -ENOENT;
> -
> -                       goto error;
> -               }
> -       }
> -
>         trace_probe_log_set_index(0);
>         if (event) {
>                 ret = traceprobe_parse_event_name(&event, &group, gbuf,
> @@ -1767,21 +1803,9 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
>         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);
> +               ret = validate_probe_symbol(func);
> +               if (ret)
> +                       return ERR_PTR(ret);
>         }
>
>         /*
>

Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads

[Francis Laniel]

Hi!


Le dimanche 29 octobre 2023, 05:10:46 EET Masami Hiramatsu (Google) a écrit :
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Check the number of probe target symbols in the target module when
> the module is loaded. If the probe is not on the unique name symbols
> in the module, it will be rejected at that point.
> 
> Note that the symbol which has a unique name in the target module,
> it will be accepted even if there are same-name symbols in the
> kernel or other modules,

I am wondering about symbols which are only part of one specific module.
I wrote a comment about it below, please let me know what you think about it.

> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_kprobe.c |  112
> ++++++++++++++++++++++++++----------------- 1 file changed, 68
> insertions(+), 44 deletions(-)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index e834f149695b..90cf2219adb4 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -670,6 +670,21 @@ static int register_trace_kprobe(struct trace_kprobe
> *tk) return ret;
>  }
> 
> +static int validate_module_probe_symbol(const char *modname, const char
> *symbol); +
> +static int register_module_trace_kprobe(struct module *mod, struct
> trace_kprobe *tk) +{
> +	const char *p;
> +	int ret = 0;
> +
> +	p = strchr(trace_kprobe_symbol(tk), ':');
> +	if (p)
> +		ret = validate_module_probe_symbol(module_name(mod), p++);
> +	if (!ret)
> +		ret = register_trace_kprobe(tk);
> +	return ret;
> +}
> +
>  /* Module notifier call back, checking event on the module */
>  static int trace_kprobe_module_callback(struct notifier_block *nb,
>  				       unsigned long val, void *data)
> @@ -688,7 +703,7 @@ static int trace_kprobe_module_callback(struct
> notifier_block *nb, if (trace_kprobe_within_module(tk, mod)) {
>  			/* Don't need to check busy - this should have gone. */
>  			__unregister_trace_kprobe(tk);
> -			ret = __register_trace_kprobe(tk);
> +			ret = register_module_trace_kprobe(mod, tk);
>  			if (ret)
>  				pr_warn("Failed to re-register probe %s on %s: 
%d\n",
>  					trace_probe_name(&tk->tp),
> @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const char
> *name, unsigned long unused) return 0;
>  }
> 
> -static unsigned int number_of_same_symbols(char *func_name)
> +static unsigned int number_of_same_symbols(const char *mod, const char
> *func_name) {
>  	struct sym_count_ctx ctx = { .count = 0, .name = func_name };
> 
> -	kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count);
> +	if (!mod)
> +		kallsyms_on_each_match_symbol(count_symbols, func_name, 
&ctx.count);
> 
> -	module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx);
> +	module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx);

I may be missing something here or reviewing too quickly.
Wouldn't this function return count to be 0 if func_name is only part of the 
module named mod?
Indeed, if the function is not in kernel symbol, 
kallsyms_on_each_match_symbol() will not loop.
And, by giving mod to module_kallsyms_on_each_symbol(), the corresponding 
module will be skipped, so count_mob_symbols() would not be called.
Hence, we would have 0 as count, which would lead to ENOENT later.

>  	return ctx.count;
>  }
> 
> +static int validate_module_probe_symbol(const char *modname, const char
> *symbol) +{
> +	unsigned int count = number_of_same_symbols(modname, symbol);
> +
> +	if (count > 1) {
> +		/*
> +		 * Users should use ADDR to remove the ambiguity of
> +		 * using KSYM only.
> +		 */
> +		return -EADDRNOTAVAIL;
> +	} else if (count == 0) {
> +		/*
> +		 * We can return ENOENT earlier than when register the
> +		 * kprobe.
> +		 */
> +		return -ENOENT;
> +	}
> +	return 0;
> +}
> +
> +static int validate_probe_symbol(char *symbol)
> +{
> +	char *mod = NULL, *p;
> +	int ret;
> +
> +	p = strchr(symbol, ':');
> +	if (p) {
> +		mod = symbol;
> +		symbol = p + 1;
> +		*p = '\0';
> +	}
> +	ret = validate_module_probe_symbol(mod, symbol);
> +	if (p)
> +		*p = ':';
> +	return ret;
> +}
> +
>  static int __trace_kprobe_create(int argc, const char *argv[])
>  {
>  	/*
> @@ -859,6 +912,14 @@ static int __trace_kprobe_create(int argc, const char
> *argv[]) trace_probe_log_err(0, BAD_PROBE_ADDR);
>  			goto parse_error;
>  		}
> +		ret = validate_probe_symbol(symbol);
> +		if (ret) {
> +			if (ret == -EADDRNOTAVAIL)
> +				trace_probe_log_err(0, NON_UNIQ_SYMBOL);
> +			else
> +				trace_probe_log_err(0, BAD_PROBE_ADDR);
> +			goto parse_error;
> +		}
>  		if (is_return)
>  			ctx.flags |= TPARG_FL_RETURN;
>  		ret = kprobe_on_func_entry(NULL, symbol, offset);
> @@ -871,31 +932,6 @@ static int __trace_kprobe_create(int argc, const char
> *argv[]) }
>  	}
> 
> -	if (symbol && !strchr(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.
> -			 */
> -			trace_probe_log_err(0, NON_UNIQ_SYMBOL);
> -			ret = -EADDRNOTAVAIL;
> -
> -			goto error;
> -		} else if (count == 0) {
> -			/*
> -			 * We can return ENOENT earlier than when register the
> -			 * kprobe.
> -			 */
> -			trace_probe_log_err(0, BAD_PROBE_ADDR);
> -			ret = -ENOENT;
> -
> -			goto error;
> -		}
> -	}
> -
>  	trace_probe_log_set_index(0);
>  	if (event) {
>  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
> @@ -1767,21 +1803,9 @@ create_local_trace_kprobe(char *func, void *addr,
> unsigned long offs, 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);
> +		ret = validate_probe_symbol(func);
> +		if (ret)
> +			return ERR_PTR(ret);
>  	}
> 
>  	/*

Best regards.

Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads

[Masami Hiramatsu]

Hi,

On Tue, 31 Oct 2023 23:24:43 +0200
Francis Laniel <flaniel@linux.microsoft.com> wrote:

> > @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const char
> > *name, unsigned long unused) return 0;
> >  }
> > 
> > -static unsigned int number_of_same_symbols(char *func_name)
> > +static unsigned int number_of_same_symbols(const char *mod, const char
> > *func_name) {
> >  	struct sym_count_ctx ctx = { .count = 0, .name = func_name };
> > 
> > -	kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count);
> > +	if (!mod)
> > +		kallsyms_on_each_match_symbol(count_symbols, func_name, 
> &ctx.count);
> > 
> > -	module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx);
> > +	module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx);
> 
> I may be missing something here or reviewing too quickly.
> Wouldn't this function return count to be 0 if func_name is only part of the 
> module named mod?

No, please read below.

> Indeed, if the function is not in kernel symbol, 
> kallsyms_on_each_match_symbol() will not loop.
> And, by giving mod to module_kallsyms_on_each_symbol(), the corresponding 
> module will be skipped, so count_mob_symbols() would not be called.
> Hence, we would have 0 as count, which would lead to ENOENT later.

Would you mean the case func_name is on the specific module?
If 'mod' is specified, module_kallsyms_on_each_symbol() only loops on
symbols in the module names 'mod'.

int module_kallsyms_on_each_symbol(const char *modname,
                                   int (*fn)(void *, const char *, unsigned long),
                                   void *data)
{
        struct module *mod;
        unsigned int i;
        int ret = 0;

        mutex_lock(&module_mutex);
        list_for_each_entry(mod, &modules, list) {
                struct mod_kallsyms *kallsyms;

                if (mod->state == MODULE_STATE_UNFORMED)
                        continue;

                if (modname && strcmp(modname, mod->name))
                        continue;
...

So with above change, 'if mod is not specified, search the symbols in kernel 
and all modules. If mod is sepecified, search the symbol on the specific
module'.

Thus, "if func_name is only part of the module named mod", the
module_kallsyms_on_each_symbol() will count the 'func_name' in 'mod' module
correctly.

Thank you,


Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads

[Francis Laniel]

Hi!

Le mercredi 1 novembre 2023, 01:15:09 EET Masami Hiramatsu a écrit :
> Hi,
> 
> On Tue, 31 Oct 2023 23:24:43 +0200
> 
> Francis Laniel <flaniel@linux.microsoft.com> wrote:
> > > @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const
> > > char
> > > *name, unsigned long unused) return 0;
> > > 
> > >  }
> > > 
> > > -static unsigned int number_of_same_symbols(char *func_name)
> > > +static unsigned int number_of_same_symbols(const char *mod, const char
> > > *func_name) {
> > > 
> > >  	struct sym_count_ctx ctx = { .count = 0, .name = func_name };
> > > 
> > > -	kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count);
> > > +	if (!mod)
> > > +		kallsyms_on_each_match_symbol(count_symbols, func_name,
> > 
> > &ctx.count);
> > 
> > > -	module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx);
> > > +	module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx);
> > 
> > I may be missing something here or reviewing too quickly.
> > Wouldn't this function return count to be 0 if func_name is only part of
> > the module named mod?
> 
> No, please read below.
> 
> > Indeed, if the function is not in kernel symbol,
> > kallsyms_on_each_match_symbol() will not loop.
> > And, by giving mod to module_kallsyms_on_each_symbol(), the corresponding
> > module will be skipped, so count_mob_symbols() would not be called.
> > Hence, we would have 0 as count, which would lead to ENOENT later.
> 
> Would you mean the case func_name is on the specific module?
> If 'mod' is specified, module_kallsyms_on_each_symbol() only loops on
> symbols in the module names 'mod'.
> 
> int module_kallsyms_on_each_symbol(const char *modname,
>                                    int (*fn)(void *, const char *, unsigned
> long), void *data)
> {
>         struct module *mod;
>         unsigned int i;
>         int ret = 0;
> 
>         mutex_lock(&module_mutex);
>         list_for_each_entry(mod, &modules, list) {
>                 struct mod_kallsyms *kallsyms;
> 
>                 if (mod->state == MODULE_STATE_UNFORMED)
>                         continue;
> 
>                 if (modname && strcmp(modname, mod->name))
>                         continue;
> ...
> 
> So with above change, 'if mod is not specified, search the symbols in kernel
> and all modules. If mod is sepecified, search the symbol on the specific
> module'.
> 
> Thus, "if func_name is only part of the module named mod", the
> module_kallsyms_on_each_symbol() will count the 'func_name' in 'mod' module
> correctly.

Sorry, I looked to quickly and forgot about the return value of strcmp()...

From the code, everything seems OK!
If I have some time, I will test it and potentially come back with a "Tested-
by" tag but without any warranty.

> Thank you,
> 
> 
> Thank you,

Best regards.

Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads

[Masami Hiramatsu]

On Thu, 02 Nov 2023 14:57:12 +0200
Francis Laniel <flaniel@linux.microsoft.com> wrote:

> Hi!
> 
> Le mercredi 1 novembre 2023, 01:15:09 EET Masami Hiramatsu a écrit :
> > Hi,
> > 
> > On Tue, 31 Oct 2023 23:24:43 +0200
> > 
> > Francis Laniel <flaniel@linux.microsoft.com> wrote:
> > > > @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const
> > > > char
> > > > *name, unsigned long unused) return 0;
> > > > 
> > > >  }
> > > > 
> > > > -static unsigned int number_of_same_symbols(char *func_name)
> > > > +static unsigned int number_of_same_symbols(const char *mod, const char
> > > > *func_name) {
> > > > 
> > > >  	struct sym_count_ctx ctx = { .count = 0, .name = func_name };
> > > > 
> > > > -	kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count);
> > > > +	if (!mod)
> > > > +		kallsyms_on_each_match_symbol(count_symbols, func_name,
> > > 
> > > &ctx.count);
> > > 
> > > > -	module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx);
> > > > +	module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx);
> > > 
> > > I may be missing something here or reviewing too quickly.
> > > Wouldn't this function return count to be 0 if func_name is only part of
> > > the module named mod?
> > 
> > No, please read below.
> > 
> > > Indeed, if the function is not in kernel symbol,
> > > kallsyms_on_each_match_symbol() will not loop.
> > > And, by giving mod to module_kallsyms_on_each_symbol(), the corresponding
> > > module will be skipped, so count_mob_symbols() would not be called.
> > > Hence, we would have 0 as count, which would lead to ENOENT later.
> > 
> > Would you mean the case func_name is on the specific module?
> > If 'mod' is specified, module_kallsyms_on_each_symbol() only loops on
> > symbols in the module names 'mod'.
> > 
> > int module_kallsyms_on_each_symbol(const char *modname,
> >                                    int (*fn)(void *, const char *, unsigned
> > long), void *data)
> > {
> >         struct module *mod;
> >         unsigned int i;
> >         int ret = 0;
> > 
> >         mutex_lock(&module_mutex);
> >         list_for_each_entry(mod, &modules, list) {
> >                 struct mod_kallsyms *kallsyms;
> > 
> >                 if (mod->state == MODULE_STATE_UNFORMED)
> >                         continue;
> > 
> >                 if (modname && strcmp(modname, mod->name))
> >                         continue;
> > ...
> > 
> > So with above change, 'if mod is not specified, search the symbols in kernel
> > and all modules. If mod is sepecified, search the symbol on the specific
> > module'.
> > 
> > Thus, "if func_name is only part of the module named mod", the
> > module_kallsyms_on_each_symbol() will count the 'func_name' in 'mod' module
> > correctly.
> 
> Sorry, I looked to quickly and forgot about the return value of strcmp()...

No problem, strcmp() always traps us :)

> 
> From the code, everything seems OK!
> If I have some time, I will test it and potentially come back with a "Tested-
> by" tag but without any warranty.


Thank you!



> 
> > Thank you,
> > 
> > 
> > Thank you,
> 
> Best regards.
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads

[Francis Laniel]

Hi!


Le dimanche 29 octobre 2023, 05:10:46 EET Masami Hiramatsu (Google) a écrit :
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Check the number of probe target symbols in the target module when
> the module is loaded. If the probe is not on the unique name symbols
> in the module, it will be rejected at that point.
> 
> Note that the symbol which has a unique name in the target module,
> it will be accepted even if there are same-name symbols in the
> kernel or other modules,

Sorry for the delay in my review, I just remember I should review your patch.
I tested it on top of latest master and it return EINVAL instead of 
EADDRNOTAVAIL:
# Behavior without the patch:
root@vm-amd64:~# uname -r
6.6.0-15859-g89cdf9d55601
root@vm-amd64:~# echo 'p:myprobe name_show' > /sys/kernel/tracing/
kprobe_events
-bash: echo: write error: Cannot assign requested address
# With the patch:
root@vm-amd64:~# uname -r
6.6.0-15860-gd6a7e0f39ec5
root@vm-amd64:~# echo 'p:myprobe name_show' > /sys/kernel/tracing/
kprobe_events
-bash: echo: write error: Invalid argument

I did not GDB to track down from where it comes (it is planned nonetheless), 
but is this behavior wanted?

> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_kprobe.c |  112
> ++++++++++++++++++++++++++----------------- 1 file changed, 68
> insertions(+), 44 deletions(-)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index e834f149695b..90cf2219adb4 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -670,6 +670,21 @@ static int register_trace_kprobe(struct trace_kprobe
> *tk) return ret;
>  }
> 
> +static int validate_module_probe_symbol(const char *modname, const char
> *symbol); +
> +static int register_module_trace_kprobe(struct module *mod, struct
> trace_kprobe *tk) +{
> +	const char *p;
> +	int ret = 0;
> +
> +	p = strchr(trace_kprobe_symbol(tk), ':');
> +	if (p)
> +		ret = validate_module_probe_symbol(module_name(mod), p++);
> +	if (!ret)
> +		ret = register_trace_kprobe(tk);
> +	return ret;
> +}
> +
>  /* Module notifier call back, checking event on the module */
>  static int trace_kprobe_module_callback(struct notifier_block *nb,
>  				       unsigned long val, void *data)
> @@ -688,7 +703,7 @@ static int trace_kprobe_module_callback(struct
> notifier_block *nb, if (trace_kprobe_within_module(tk, mod)) {
>  			/* Don't need to check busy - this should have gone. */
>  			__unregister_trace_kprobe(tk);
> -			ret = __register_trace_kprobe(tk);
> +			ret = register_module_trace_kprobe(mod, tk);
>  			if (ret)
>  				pr_warn("Failed to re-register probe %s on %s: 
%d\n",
>  					trace_probe_name(&tk->tp),
> @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const char
> *name, unsigned long unused) return 0;
>  }
> 
> -static unsigned int number_of_same_symbols(char *func_name)
> +static unsigned int number_of_same_symbols(const char *mod, const char
> *func_name) {
>  	struct sym_count_ctx ctx = { .count = 0, .name = func_name };
> 
> -	kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count);
> +	if (!mod)
> +		kallsyms_on_each_match_symbol(count_symbols, func_name, 
&ctx.count);
> 
> -	module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx);
> +	module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx);
> 
>  	return ctx.count;
>  }
> 
> +static int validate_module_probe_symbol(const char *modname, const char
> *symbol) +{
> +	unsigned int count = number_of_same_symbols(modname, symbol);
> +
> +	if (count > 1) {
> +		/*
> +		 * Users should use ADDR to remove the ambiguity of
> +		 * using KSYM only.
> +		 */
> +		return -EADDRNOTAVAIL;
> +	} else if (count == 0) {
> +		/*
> +		 * We can return ENOENT earlier than when register the
> +		 * kprobe.
> +		 */
> +		return -ENOENT;
> +	}
> +	return 0;
> +}
> +
> +static int validate_probe_symbol(char *symbol)
> +{
> +	char *mod = NULL, *p;
> +	int ret;
> +
> +	p = strchr(symbol, ':');
> +	if (p) {
> +		mod = symbol;
> +		symbol = p + 1;
> +		*p = '\0';
> +	}
> +	ret = validate_module_probe_symbol(mod, symbol);
> +	if (p)
> +		*p = ':';
> +	return ret;
> +}
> +
>  static int __trace_kprobe_create(int argc, const char *argv[])
>  {
>  	/*
> @@ -859,6 +912,14 @@ static int __trace_kprobe_create(int argc, const char
> *argv[]) trace_probe_log_err(0, BAD_PROBE_ADDR);
>  			goto parse_error;
>  		}
> +		ret = validate_probe_symbol(symbol);
> +		if (ret) {
> +			if (ret == -EADDRNOTAVAIL)
> +				trace_probe_log_err(0, NON_UNIQ_SYMBOL);
> +			else
> +				trace_probe_log_err(0, BAD_PROBE_ADDR);
> +			goto parse_error;
> +		}
>  		if (is_return)
>  			ctx.flags |= TPARG_FL_RETURN;
>  		ret = kprobe_on_func_entry(NULL, symbol, offset);
> @@ -871,31 +932,6 @@ static int __trace_kprobe_create(int argc, const char
> *argv[]) }
>  	}
> 
> -	if (symbol && !strchr(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.
> -			 */
> -			trace_probe_log_err(0, NON_UNIQ_SYMBOL);
> -			ret = -EADDRNOTAVAIL;
> -
> -			goto error;
> -		} else if (count == 0) {
> -			/*
> -			 * We can return ENOENT earlier than when register the
> -			 * kprobe.
> -			 */
> -			trace_probe_log_err(0, BAD_PROBE_ADDR);
> -			ret = -ENOENT;
> -
> -			goto error;
> -		}
> -	}
> -
>  	trace_probe_log_set_index(0);
>  	if (event) {
>  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
> @@ -1767,21 +1803,9 @@ create_local_trace_kprobe(char *func, void *addr,
> unsigned long offs, 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);
> +		ret = validate_probe_symbol(func);
> +		if (ret)
> +			return ERR_PTR(ret);
>  	}
> 
>  	/*


Best regards.