livepatch: match symbols exactly in klp_find_object_symbol()

PATCH In review Song Liu 5 participants 7 messages Jun 2, 2023 – Jun 16, 2023 Live patching

Share & tools

Song Liu

author maintainer

2023-06-02 23:24 UTC #0 lore raw

PATCH 1/1 livepatch: match symbols exactly in klp_find_object_symbol()

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:

livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Fix this by using kallsyms_on_each_symbol instead, and matching symbols exactly.

Signed-off-by Song Liu <[email protected]>

---
 kernel/livepatch/core.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

kernel/livepatch/core.c +5 ~~-12~~

index 61328328c474..507e1e2e4290 100644
@@ -125,10 +125,13 @@ struct klp_find_arg {
 	unsigned long pos;
 };
 
-static int klp_match_callback(void *data, unsigned long addr)
+static int klp_find_callback(void *data, const char *name, unsigned long addr)
 {
 	struct klp_find_arg *args = data;
 
+	if (strcmp(args->name, name))
+		return 0;
+
 	args->addr = addr;
 	args->count++;

@@ -143,16 +146,6 @@ static int klp_match_callback(void *data, unsigned long addr)
 	return 0;
 }
 
-static int klp_find_callback(void *data, const char *name, unsigned long addr)
-{
-	struct klp_find_arg *args = data;
-
-	if (strcmp(args->name, name))
-		return 0;
-
-	return klp_match_callback(data, addr);
-}
-
 static int klp_find_object_symbol(const char *objname, const char *name,
 				  unsigned long sympos, unsigned long *addr)
 {

@@ -166,7 +159,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 	if (objname)
 		module_kallsyms_on_each_symbol(objname, klp_find_callback, &args);
 	else
-		kallsyms_on_each_match_symbol(klp_match_callback, name, &args);
+		kallsyms_on_each_symbol(klp_find_callback, &args);
 
 	/*
 	 * Ensure an address was found. If sympos is 0, ensure symbol is unique;

Signature · 1 line

2.34.1

Josh Poimboeuf

maintainer Acked-by on v1 1/1

2023-06-05 16:39 UTC #1 lore raw

Quoted 11 lines · Song Liu

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:
livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
Fix this by using kallsyms_on_each_symbol instead, and matching symbols exactly.
Signed-off-by Song Liu <[email protected]>

Acked-by Josh Poimboeuf <[email protected]>

Signature · 1 line

Josh

Petr Mladek

maintainer

2023-06-09 07:09 UTC #2 lore raw

In reply to Song Liu

Quoted 13 lines · Song Liu

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:
livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
Fix this by using kallsyms_on_each_symbol instead, and matching symbols exactly.

kernel/livepatch/core.c static int klp_find_object_symbol(const char *objname, const char *name, · line 162

Quoted 5 lines

kernel/livepatch/core.c +1 -1

@@ -166,7 +159,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 	if (objname)
 		module_kallsyms_on_each_symbol(objname, klp_find_callback, &args);
 	else
-		kallsyms_on_each_match_symbol(klp_match_callback, name, &args);
+		kallsyms_on_each_symbol(klp_find_callback, &args);

AFAIK, you have put a lot of effort to optimize the search recently. The speedup was amazing, see commit 4dc533e0f2c04174e1ae ("kallsyms: Add helper kallsyms_on_each_match_symbol()").

Do we really need to waste this effort completely?

What about creating variants:

+ kallsyms_on_each_match_exact_symbol()
+ kallsyms_lookup_exact_names()
+ compare_exact_symbol_name()

Where compare_exact_symbol_name() would not try comparing with cleanup_symbol_name()?

kernel/livepatch/core.c static int klp_find_object_symbol(const char *objname, const char *name, · line 165

Quoted 3 lines

kernel/livepatch/core.c +0 -0

 	/*
 	 * Ensure an address was found. If sympos is 0, ensure symbol is unique;

Otherwise, the patch looks fine. It is acceptable for me. I just want to be sure that we considered the above alternative solution.

Best Regards,
Petr

Song Liu

2023-06-09 16:35 UTC #3 lore raw

Quoted 15 lines · Petr Mladek

Quoted 10 lines · Song Liu
With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:
livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
Fix this by using kallsyms_on_each_symbol instead, and matching symbols exactly.

Quoted 13 lines

Quoted 8 lines
kernel/livepatch/core.c +0 -0
@@ -166,7 +159,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 if (objname)
 module_kallsyms_on_each_symbol(objname, klp_find_callback, &args);
else
- kallsyms_on_each_match_symbol(klp_match_callback, name, &args);
+ kallsyms_on_each_symbol(klp_find_callback, &args);
AFAIK, you have put a lot of effort to optimize the search recently. The speedup was amazing, see commit 4dc533e0f2c04174e1ae ("kallsyms: Add helper kallsyms_on_each_match_symbol()").

That's not me. :) Or maybe you meant Josh?

Quoted 10 lines

Do we really need to waste this effort completely?
What about creating variants:
+ kallsyms_on_each_match_exact_symbol()
+ kallsyms_lookup_exact_names()
+ compare_exact_symbol_name()
Where compare_exact_symbol_name() would not try comparing with cleanup_symbol_name()?

The rationale is that livepatch symbol look up is not a hot path, and the changes (especially with kallsyms_lookup_exact_names) seem an overkill. OTOH, this version is simpler and should work just as well.

Thanks,
Song

Quoted 10 lines

Quoted 4 lines
/*
* Ensure an address was found. If sympos is 0, ensure symbol is unique;
Otherwise, the patch looks fine. It is acceptable for me. I just want to be sure that we considered the above alternative solution.
Best Regards,
Petr

Petr Mladek

maintainer

2023-06-13 15:16 UTC #4 lore raw

Quoted 29 lines · Song Liu

Quoted 15 lines · Petr Mladek
Quoted 10 lines · Song Liu
With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:
livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
Fix this by using kallsyms_on_each_symbol instead, and matching symbols exactly.
Quoted 13 lines
Quoted 8 lines
kernel/livepatch/core.c +0 -0
@@ -166,7 +159,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 if (objname)
 module_kallsyms_on_each_symbol(objname, klp_find_callback, &args);
else
- kallsyms_on_each_match_symbol(klp_match_callback, name, &args);
+ kallsyms_on_each_symbol(klp_find_callback, &args);
AFAIK, you have put a lot of effort to optimize the search recently. The speedup was amazing, see commit 4dc533e0f2c04174e1ae ("kallsyms: Add helper kallsyms_on_each_match_symbol()").

Quoted 1 line

That's not me. :) Or maybe you meant Josh?

Ah, I see. I am sorry, I am bad at names.

Quoted 8 lines

Quoted 8 lines
Do we really need to waste this effort completely?
What about creating variants:
+ kallsyms_on_each_match_exact_symbol()
+ kallsyms_lookup_exact_names()
+ compare_exact_symbol_name()

Quoted 6 lines

Quoted 2 lines
Where compare_exact_symbol_name() would not try comparing with cleanup_symbol_name()?
The rationale is that livepatch symbol look up is not a hot path, and the changes (especially with kallsyms_lookup_exact_names) seem an overkill.

I agree that it is a slow path.

Well, Zhen put a lot of effort into the optimization. I am not sure what was the primary motivation. But it would be harsh to remove it without asking.

Zhen, what was the motivation for the speedup of kallsyms, please?

Quoted 2 lines

OTOH, this version is simpler and should work just as well.

Sure. But we should double check Zhen's motivation.

Anyway, iterating over all symbols costs a lot. See also the commit f5bdb34bf0c9314548f2d ("livepatch: Avoid CPU hogging with cond_resched").

Best Regards,
Petr

Song Liu

author maintainer

2023-06-15 16:58 UTC #5 lore raw

Quoted 1 line · Petr Mladek

[...]

Quoted 9 lines

I agree that it is a slow path.
Well, Zhen put a lot of effort into the optimization. I am not sure what was the primary motivation. But it would be harsh to remove it without asking.
Zhen, what was the motivation for the speedup of kallsyms, please?

I took a closer look at the code. kallsyms_on_each_match_symbol() is only used by livepatch. So indeed it doesn't make sense to drop all the optimizations by Zhen.

I got another solution for this. Will send it shortly.

Thanks,
Song

Quoted 9 lines

Quoted 3 lines
OTOH, this version is simpler and should work just as well.
Sure. But we should double check Zhen's motivation.
Anyway, iterating over all symbols costs a lot. See also the commit f5bdb34bf0c9314548f2d ("livepatch: Avoid CPU hogging with cond_resched").

Leizhen (ThunderTown)

2023-06-16 01:24 UTC #6 lore raw

Quoted 6 lines · Song Liu

Quoted 1 line · Petr Mladek
[...]
Quoted 1 line

Quoted 8 lines

Quoted 8 lines
I agree that it is a slow path.
Well, Zhen put a lot of effort into the optimization. I am not sure what was the primary motivation. But it would be harsh to remove it without asking.
Zhen, what was the motivation for the speedup of kallsyms, please?

If a large number of functions are modified, livepatch may fail. In some actual scenarios(business), the patch needs to be activated within a limited time.

Quoted 4 lines

I took a closer look at the code. kallsyms_on_each_match_symbol() is only used by livepatch. So indeed it doesn't make sense to drop all the optimizations by Zhen.

Yes, if CLANG has special requirements, it can be specially addressed. I was thinking about CLANG, and I'll take a closer look at the current problem.

Quoted 17 lines

I got another solution for this. Will send it shortly.
Thanks,
Song
Quoted 9 lines
Quoted 3 lines
OTOH, this version is simpler and should work just as well.
Sure. But we should double check Zhen's motivation.
Anyway, iterating over all symbols costs a lot. See also the commit f5bdb34bf0c9314548f2d ("livepatch: Avoid CPU hogging with cond_resched").
.

Signature · 2 lines

Regards,
  Zhen Lei