[PATCH 5.15.y v1 0/2] Return EADDRNOTAVAIL when func matches several symbols during kprobe creation

[PATCH 5.15.y v1 0/2] Return EADDRNOTAVAIL when func matches several symbols during kprobe creation

[Francis Laniel]

Hi!


The second patch of this series caused some troubles on 5.15.y, I apologize
about it [1].
When I tested it locally, I needed to set CONFIG_LIVEPATCH, so
kallsyms_on_each_symbol() would be known at link time.
To cope with this problem, we need to backport the first patch of this series as
pointed by Guenter Roeck [2].
This patch is only needed for 5.15, indeed the dependencies on CONFIG_LIVEPATCH
for kallsyms_on_each_symbol() was added in 3e3552056ab4
("kallsyms: only build {,module_}kallsyms_on_each_symbol when required").
This commit was first added to kernel 5.12, as git indicates it:
$ git name-rev --tags --name-only 3e3552056ab4
v5.12-rc1~67^2~8
Moreover, the first patch of this series, i.e. d721def7392a
("kallsyms: Make kallsyms_on_each_symbol generally available") was first added
in:
$ git name-rev --tags --name-only d721def7392a
v5.19-rc1~159^2~4^2~38^2~4
So, my patch only needs the first patch for the 5.15 kernel.

Regarding these two patches, I built and tested the series for several
architectures:
* i386:
  $ make ARCH=i386 defconfig kvm_guest.config
  ...
  $ make ARCH=i386 -j$(nproc)
  ...
    Kernel: arch/x86/boot/bzImage is ready  (#1)
  $ bash run_extern_kernel.sh i386
  ...
  root@vm-i386:~# uname -mr
  5.15.140+ i686
  root@vm-i386:~# echo 'p:myprobe name_show' > /sys/kernel/tracing/kprobe_events
  -bash: echo: write error: Cannot assign requested address
* x86_64:
  $ make x86_64_defconfig kvm_guest.config
  ...
  $ make -j$(nproc)
  ...
    Kernel: arch/x86/boot/bzImage is ready  (#9)
  $ bash run_extern_kernel.sh
  ...
  root@vm-amd64:~# uname -rm
  5.15.140+ x86_64
  root@vm-amd64:~# echo 'p:myprobe name_show' > /sys/kernel/tracing/kprobe_events
  -bash: echo: write error: Cannot assign requested address
* arm:
  $ make ARCH=arm defconfig kvm_guest.config
  ...
  $ make ARCH=arm menuconfig
  # Add CONFIG_KPROBES
  $ make ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- -j$(nproc)
  ...
    Kernel: arch/arm/boot/zImage is ready
  $ bash run_extern_kernel.sh armel
  ...
  root@vm-armel:~# uname -mr
  5.15.140-00002-gd3fdc3ca50b5 armv7l
  root@vm-armel:~# echo 'p:myprobe name_show' > /sys/kernel/tracing/kprobe_events
  -bash: echo: write error: Cannot assign requested address
* arm64:
  $ make ARCH=arm64 defconfig kvm_guest.config
  # Add CONFIG_KPROBES
  ...
  $ make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j$(nproc)
  ...
    OBJCOPY arch/arm64/boot/Image
    GZIP    arch/arm64/boot/Image.gz
  $ bash run_extern_kernel.sh arm64
  ...
  root@vm-arm64:~# uname -mr
  5.15.140-00002-gd3fdc3ca50b5 aarch64
  root@vm-arm64:~# echo 'p:myprobe name_show' > /sys/kernel/tracing/kprobe_events
  -bash: echo: write error: Cannot assign requested address

If you see any ways to improve this patch, please share your feedback.

Francis Laniel (1):
  tracing/kprobes: Return EADDRNOTAVAIL when func matches several
    symbols

Jiri Olsa (1):
  kallsyms: Make kallsyms_on_each_symbol generally available

 include/linux/kallsyms.h    |  7 +++-
 kernel/kallsyms.c           |  2 -
 kernel/trace/trace_kprobe.c | 74 +++++++++++++++++++++++++++++++++++++
 kernel/trace/trace_probe.h  |  1 +
 4 files changed, 81 insertions(+), 3 deletions(-)


Best regards.
---
[1]: https://lore.kernel.org/stable/CAEUSe78tYPTFuauB7cxZzvAeMhzB_25Q8DqLUfF7Nro9WsUhNw@mail.gmail.com/
[2]: https://lore.kernel.org/stable/06deae26-c59c-4746-867d-ab6f5852b0af@roeck-us.net/
--
2.34.1

[PATCH 5.15.y v1 1/2] kallsyms: Make kallsyms_on_each_symbol generally available

[Francis Laniel]

From: Jiri Olsa <jolsa@kernel.org>

Making kallsyms_on_each_symbol generally available, so it can be
used outside CONFIG_LIVEPATCH option in following changes.

Rather than adding another ifdef option let's make the function
generally available (when CONFIG_KALLSYMS option is defined).

Cc: Christoph Hellwig <hch@lst.de>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Link: https://lore.kernel.org/r/20220510122616.2652285-2-jolsa@kernel.org
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/kallsyms.h | 7 ++++++-
 kernel/kallsyms.c        | 2 --
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index a1d6fc82d7f0..eae9f423bd64 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -74,11 +74,11 @@ static inline void *dereference_symbol_descriptor(void *ptr)
 	return ptr;
 }
 
+#ifdef CONFIG_KALLSYMS
 int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
 				      unsigned long),
 			    void *data);
 
