[llvm-symbolizer] restore --[no-]use-symbol-table option #71008

quic-likaid · 2023-11-02T01:04:52Z

Linux kernel modules don't have section address set statically. During kernel fuzzing, we may get a fuction address but llvm-symbolizer ends up with a static variable, because in symbolizer's view, bss and text sections both starts from 0, and thus overlap.

The option was unintentionally removed by 593e196, and remained as a no-op since 3d54976. Adding back the option allows us to prevent the undesired behaviour.

llvmbot · 2023-11-02T01:05:22Z

@llvm/pr-subscribers-llvm-binary-utilities

Author: None (quic-likaid)

Changes

Linux kernel modules don't have section address set statically. During kernel fuzzing, we may get a fuction address but llvm-symbolizer ends up with a static variable, because in symbolizer's view, bss and text sections both starts from 0, and thus overlap.

The option was unintentionally removed by 593e196, and remained as a no-op since 3d54976. Adding back the option allows us to prevent the undesired behaviour.

Full diff: https://github.com/llvm/llvm-project/pull/71008.diff

3 Files Affected:

(modified) llvm/docs/CommandGuide/llvm-symbolizer.rst (+10)
(modified) llvm/tools/llvm-symbolizer/Opts.td (+2)
(modified) llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp (+2-1)

diff --git a/llvm/docs/CommandGuide/llvm-symbolizer.rst b/llvm/docs/CommandGuide/llvm-symbolizer.rst
index fe5df077b45664d..a85dbdfef47d408 100644
--- a/llvm/docs/CommandGuide/llvm-symbolizer.rst
+++ b/llvm/docs/CommandGuide/llvm-symbolizer.rst
@@ -303,6 +303,11 @@ OPTIONS
 
   Don't print demangled function names.
 
+.. option:: --no-use-symbol-table
+
+  Don't prefer function names stored in symbol table to function names in debug
+  info sections.
+
 .. option:: --obj <path>, --exe, -e
 
   Path to object file to be symbolized. If ``-`` is specified, read the object
@@ -447,6 +452,11 @@ OPTIONS
   of the absolute path. If the command-line to the compiler included
   the full path, this will be the same as the default.
 
+.. option:: --use-symbol-table
+
+  Prefer function names stored in symbol table to function names in debug info
+  sections. This is the default.
+
 .. option:: --verbose
 
   Print verbose address, line and column information.
diff --git a/llvm/tools/llvm-symbolizer/Opts.td b/llvm/tools/llvm-symbolizer/Opts.td
index 6742e086d6ff954..29d376457a929b0 100644
--- a/llvm/tools/llvm-symbolizer/Opts.td
+++ b/llvm/tools/llvm-symbolizer/Opts.td
@@ -57,6 +57,8 @@ def relative_address : F<"relative-address", "Interpret addresses as addresses r
 def relativenames : F<"relativenames", "Strip the compilation directory from paths">;
 defm untag_addresses : B<"untag-addresses", "", "Remove memory tags from addresses before symbolization">;
 def use_dia: F<"dia", "Use the DIA library to access symbols (Windows only)">;
+defm use_symbol_table : B<"use-symbol-table", "Prefer function names stored in symbol table",
+                          "Don't prefer function names stored in symbol table">;
 def verbose : F<"verbose", "Print verbose line info">;
 def version : F<"version", "Display the version">;
 
diff --git a/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp b/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
index 78a0e6772f3fb36..646bcd163e93c32 100644
--- a/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
+++ b/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
@@ -469,7 +469,8 @@ int llvm_symbolizer_main(int argc, char **argv, const llvm::ToolContext &) {
     Opts.UseDIA = false;
   }
 #endif
-  Opts.UseSymbolTable = true;
+  Opts.UseSymbolTable =
+      Args.hasFlag(OPT_use_symbol_table, OPT_no_use_symbol_table, true);
   if (Args.hasArg(OPT_cache_size_EQ))
     parseIntArg(Args, OPT_cache_size_EQ, Opts.MaxCacheSize);
   Config.PrintAddress = Args.hasArg(OPT_addresses);

jh7370

Test case?

@MaskRay, any thoughts?

dwblaikie · 2023-11-02T17:55:37Z

The option was unintentionally removed by 593e196, and remained as a no-op since 3d54976.

Got links to these commits? I can't find them at first glance, at least...

So the issue is that if we use the ELF symbol table we can't differentiate functions from variables, but if we use the DWARF we can, and so we don't mistakenly symbolize unrelocated addresses as referring to variables?

MaskRay · 2023-11-02T18:20:21Z

The loadable kernel module files (.ko) are relocatable object files. Most functions have st_value==0. I am curious about the use case. DWARF needs relocating as well, but I haven't studied enough examples to tell how well it works.

