pping - Reduce verification complexity #47
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove the is_egress and ingress_ifindex from the parsing_context struct to the packet_info struct. Also change the member is_egress to is_ingress to better fit with the ingress_ifindex member. These members were only in parsing_context because they were convenient to fill in right from the start. However, it semantically makes little sense for the parsing_context to contain these because they are not used for any parsing, and they fit better with the packet_info. This also allows later functions (is_local_address(), pping_timestamp_packet() and pping_match_packet()) to get rid of their dependency on parsing_context, as it was only used for the is_egress and ingress_ifindex members (they do not do any parsing). After this change, parsing_context is only used for the initial parsing, and packet_info contains all the necessary data for all the functions related to the pping logic that runs after the packet has been parsed. Signed-off-by: Simon Sundberg <simon.sundberg@kau.se>
Do not provide pointers into the original packet from packet_info anymore (which the verifier has to ensure are valid), and instead directly parse all necessary data in parse_packet_identifier and then only use the parsed data in later functions. This allows a cleaner separation of concerns, where the parsing functions parse all necessary data from the packets, and other functions that need information about the packet only rely on the data provided in packet_info (and do not attempt to parse any data on their own). Signed-off-by: Simon Sundberg <simon.sundberg@kau.se>
Use global functions to make use of function-by-function verification. This allows the verifier to analyze parts of the program individually from each other, which should help reduce the verification complexity (the number of instructions the verifier must go through to verify the program) and help prevent exponentially growing with every loop or branch added to the code. In this case, break out the packet parsing (parse_packet_identifier) as a global function, so that it can be handled separately from the logic after it (updating flow state, saving timestamps, matching replies to timestamps, calculating and pushing RTTs) etc. To do this, create small separate wrapper functions (parse_packet_identifier_tc() and parse_packet_identifier_xdp()) for tc/xdp, so that the verifier can correctly identify the arguments as pointers to context (PTR_TO_CTX) when evaluating the global functions. Also create small wrapper functions pping_tc() and pping_xdp() which call the corresponding parse_packet_identifier_tc/xdp function. For this to work in XDP mode (which is the default), the kernel must have been patched with a fix that addresses an issue with how global functions are verified for XDP programs, see: https://lore.kernel.org/all/20220606075253.28422-1-toke@redhat.com/ Signed-off-by: Simon Sundberg <simon.sundberg@kau.se>
tohojo
approved these changes
Jun 21, 2022
Member
tohojo
left a comment
tohojo
left a comment
There was a problem hiding this comment.
Nice set of cleanups! Doesn't really matter if the improvement comes from the use of global functions, or just generally from better code, does it? Either way it's a win; nice work!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR tries to reduce the verification complexity (number of instructions it takes the verifier to verify the program) by breaking out parts of the program (specifically the packet parsing) into global functions that can be verified independently from the rest of the code.
The first commit just moves some struct members from the generic
parsing_contexstruct to thepacket_infostruct, partly because it semantically fits better, and partly to better separate the packet parsing functions from functions that operate on the information from the parsed packet. This will make it easier to break out the packet parsing into a global function later, as other functions will no longer require access to theparsing_contextanymore.The second commit removes some packet pointers in
packet_infoand instead adds a couple of fields (ip_lenandip_tos) that the FIB lookup needs. So instead of later retrieving relevant packet pointers and parse the necessary information to do a FIB lookup, parse the required information directly as part of the packet parsing stage and then simply get it frompacket_infowhen doing the FIB lookup.The third commit actually breaks out the packet parsing into global functions (one for tc and one for XDP), and this commit will require Toke's kernel fix to work if using the XDP ingress hook.
While this PR does sucessfully lower the verification complexity (and makes it possible to run it when compiled with LLVM-14 on kernels > 5.15 without hitting the 1m limit), it's a bit unclear how much of it can really be attributed to using global functions and how much of it is simply from restructuring the code. If one inlines the global functions, it takes ~302k insn to verify the program if compiled with LLVM-13 and ~301k if compiled with LLVM-14. But if called as global functions (as is done here), it takes ~231k (plus 228k for the global function) with LLVM-13 and ~221k (plus ~218k for the global function) with LLVM-14. So while the global functions do help reduce the maximum insn somewhat, total verification time will likely be a bit higher due to having to verify both the global function and the program (which added together takes more instructions).