Message-Id: <20240723063258.2240610-4-zhengyejian@huaweicloud.com>
Date: Tue, 23 Jul 2024 14:32:56 +0800
From: Zheng Yejian <zhengyejian@...weicloud.com>
To: masahiroy@...nel.org,
	peterz@...radead.org,
	rostedt@...dmis.org,
	mhiramat@...nel.org,
	mark.rutland@....com,
	mpe@...erman.id.au,
	npiggin@...il.com,
	christophe.leroy@...roup.eu,
	naveen.n.rao@...ux.ibm.com,
	tglx@...utronix.de,
	mingo@...hat.com,
	bp@...en8.de,
	dave.hansen@...ux.intel.com,
	hpa@...or.com,
	mcgrof@...nel.org,
	mathieu.desnoyers@...icios.com,
	nathan@...nel.org,
	nicolas@...sle.eu,
	ojeda@...nel.org,
	akpm@...ux-foundation.org,
	surenb@...gle.com,
	pasha.tatashin@...een.com,
	kent.overstreet@...ux.dev,
	james.clark@....com,
	jpoimboe@...nel.org
Cc: x86@...nel.org,
	linux-kernel@...r.kernel.org,
	linux-trace-kernel@...r.kernel.org,
	linuxppc-dev@...ts.ozlabs.org,
	linux-modules@...r.kernel.org,
	linux-kbuild@...r.kernel.org,
	bpf@...r.kernel.org,
	zhengyejian@...weicloud.com
Subject: [PATCH v2 3/5] ftrace: Skip invalid __fentry__ in ftrace_process_locs()