If the .ko files are compiled with -ffunction-sections (not popular as CONFIG_LD_DEAD_CODE_DATA_ELIMINATION is off by default, and I think there is a lot of work to do), I suspect that both .symtab/DWARF will not work for the majority of cases.

MaskRay

.

quic-likaid · 2023-11-03T00:39:45Z

The option was unintentionally removed by 593e196, and remained as a no-op since 3d54976.

Got links to these commits? I can't find them at first glance, at least...

unintentional removal: https://reviews.llvm.org/D83530#inline-790089
no-op: https://reviews.llvm.org/D87067

So the issue is that if we use the ELF symbol table we can't differentiate functions from variables, but if we use the DWARF we can, and so we don't mistakenly symbolize unrelocated addresses as referring to variables?

Yes. It's the case we observed with Dynamically Loadable Kernel Modules.

MaskRay · 2023-11-03T03:18:50Z

no-op: reviews.llvm.org/D87067

https://android-review.googlesource.com/c/platform/ndk/+/1419436 landed 3 years ago. I think we can remove --use-symbol-table=true now.

Do you have an example how a false UseSymbolTable behaves differently?

quic-likaid · 2023-11-03T07:03:57Z

no-op: reviews.llvm.org/D87067

https://android-review.googlesource.com/c/platform/ndk/+/1419436 landed 3 years ago. I think we can remove --use-symbol-table=true now.

I can remove it if nobody objects

Do you have an example how a false UseSymbolTable behaves differently?

I just added a test to the commit. It prints b when use symbol table, and foo if not.

The test is somewhat artifitial though, here is an output from real world .ko, which motivated this PR (the module is too large to be shared here):

$ llvm-addr2line -afie qca_cld3_kiwi_v2.ko 0x3482c4
0x3482c4
__lim_process_sme_set_ht2040_mode
out/android14-6.1/msm-kernel/../vendor/qcom/opensource/wlan/qcacld-3.0/core/mac/src/pe/lim/lim_process_sme_req_messages.c:7981
global_dfs
out/android14-6.1/msm-kernel/../vendor/qcom/opensource/wlan/qcacld-3.0/core/mac/src/pe/lim/lim_process_sme_req_messages.c:9013

$ llvm-addr2line --no-use-symbol-table -afie qca_cld3_kiwi_v2.ko 0x3482c4
__lim_process_sme_set_ht2040_mode
out/android14-6.1/msm-kernel/../vendor/qcom/opensource/wlan/qcacld-3.0/core/mac/src/pe/lim/lim_process_sme_req_messages.c:7981
lim_process_sme_req_messages
out/android14-6.1/msm-kernel/../vendor/qcom/opensource/wlan/qcacld-3.0/core/mac/src/pe/lim/lim_process_sme_req_messages.c:9013

dwblaikie · 2023-11-06T04:49:44Z

I just added a test to the commit. It prints b when use symbol table, and foo if not.

And which behavior is it that the Dynamically Loadable Kernel Modules need? (I guess if you're restoring the behavior of the use-symbol-table=true case, that's the behavior you require?)

Or is @MaskRay's comments about changes addressed your needs & the use-symbol-table=true isn't needed by you/for dynamically loadable kernel modules?

quic-likaid · 2023-11-06T06:03:50Z

And which behavior is it that the Dynamically Loadable Kernel Modules need? (I guess if you're restoring the behavior of the use-symbol-table=true case, that's the behavior you require?)

For DLKM, --no-use-symbol-table is preferred, which is restored by the PR.

I'm not restoring use-symbol-table=true. It is for compatibility and is a no-op. Use symbol table is the default, and can't be disabled before this PR.

Or is @MaskRay's comments about changes addressed your needs & the use-symbol-table=true isn't needed by you/for dynamically loadable kernel modules?

I believe he is suggesting removing the use-symbol-table=true compatibility option.

dwblaikie · 2023-11-06T06:23:51Z

OK, so we're reintroducing the same functionality we had before (I assume we had it under --use-symbol-table={true,false}, but under a different flag name (--{no-,}use-symbol-table)? Seems like we might as well restore it under the same/old flag name? Not that I mind the more consistent/common spelling introduced here - just seems like some unnecessary breakage/incompatibility.

quic-likaid · 2023-11-06T09:58:22Z

This PR doesn't create new breakage by itself. Since https://reviews.llvm.org/D83530, we are using --no-feature instead of --feature=false. I'm following this convention.

dwblaikie · 2023-11-06T16:56:21Z

This PR doesn't create new breakage by itself. Since https://reviews.llvm.org/D83530, we are using --no-feature instead of --feature=false. I'm following this convention.

Yeah, just seems like a weird situation overall. No particular notes now, though.

I think we usually test from assembly (have the test assemble it with llvm-mc) rather than checked in object files - could you update this test to work that way? (leaving the source code and compilation command line as a comment in the asm file would be useful too, though). @jh7370 to confirm.

jh7370 · 2023-11-07T08:06:28Z

