Add SymbolIndex and kallsyms helpers

[brenns10]

This is the next step of the DWARFless debugging / CTF feature, #176. Unlike the symbol finder, there aren't too many questions of API, instead we're just implementing the API. This PR adds:

• The SymbolIndex type, which holds an immutable list of symbols and indexes them for efficient name and address based lookup. Some interesting items here:
  • It uses a hash for name lookup, and an interesting binary search approach for address lookup, in order to support duplicated names and overlapping address ranges.
  • I chose SymbolIndex over SymbolTable since I didn't want to overload the common usage of that term applying to ELF symbol tables. That said, SymbolIndex is a bit strange so I'm open to suggestions.
• Two C helpers (load_proc_kallsyms() and load_builtin_kallsyms()) enumerate all kallsyms symbols and return a SymbolIndex object. The first uses /proc/kallsyms and the second uses information from VMCOREINFO to find the kallsyms tables.
  • The proc version lets you specify a filename, in case you happen to have a copy of /proc/kallsyms from a vmcore.
  • /proc/kallsyms includes module symbols, while the built-in vmlinux kallsyms of course does not. I've included an option to parse these symbols, however by default, we do not. This is because you can actually get the ELF symbol objects, which have more information, directly from the struct module.
• A Python helper (load_module_kallsyms()) for loading module symbols from the kernel image and creating a symbol index from that. This does require debuginfo for vmlinux.

I also of course have tests for the above.

Below are two issues you'll probably notice and want to discuss:

1. The guess_long_names() function works around torvalds/linux@73bbb94466fd ("kallsyms: support "big" kernel symbols") which was merged in 6.1. I didn't catch it immediately so I'm not sure whether there's any use in sending a commit to include a version number for kallsyms... Especially since I believe it's unlikely to be backported, but I could be wrong. It has certainly not been backported to stable kernels.
2. The final commit of the branch adds an optimization where we can mark symbols as having a lifetime. Symbols with static lifetimes are not freed by drgn_symbol_destroy(). This enables the SymbolIndex to provides symbols with zero copying, even all the way out to the Python layer! However, I'm not certain it's worth the complexity it adds. That's why I kept the change as a second commit; it's easy to drop if you don't like it. I haven't measured any performance impact and admittedly, I don't know if it would be noticeable.

[brenns10]

Alright, sorry for the failure (it was an old commit changing how ELF symbols are stored in struct mod_kallsyms). I had been hoping for a clean bill of health on the first try :D
But it's ready now, excited to take the new symbol finder API out for a spin :D

[brenns10]

These latest pushes rebase on the current main branch, with the updated symbol finder API.

[osandov]

I've only looked at the first two commits so far, so feel free to ignore these comments until I'm done or address them now, up to you. I'll continue reviewing tomorrow.

[osandov]

Okay, I finally made it through the whole series. Good stuff! I can't claim that I fully understood the kallsyms format this time around, but I at least tried to look for coding errors.

The guts of the implementation look good, but the drgn.helpers.linux.kallsyms interface is a bit overwhelming. There are lots of similar-looking helpers that I wouldn't know how to make use as an end user. I wonder if we can pare it down a bit. What are the most important use cases that you're targeting?

[osandov]

Can we return without ever setting this, leaving the caller with uninitialized garbage?

[brenns10]

Yes, I think that's worth guarding against. I think the easiest place to do it is actually in drgn_load_builtin_kallsyms(), where we can simply check whether the string builder length is zero and raise an error.

There's a lot of stuff in the kallsyms tables that, if fuzzed, could cause these parsers to behave weirdly or maybe crash. I'm not certain how paranoid we want to be here. At this point, if the correct set of symbols are provided, with real values, and we've managed to read the token and name table successfully (without faulting or mallocing a buffer that's way too large), then it seems likely that the data will match the kallsyms format. So in practical terms, these sorts of things shouldn't happen. What are your opinions here? I can add paranoia to this, but it does feel like it will take rather unclear code and make it even worse...

[osandov]

There's a lot of stuff in the kallsyms tables that, if fuzzed, could cause these parsers to behave weirdly or maybe crash.

The general philosophy in drgn is that bogus input can produce bogus results, but it should never cause drgn to crash or invoke undefined behavior. That does tend to require lots of extra bounds checking and overflow checking. I'll keep an eye out for that as I review the kallsyms code.

[brenns10]

Thanks for the review! I'll take a look through it in detail and try to get an update ASAP, rebased on main as well. It looks like you made a lot of good catches, so thanks for that.

The guts of the implementation look good, but the drgn.helpers.linux.kallsyms interface is a bit overwhelming. There are lots of similar-looking helpers that I wouldn't know how to make use as an end user. I wonder if we can pare it down a bit. What are the most important use cases that you're targeting?

Yeah, I have even confused myself... I've occasionally imported from there and not known which function I want to use. So it does need to be simplified.

The primary use cases I can think of are:

1. The full non-DWARF approach where you use kallsyms, CTF || BTF, and ORC or frame pointers for unwinding. In this case, you need to do a delicate dance: first load vmlinux kallsyms, then the types (you need the symbol table to find the BTF, though CTF doesn't require the symbol table first), then the module kallsyms (since module kallsyms requires vmlinux types). In the final CTF branch, there's a simple drgn.helpers.linux.ctf:load_ctf() function that just does everything for you in the right order, so most of the time nobody would need to think about it.
2. The partial DWARF approach, where you have vmlinux DWARF loaded, but you didn't extract all the modules. This actually happens all the time for us internally, because we typically extract only the debuginfo we need. In that case, it's very nice to be able to add a symbol finder for the kernel modules, so that symbolization works as expected without loading the module debuginfo.

That would mean at a minimum, we'd really just need load_vmlinux_kallsyms() and load_module_kallsyms(). I don't think the load_builtin_kallsyms() and load_proc_kallsyms() functions are really necessary to be part of the API (though keeping them as private/internal functions would be nice, because I could see myself wanting to use them hackily).

The module_kallsyms() function is exceptionally poorly named. I included this as a public function because I could imagine a world where a user may have some kernel modules' DWARF debuginfo loaded, but they want to load the other modules' symbols. They could use this function to get that subset of symbols. Or, alternatively, I could see it being useful for a user to simply ask: what symbols are part of this kernel module? But to sum up, I don't have a single great use case for this function. I wouldn't be disappointed to scrap it from the API, or at a minimum, it should be renamed to something clearer (e.g. get_module_symbols()).

[brenns10]

Ok after a few botched pushes I think I've addressed everything here. I've also rebased on main to drop my drgn.helpers.linux.module changes which conflicted with #411. I did leave open the comments with more substantial replies in case you want to continue those discussions, though I think I addressed those too. Thanks!

[brenns10]

Noticed that the branch had become stale due to some merge conflicts.

This first push here has just resolved the conflicts. Got a lint error (git rebase does not run pre-commit :/). I still need to incorporate your new binary search macro, so I'll resolve both shortly.

[brenns10]

I'm loving the binary search helpers, they simplified things a lot here:

diff --git a/libdrgn/symbol.c b/libdrgn/symbol.c
index 46e21a6b..92bcfdb9 100644
--- a/libdrgn/symbol.c
+++ b/libdrgn/symbol.c
@@ -6,6 +6,7 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "binary_search.h"
 #include "drgn_internal.h"
 #include "string_builder.h"
 #include "symbol.h"
@@ -300,38 +301,21 @@ drgn_symbol_index_deinit(struct drgn_symbol_index *index)
 static void address_search_range(struct drgn_symbol_index *index, uint64_t address,
 				 uint32_t *start_ret, uint32_t *end_ret)
 {
-	// bsearch() only handles exact matches, and we need to find what is
-	// essentially the correct insertion point, even if there's not an exact
-	// match. So bsearch() can't be used here. It's helpful to think of this
-	// code in terms of the Python library functions bisect_right().
-	uint32_t lo = 0, hi = index->num_syms;
-	uint32_t mid;
-
 	// First, identify the maximum symbol index which could possibly contain
 	// this address. Think of this as:
 	//   end_ret = bisect_right([s.address for s in symbols], address)
-	while (lo < hi) {
-		mid = lo + (hi - lo) / 2;
-		if (address < index->symbols[mid].address)
-			hi = mid;
-		else
-			lo = mid + 1;
-	}
-	*end_ret = lo;
+	#define less_than_start(a, b) (*(a) < (b)->address)
+	*end_ret = binary_search_gt(index->symbols, index->num_syms, &address,
+				    less_than_start);
+	#undef less_than_start
 
 	// Second, identify first symbol index which could possibly contain this
 	// address. We need to use "max_addrs" for this task:
 	//    bisect_right(max_addrs, address)
-	hi  = lo;
-	lo = 0;
-	while (lo < hi) {
-		mid = lo + (hi - lo) / 2;
-		if (address < index->max_addrs[mid])
-			hi = mid;
-		else
-			lo = mid + 1;
-	}
-	*start_ret = lo;
+	#define less_than_end(a, b) (*(a) < *(b))
+	*start_ret = binary_search_gt(index->max_addrs, index->num_syms, &address,
+				    less_than_end);
+	#undef less_than_end
 }
 
 struct drgn_error *

[osandov]

Signing off for the day after looking at commits 1-3. Will continue tomorrow, thanks!

[osandov]

There's a lot of stuff in the kallsyms tables that, if fuzzed, could cause these parsers to behave weirdly or maybe crash.

The general philosophy in drgn is that bogus input can produce bogus results, but it should never cause drgn to crash or invoke undefined behavior. That does tend to require lots of extra bounds checking and overflow checking. I'll keep an eye out for that as I review the kallsyms code.

[osandov]

Okay, that's the rest of it, looking good overall. I gave the kallsyms code a much closer read than I had before, so there are some new comments there that I missed in the last round. Have a good weekend!

[brenns10]

I double checked to ensure the s390x failure was transient, so we've got a clean test run :)

I believe I have addressed all your comments, and I avoided rebasing on the main so the diff between versions stays clean. Thanks for the review again!

[osandov]

Yeah Packit has been flaky today. I'll give these another look today!

[osandov]

Thanks, just need a few final easy fixes.

[osandov]

(LGTM other than the u64_converter() comment)

[osandov]

Thanks for all of your work on this, it looks ready to go 🎉

[brenns10]

Yay, that's great! Thanks for all the reviewing work on this!