-#ifdef CONFIG_KALLSYMS
 /* Lookup the address for a symbol. Returns 0 if not found. */
 unsigned long kallsyms_lookup_name(const char *name);
 
@@ -172,6 +172,11 @@ static inline bool kallsyms_show_value(const struct cred *cred)
 	return false;
 }
 
+static inline int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
+					  unsigned long), void *data)
+{
+	return -EOPNOTSUPP;
+}
 #endif /*CONFIG_KALLSYMS*/
 
 static inline void print_ip_sym(const char *loglvl, unsigned long ip)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 0ba87982d017..e0d9f77cf2d4 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -204,7 +204,6 @@ unsigned long kallsyms_lookup_name(const char *name)
 	return module_kallsyms_lookup_name(name);
 }
 
-#ifdef CONFIG_LIVEPATCH
 /*
  * Iterate over all symbols in vmlinux.  For symbols from modules use
  * module_kallsyms_on_each_symbol instead.
@@ -226,7 +225,6 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
 	}
 	return 0;
 }
-#endif /* CONFIG_LIVEPATCH */
 
 static unsigned long get_symbol_pos(unsigned long addr,
 				    unsigned long *symbolsize,
-- 
2.34.1

Re: [PATCH 5.15.y v1 1/2] kallsyms: Make kallsyms_on_each_symbol generally available

[Greg KH]

On Fri, Dec 01, 2023 at 04:19:56PM +0100, Francis Laniel wrote:
> From: Jiri Olsa <jolsa@kernel.org>
> 
> Making kallsyms_on_each_symbol generally available, so it can be
> used outside CONFIG_LIVEPATCH option in following changes.
> 
> Rather than adding another ifdef option let's make the function
> generally available (when CONFIG_KALLSYMS option is defined).
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> Link: https://lore.kernel.org/r/20220510122616.2652285-2-jolsa@kernel.org
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  include/linux/kallsyms.h | 7 ++++++-
>  kernel/kallsyms.c        | 2 --
>  2 files changed, 6 insertions(+), 3 deletions(-)

What is the git id of this commit in Linus's tree?

Re: [PATCH 5.15.y v1 1/2] kallsyms: Make kallsyms_on_each_symbol generally available

[Francis Laniel]

Hi!


Le samedi 2 décembre 2023, 00:02:58 CET Greg KH a écrit :
> On Fri, Dec 01, 2023 at 04:19:56PM +0100, Francis Laniel wrote:
> > From: Jiri Olsa <jolsa@kernel.org>
> > 
> > Making kallsyms_on_each_symbol generally available, so it can be
> > used outside CONFIG_LIVEPATCH option in following changes.
> > 
> > Rather than adding another ifdef option let's make the function
> > generally available (when CONFIG_KALLSYMS option is defined).
> > 
> > Cc: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > Link: https://lore.kernel.org/r/20220510122616.2652285-2-jolsa@kernel.org
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > ---
> > 
> >  include/linux/kallsyms.h | 7 ++++++-
> >  kernel/kallsyms.c        | 2 --
> >  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> What is the git id of this commit in Linus's tree?

Sorry, the commit ID is [1]:
d721def7392a7348ffb9f3583b264239cbd3702c


Best regards.
---
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
commit/?id=d721def7392a7348ffb9f3583b264239cbd3702c

Re: [PATCH 5.15.y v1 1/2] kallsyms: Make kallsyms_on_each_symbol generally available

[Greg KH]

On Mon, Dec 04, 2023 at 09:19:05AM +0100, Francis Laniel wrote:
> Hi!
> 
> 
> Le samedi 2 décembre 2023, 00:02:58 CET Greg KH a écrit :
> > On Fri, Dec 01, 2023 at 04:19:56PM +0100, Francis Laniel wrote:
> > > From: Jiri Olsa <jolsa@kernel.org>
> > > 
> > > Making kallsyms_on_each_symbol generally available, so it can be
> > > used outside CONFIG_LIVEPATCH option in following changes.
> > > 
> > > Rather than adding another ifdef option let's make the function
> > > generally available (when CONFIG_KALLSYMS option is defined).
> > > 
> > > Cc: Christoph Hellwig <hch@lst.de>
> > > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > Link: https://lore.kernel.org/r/20220510122616.2652285-2-jolsa@kernel.org
> > > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > > ---
> > > 
> > >  include/linux/kallsyms.h | 7 ++++++-
> > >  kernel/kallsyms.c        | 2 --
> > >  2 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > What is the git id of this commit in Linus's tree?
> 
> Sorry, the commit ID is [1]:
> d721def7392a7348ffb9f3583b264239cbd3702c

Please send a new, updated series for all branches that you wish this
series to go on, and MOST IMPORTANTLY, some sort of proof that this
actually works this time...

In other words, you need to test-build this on all arches and somehow
run-time test it as well, good luck!

thanks,

greg k-h

Re: [PATCH 5.15.y v1 1/2] kallsyms: Make kallsyms_on_each_symbol generally available

[Francis Laniel]

Hi!


Le mardi 5 décembre 2023, 00:58:38 CET Greg KH a écrit :
> On Mon, Dec 04, 2023 at 09:19:05AM +0100, Francis Laniel wrote:
> > Hi!
> > 
> > Le samedi 2 décembre 2023, 00:02:58 CET Greg KH a écrit :
> > > On Fri, Dec 01, 2023 at 04:19:56PM +0100, Francis Laniel wrote:
> > > > From: Jiri Olsa <jolsa@kernel.org>
> > > > 
> > > > Making kallsyms_on_each_symbol generally available, so it can be
> > > > used outside CONFIG_LIVEPATCH option in following changes.
> > > > 
> > > > Rather than adding another ifdef option let's make the function
> > > > generally available (when CONFIG_KALLSYMS option is defined).
> > > > 
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > Link:
> > > > https://lore.kernel.org/r/20220510122616.2652285-2-jolsa@kernel.org
> > > > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > > > ---
> > > > 
> > > >  include/linux/kallsyms.h | 7 ++++++-
> > > >  kernel/kallsyms.c        | 2 --
> > > >  2 files changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > What is the git id of this commit in Linus's tree?
> > 
> > Sorry, the commit ID is [1]:
> > d721def7392a7348ffb9f3583b264239cbd3702c
> 
> Please send a new, updated series for all branches that you wish this
> series to go on, and MOST IMPORTANTLY, some sort of proof that this
> actually works this time...
> 
> In other words, you need to test-build this on all arches and somehow
> run-time test it as well, good luck!

I will take a look at TuxMake to ease this process [1].
But if someone has other tools to advise, feedback would be welcomed.

> thanks,
> 
> greg k-h


Best regards.
---
[1]: https://tuxmake.org/

[PATCH 5.15.y v1 2/2] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols

[Francis Laniel]

When a kprobe is attached to a function that's name is not unique (is
static and shares the name with other functions in the kernel), the
kprobe is attached to the first function it finds. This is a bug as the
function that it is attaching to is not necessarily the one that the
user wants to attach to.