ftrace_location() was changed to not only return the __fentry__ location
when called for the __fentry__ location, but also when called for the
sym+0 location after commit aebfd12521d9 ("x86/ibt,ftrace: Search for
__fentry__ location"). That is, if sym+0 location is not __fentry__,
ftrace_location() would find one over the entire size of the sym.

However, there is case that more than one __fentry__ exist in the sym
range (described below) and ftrace_location() would find wrong __fentry__
location by binary searching, which would cause its users like livepatch/
kprobe/bpf to not work properly on this sym!

The case is that, based on current compiler behavior, suppose:
 - function A is followed by weak function B1 in same binary file;
 - weak function B1 is overridden by function B2;
Then in the final binary file:
 - symbol B1 will be removed from symbol table while its instructions are
   not removed;
 - __fentry__ of B1 will be still in __mcount_loc table;
 - function size of A is computed by substracting the symbol address of
   A from its next symbol address (see kallsyms_lookup_size_offset()),
   but because symbol info of B1 is removed, the next symbol of A is
   originally the next symbol of B1. See following example, function
   sizeof A will be (symbol_address_C - symbol_address_A):

     symbol_address_A
     symbol_address_B1 (Not in symbol table)
     symbol_address_C

The weak function issue has been discovered in commit b39181f7c690
("ftrace: Add FTRACE_MCOUNT_MAX_OFFSET to avoid adding weak function")
but it didn't resolve the issue in ftrace_location().

To solve the issue, with Peter's suggestions, in previous patches, all
holes in the text have been found and filled with specail symbols, also
the same case with module weak function has been handled. Then check and
skip __fentry__ that locate in the holes.

Also in this patch, introduce module_kallsyms_find_symbol() to check if
a __fentry__ locate in a valid function of the given module. It is needed
because other symbol lookup functions like module_address_lookup() will
find module of the passed address first, but as ftrace_process_locs() is
called, the module has not been fully loaded, so those lookup functions
can not work.

Fixes: aebfd12521d9 ("x86/ibt,ftrace: Search for __fentry__ location")
Signed-off-by: Zheng Yejian <zhengyejian@...weicloud.com>
---
 include/linux/module.h   |  7 +++++++
 kernel/module/kallsyms.c | 23 +++++++++++++++++------
 kernel/trace/ftrace.c    | 12 +++++++++++-
 3 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 0299d79433ae..4f5dd018e33d 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -962,6 +962,8 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 unsigned long module_kallsyms_lookup_name(const char *name);
 
 unsigned long find_kallsyms_symbol_value(struct module *mod, const char *name);
+int module_kallsyms_find_symbol(struct module *mod, unsigned long addr,
+				unsigned long *size, unsigned long *offset);
 
 #else	/* CONFIG_MODULES && CONFIG_KALLSYMS */
 
@@ -1006,6 +1008,11 @@ static inline unsigned long find_kallsyms_symbol_value(struct module *mod,
 	return 0;
 }
 
+static inline int module_kallsyms_find_symbol(struct module *mod, unsigned long addr,
+					      unsigned long *size, unsigned long *offset)
+{
+	return 0;
+}
 #endif  /* CONFIG_MODULES && CONFIG_KALLSYMS */
 
 #endif /* _LINUX_MODULE_H */
diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index cce4f81b9933..71b3ed25cd40 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -253,10 +253,10 @@ static const char *kallsyms_symbol_name(struct mod_kallsyms *kallsyms, unsigned
  * Given a module and address, find the corresponding symbol and return its name
  * while providing its size and offset if needed.
  */
-static const char *find_kallsyms_symbol(struct module *mod,
-					unsigned long addr,
-					unsigned long *size,
-					unsigned long *offset)
+static const char *__find_kallsyms_symbol(struct module *mod,
+					  unsigned long addr,
+					  unsigned long *size,
+					  unsigned long *offset)
 {
 	unsigned int i, best = 0;
 	unsigned long nextval, bestval;
@@ -326,6 +326,17 @@ static const char *find_kallsyms_symbol(struct module *mod,
 	return kallsyms_symbol_name(kallsyms, best);
 }
 
+int module_kallsyms_find_symbol(struct module *mod, unsigned long addr,
+				unsigned long *size, unsigned long *offset)
+{
+	const char *ret;
+
+	preempt_disable();
+	ret = __find_kallsyms_symbol(mod, addr, size, offset);
+	preempt_enable();
+	return !!ret;
+}
+
 void * __weak dereference_module_function_descriptor(struct module *mod,
 						     void *ptr)
 {
@@ -360,7 +371,7 @@ int module_address_lookup(unsigned long addr,
 #endif
 		}
 
-		sym = find_kallsyms_symbol(mod, addr, size, offset);
+		sym = __find_kallsyms_symbol(mod, addr, size, offset);
 
 		if (sym)
 			ret = strscpy(namebuf, sym, KSYM_NAME_LEN);
@@ -381,7 +392,7 @@ int lookup_module_symbol_name(unsigned long addr, char *symname)
 		if (within_module(addr, mod)) {
 			const char *sym;
 
-			sym = find_kallsyms_symbol(mod, addr, NULL, NULL);
+			sym = __find_kallsyms_symbol(mod, addr, NULL, NULL);
 			if (!sym)
 				goto out;
 
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 0f579430f02a..fff5d3466c41 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -6989,6 +6989,16 @@ static void test_is_sorted(unsigned long *start, unsigned long count)
 }
 #endif
 
+static int is_invalid_rec(struct module *mod, unsigned long addr)
+{
+	char str[KSYM_SYMBOL_LEN];
+
+	if (mod)
+		return !module_kallsyms_find_symbol(mod, addr, NULL, NULL);
+
+	return !kallsyms_lookup(addr, NULL, NULL, NULL, str);
+}
+
 static int ftrace_process_locs(struct module *mod,
 			       unsigned long *start,
 			       unsigned long *end)
@@ -7060,7 +7070,7 @@ static int ftrace_process_locs(struct module *mod,
 		 * object files to satisfy alignments.
 		 * Skip any NULL pointers.
 		 */
-		if (!addr) {
+		if (!addr || is_invalid_rec(mod, addr)) {
 			skipped++;
 			continue;
 		}
-- 
2.25.1

Powered by blists - more mailing lists