I think we usually test from assembly (have the test assemble it with llvm-mc) rather than checked in object files - could you update this test to work that way? (leaving the source code and compilation command line as a comment in the asm file would be useful too, though). @jh7370 to confirm.

Yes, prefer to avoid canned object files and use YAML/assembly/IR (roughly from most to least preferred) instead. This allows you to annotate the bits of the input that are specifically important for the test, so that future maintainers know what they should/should not change if updating tests, and also to see changes over time. It also avoids permanent binary blobs in the git history, which helps with the repo size (the first binary might sometimes be smaller than the input asm etc, but the cumulative effect of updates won't be).

Sections in relocatable ELFs have their `sh_addr` set to 0. This can confuse llvm-symbolizer when it tries to use symbol table to get function name. It may end up with a global variable in the bss section. This is observed when the symbolizer is used for Linux's dynamically loadable kernel modules. The option was unintentionally removed by 593e196, and remained as a no-op since 3d54976. Adding back the option allows us to prevent the undesired behaviour.

llvm/test/tools/llvm-symbolizer/no-use-symbol-table.s

jh7370 · 2023-11-08T08:57:26Z

@quic-likaid, please avoid force pushes unless you really need them, as it makes it harder to track the changes between versions of the PR. Instead, use fix up commits (they'll be squashed in when landing the change).

jh7370

Just a few remaining minor points from me.

@MaskRay, anything to add?

llvm/test/tools/llvm-symbolizer/use-symbol-table.s

jh7370

LGTM, in that I think this patch does what it sets out to do and conforms to LLVM standards, but please wait for further feedback from @MaskRay, as I have no opinions on whether we actually want this change.

MaskRay

This feature is low overhead and we used to have it, so LG. It at least gives us a way to test the symbolization behavior, so I am skeptical how much this can help symbolizing a relocatable object file.

llvmbot added the llvm:binary-utilities label Nov 2, 2023

jh7370 requested changes Nov 2, 2023

View reviewed changes

MaskRay requested changes Nov 2, 2023

View reviewed changes

quic-likaid force-pushed the no-use-symbol-table branch from 128db1a to 3e88cda Compare November 3, 2023 06:52

quic-likaid force-pushed the no-use-symbol-table branch from 3e88cda to 65e1664 Compare November 8, 2023 05:23

jh7370 reviewed Nov 8, 2023

View reviewed changes

update test

15465b4

jh7370 reviewed Nov 9, 2023

View reviewed changes

update test 2

cb4892f

jh7370 approved these changes Nov 14, 2023

View reviewed changes

MaskRay approved these changes Nov 14, 2023

View reviewed changes

bd1976bris mentioned this pull request May 16, 2024

[llvm-symbolizer] Improve output for addresses with no line correspondence #92403

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[llvm-symbolizer] restore --[no-]use-symbol-table option #71008

[llvm-symbolizer] restore --[no-]use-symbol-table option #71008

quic-likaid commented Nov 2, 2023

llvmbot commented Nov 2, 2023

jh7370 left a comment

dwblaikie commented Nov 2, 2023

MaskRay commented Nov 2, 2023

MaskRay left a comment

quic-likaid commented Nov 3, 2023 •

edited

MaskRay commented Nov 3, 2023

quic-likaid commented Nov 3, 2023

dwblaikie commented Nov 6, 2023

quic-likaid commented Nov 6, 2023

dwblaikie commented Nov 6, 2023

quic-likaid commented Nov 6, 2023

dwblaikie commented Nov 6, 2023

jh7370 commented Nov 7, 2023

jh7370 commented Nov 8, 2023

jh7370 left a comment

jh7370 left a comment

MaskRay left a comment

[llvm-symbolizer] restore --[no-]use-symbol-table option #71008

Are you sure you want to change the base?

[llvm-symbolizer] restore --[no-]use-symbol-table option #71008

Conversation

quic-likaid commented Nov 2, 2023

llvmbot commented Nov 2, 2023

jh7370 left a comment

Choose a reason for hiding this comment

dwblaikie commented Nov 2, 2023

MaskRay commented Nov 2, 2023

MaskRay left a comment

Choose a reason for hiding this comment

quic-likaid commented Nov 3, 2023 • edited

MaskRay commented Nov 3, 2023

quic-likaid commented Nov 3, 2023

dwblaikie commented Nov 6, 2023

quic-likaid commented Nov 6, 2023

dwblaikie commented Nov 6, 2023

quic-likaid commented Nov 6, 2023

dwblaikie commented Nov 6, 2023

jh7370 commented Nov 7, 2023

jh7370 commented Nov 8, 2023

jh7370 left a comment

Choose a reason for hiding this comment

jh7370 left a comment

Choose a reason for hiding this comment

MaskRay left a comment

Choose a reason for hiding this comment

quic-likaid commented Nov 3, 2023 •

edited