Instead of blindly picking a function to attach to what is ambiguous,
error with EADDRNOTAVAIL to let the user know that this function is not
unique, and that the user must use another unique function with an
address offset to get to the function they want to attach to.

Link: https://lore.kernel.org/all/20231020104250.9537-2-flaniel@linux.microsoft.com/

Cc: stable@vger.kernel.org
Fixes: 413d37d1eb69 ("tracing: Add kprobe-based event tracer")
Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Link: https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kernel.org/
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
(cherry picked from commit b022f0c7e404887a7c5229788fc99eff9f9a80d5)
---
 kernel/trace/trace_kprobe.c | 74 +++++++++++++++++++++++++++++++++++++
 kernel/trace/trace_probe.h  |  1 +
 2 files changed, 75 insertions(+)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 0b3ee4eea51b..1c565db2de7b 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -708,6 +708,36 @@ static struct notifier_block trace_kprobe_module_nb = {
 	.priority = 1	/* Invoked after kprobe module callback */
 };
 
+struct count_symbols_struct {
+	const char *func_name;
+	unsigned int count;
+};
+
+static int count_symbols(void *data, const char *name, struct module *unused0,
+			 unsigned long unused1)
+{
+	struct count_symbols_struct *args = data;
+
+	if (strcmp(args->func_name, name))
+		return 0;
+
+	args->count++;
+
+	return 0;
+}
+
+static unsigned int number_of_same_symbols(char *func_name)
+{
+	struct count_symbols_struct args = {
+		.func_name = func_name,
+		.count = 0,
+	};
+
+	kallsyms_on_each_symbol(count_symbols, &args);
+
+	return args.count;
+}
+
 static int __trace_kprobe_create(int argc, const char *argv[])
 {
 	/*
@@ -836,6 +866,31 @@ 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, buf,
@@ -1755,6 +1810,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,
@@ -1765,6 +1821,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
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 0f0e5005b97a..82e1df8aefcb 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -405,6 +405,7 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
 	C(BAD_MAXACT,		"Invalid maxactive number"),		\
 	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"), \
 	C(BAD_ADDR_SUFFIX,	"Invalid probed address suffix"), \
 	C(NO_GROUP_NAME,	"Group name is not specified"),		\
-- 
2.34.1