Return EADDRNOTAVAIL when func matches several symbols during kprobe creation

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/[email protected]/
--
2.34.1

From: Jiri Olsa <[email protected]>

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).

---
 include/linux/kallsyms.h | 7 ++++++-
 kernel/kallsyms.c        | 2 --
 2 files changed, 6 insertions(+), 3 deletions(-)

index a1d6fc82d7f0..eae9f423bd64 100644
@@ -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)

index 0ba87982d017..e0d9f77cf2d4 100644
@@ -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

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

Hi!

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

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

Hi!

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

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

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.

---
 kernel/trace/trace_kprobe.c | 74 +++++++++++++++++++++++++++++++++++++
 kernel/trace/trace_probe.h  |  1 +
 2 files changed, 75 insertions(+)

index 0b3ee4eea51b..1c565db2de7b 100644
@@ -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

index 0f0e5005b97a..82e1df8aefcb 100644
@@ -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