-
Notifications
You must be signed in to change notification settings - Fork 199
crash: add bpf command for displaying eBPF programs and maps #559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
crash: add bpf command for displaying eBPF programs and maps #559
Conversation
|
@osandov, following up on this PR just in case if you haven't had a chance to look at this PR yet. |
|
Thanks again for working on this! I'm wrapping up a few things before cutting a new release, so I won't be able to take a good look until Friday at the earliest, but I haven't forgotten about it. |
|
Thanks for the heads up @osandov. Sorry if I bugged you. |
osandov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, Suchit! Thanks so much for working on this. A couple of high-level comments:
- The object-oriented style isn't idiomatic for drgn, which tends to follow a procedural style in order to follow the C kernel code more. I think you mostly inherited that from the
contrib/bpf_inspect.pyscript, but that script wasn't typical either. Specifically, I'd prefer for the more complex methods to be helper functions indrgn.helpers.linux.bpfthat operate directly ondrgn.Objects (e.g.,BpfProg.get_used_maps()->bpf_prog_used_maps()or something like that). The simpler methods that only format things can just be folded directly into_crash_cmd_bpf(). You can add those helpers as separate PRs or as separate commits in this PR, your choice. - The
--drgnoption is the main motivation for doing this porting work, so it'd be nice for the initial version of this command to support it. Adding support for that will likely make it clear what should be added as a helper.
I left a couple of minor comments inline, too.
Let me know if you need any clarification or pointers!
drgn/commands/_builtin/crash/_bpf.py
Outdated
| from drgn.helpers.linux import ( # type: ignore[attr-defined] | ||
| bpf_map_for_each, | ||
| bpf_prog_for_each, | ||
| hlist_for_each_entry, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import these from the specific submodules where they're defined (drgn.helpers.linux.bpf and drgn.helpers.linux.list) to avoid the need for the type: ignore.
drgn/commands/_builtin/crash/_bpf.py
Outdated
| print( | ||
| f"{'ID':>4} {'BPF_PROG':<18} {'BPF_PROG_AUX':<18} {'BPF_PROG_TYPE':<18} " | ||
| f"{'TAG':<18} {'USED_MAPS'}" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer use of drgn.helpers.common.format.print_table() rather than manual formatting. There are lots of example in drgn/commands.
|
Hi @osandov. Thanks for the comments. I’ll make the required changes and let you know once they’re done. Apologies for the inconvenience. |
187ac4e to
d828cc7
Compare
|
Hi @osandov. Sorry for the delay. I've rewritten the code according to your comments. Could you please review it? Apologies in advance if I did mistakes again. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much closer to what I had in mind, thanks! I left some inline comments about formatting things more like crash and little tricks/cleanups.
Some high-level things:
--drgnstill needs to be implemented. Search forargs.drgnindrgn/commands/_builtin/crashfor some examples.- This needs a test case. I implemented a test for the
bpf_prog_used_maps()helper in mybpf-prog-used-mapsbranch. The crash command test cases tend to be pretty basic sanity checks, seetests/linux_kernel/crash_commandsfor examples. I think a test case for this command could simply create a BPF program and map just liketest_bpf_prog_used_maps()and then check that the command prints at least that program and map. - When this is ready to be merged, just like when contributing to the kernel, I'd prefer for the commit history to be cleaned up so that there's one commit adding the helper and another adding the command (rather than the development history with fixups and merges).
|
Hi @osandov. Extremely sorry for the delay. I got a bit busy with work and some other stuff. I'll try to address your comments as early as possible. Thanks and sorry for the inconvenience. |
741d9e7 to
2ebbe80
Compare
Introduce a new crash command 'bpf' to list eBPF programs and maps from the kernel. The command enumerates over all loaded BPF programs and maps, showing each program’s ID, addresses, type, tag, and the maps it uses. Signed-off-by: Suchit Karunakaran <[email protected]>
Refactor BPF inspection logic to follow drgn's procedural style rather than an object-oriented one. Complex methods previously implemented as class methods are now standalone helper functions. Also replace manual table formatting in _crash_cmd_bpf() with print_table() for consistency with other commands in the drgn codebase. Signed-off-by: Suchit Karunakaran <[email protected]>
Refactor drgn BPF helpers by removing bpf_prog_trampoline_progs() and bpf_prog_subprogs() to streamline the API. Update the _crash_cmd_bpf() command to consistently format BPF program and map information, display addresses in hexadecimal, and support drgn code generation in interactive mode. Enhance the test suite by adding instruction constants and helper functions in tests/linux_kernel/bpf.py, and introduce a dedicated crash command test that verifies BPF program and map output, including tags and map IDs. Signed-off-by: Suchit Karunakaran <[email protected]>
201cfbf to
b31991f
Compare
osandov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than cleaning up the commits, some linter errors in the CI, and a couple of minor comments below, this looks pretty much ready, thank you!
| """\ | ||
| for bpf_prog in bpf_prog_for_each(prog): | ||
| prog_id = bpf_prog.aux.id.value_() | ||
| prog_type_name = bpf_prog.type.format_(type_name=False).split("BPF_PROG_TYPE_")[ | ||
| -1 | ||
| ] | ||
| tag = bpf_prog.tag | ||
| prog_tag = "".join(f"{b.value_():02x}" for b in tag) | ||
| used_maps = [] | ||
| for map in bpf_prog_used_maps(bpf_prog): | ||
| used_maps.append(map.id.value_()) | ||
| used_maps_str = ",".join(str(m) for m in used_maps) | ||
| for bpf_map in bpf_map_for_each(prog): | ||
| map_id = bpf_map.id.value_() | ||
| map_type_name = bpf_map.map_type.format_(type_name=False).split("BPF_MAP_TYPE_")[-1] | ||
| try: | ||
| map_flags = f"{bpf_map.map_flags.value_():08x}" | ||
| except LookupError: | ||
| map_flags = "00000000" | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry about any of the formatting stuff in the --drgn output; it's good enough to fetch the fields. Something like this:
| """\ | |
| for bpf_prog in bpf_prog_for_each(prog): | |
| prog_id = bpf_prog.aux.id.value_() | |
| prog_type_name = bpf_prog.type.format_(type_name=False).split("BPF_PROG_TYPE_")[ | |
| -1 | |
| ] | |
| tag = bpf_prog.tag | |
| prog_tag = "".join(f"{b.value_():02x}" for b in tag) | |
| used_maps = [] | |
| for map in bpf_prog_used_maps(bpf_prog): | |
| used_maps.append(map.id.value_()) | |
| used_maps_str = ",".join(str(m) for m in used_maps) | |
| for bpf_map in bpf_map_for_each(prog): | |
| map_id = bpf_map.id.value_() | |
| map_type_name = bpf_map.map_type.format_(type_name=False).split("BPF_MAP_TYPE_")[-1] | |
| try: | |
| map_flags = f"{bpf_map.map_flags.value_():08x}" | |
| except LookupError: | |
| map_flags = "00000000" | |
| """ | |
| """\ | |
| for bpf_prog in bpf_prog_for_each(): | |
| aux = bpf_prog.aux | |
| prog_id = aux.id | |
| prog_type = bpf_prog.type | |
| tag = bpf_prog.tag | |
| for used_map in bpf_prog_used_maps(bpf_prog): | |
| map_id = used_map.id | |
| for bpf_map in bpf_map_for_each(): | |
| map_id = bpf_map.id | |
| map_type = bpf_map.map_type | |
| try: | |
| map_flags = bpf_map.map_flags | |
| except AttributeError: | |
| # map_flags is only available since Linux 4.6. | |
| pass | |
| """ |
| # may not have this field, so we catch LookupError and default to 0. | ||
| try: | ||
| map_flags = f"{bpf_map.map_flags.value_():08x}" | ||
| except LookupError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be AttributeError:
| except LookupError: | |
| except AttributeError: |
Introduce a new crash command 'bpf' to list eBPF programs and maps from the kernel. The command enumerates over all loaded BPF programs and maps, showing each program’s ID, addresses, type, tag, and the maps it uses.