Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
References: <169854904604.132316.12500381416261460174.stgit@devnote2>
In-Reply-To: <169854904604.132316.12500381416261460174.stgit@devnote2>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Tue, 31 Oct 2023 09:56:58 -0700
Message-ID: <CAEf4BzaRCOKL1iYsryLCR0pO8wXHnxGE0=1j6+nYx7m07j7YFg@mail.gmail.com>
Subject: Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when
 module loads
To:     "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Linux trace kernel <linux-trace-kernel@vger.kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Andrii Nakryiko <andrii@kernel.org>,
        Francis Laniel <flaniel@linux.microsoft.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Oct 28, 2023 at 8:10=E2=80=AFPM 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 =3D 0;
> +
> +       p =3D strchr(trace_kprobe_symbol(tk), ':');
> +       if (p)
> +               ret =3D validate_module_probe_symbol(module_name(mod), p+=
+);
> +       if (!ret)
> +               ret =3D 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 notifi=
er_block *nb,
>                 if (trace_kprobe_within_module(tk, mod)) {
>                         /* Don't need to check busy - this should have go=
ne. */
>                         __unregister_trace_kprobe(tk);
> -                       ret =3D __register_trace_kprobe(tk);
> +                       ret =3D register_module_trace_kprobe(mod, tk);
>                         if (ret)
>                                 pr_warn("Failed to re-register probe %s o=
n %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 =3D { .count =3D 0, .name =3D func_name =
};
>
> -       kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.coun=
t);
> +       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 =3D 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 =3D=3D 0) {
> +               /*
> +                * We can return ENOENT earlier than when register the
> +                * kprobe.
> +                */
> +               return -ENOENT;
> +       }
> +       return 0;
> +}
> +
> +static int validate_probe_symbol(char *symbol)
> +{
> +       char *mod =3D NULL, *p;
> +       int ret;
> +
> +       p =3D strchr(symbol, ':');
> +       if (p) {
> +               mod =3D symbol;
> +               symbol =3D p + 1;
> +               *p =3D '\0';
> +       }
> +       ret =3D validate_module_probe_symbol(mod, symbol);
> +       if (p)
> +               *p =3D ':';
> +       return ret;
> +}
> +
>  static int __trace_kprobe_create(int argc, const char *argv[])
>  {
>         /*
> @@ -859,6 +912,14 @@ static int __trace_kprobe_create(int argc, const cha=
r *argv[])
>                         trace_probe_log_err(0, BAD_PROBE_ADDR);
>                         goto parse_error;
>                 }
> +               ret =3D validate_probe_symbol(symbol);
> +               if (ret) {
> +                       if (ret =3D=3D -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 |=3D TPARG_FL_RETURN;
>                 ret =3D kprobe_on_func_entry(NULL, symbol, offset);
> @@ -871,31 +932,6 @@ static int __trace_kprobe_create(int argc, const cha=
r *argv[])
>                 }
>         }
>
> -       if (symbol && !strchr(symbol, ':')) {
> -               unsigned int count;
> -
> -               count =3D 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 =3D -EADDRNOTAVAIL;
> -
> -                       goto error;
> -               } else if (count =3D=3D 0) {
> -                       /*
> -                        * We can return ENOENT earlier than when registe=
r the
> -                        * kprobe.
> -                        */
> -                       trace_probe_log_err(0, BAD_PROBE_ADDR);
> -                       ret =3D -ENOENT;
> -
> -                       goto error;
> -               }
> -       }
> -
>         trace_probe_log_set_index(0);
>         if (event) {
>                 ret =3D 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 =3D 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 =3D=3D 0)
> -                       /*
> -                        * We can return ENOENT earlier than when registe=
r the
> -                        * kprobe.
> -                        */
> -                       return ERR_PTR(-ENOENT);
> +               ret =3D validate_probe_symbol(func);
> +               if (ret)
> +                       return ERR_PTR(ret);
>         }
>
>         /*
>