PPing minor userspace fixes #57
Merged
Conversation
This file contains 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
tohojo
requested changes
Oct 10, 2022
simosund
force-pushed
the
pping-minor-userspace-fixes
branch
from
2ee87a1
to
7830efd
Compare
|
So, I've expanded this PR a bit. A brief overview of the commits as of now:
|
|
The changes LGTM, but seems the PR needs a rebase... |
The userspace loader would only check if the tc clsact was created when the egress program was loaded. Thus, if the ingress program created the clsact the egress program would not have to create the clsact, the ePPing would thus falsely believe it did not create a clsact and fail to remove it on shutdown even if --force was used. Fix this by checking if either ingress or egress created clsact. This bug was introduced as a sneaky side effect of commit 78b45bd (pping: Use libxdp to load and attach XDP program). Before this commit the egress program (for which there is only a tc alternative) would be loaded first, and thus it was sufficient to check if it created the clsact. When switching to libxdp however, the ingress program (specifically the XDP program) had to be loaded first, and thus the order of loading ingress and egress program were swapped. Therefore, it was no longer sufficient to only check the egress program as the tc ingress program may have created the clsact before the the egress program is attached (and only checking the ingress program would also not be enough as the tc ingress program may never be loaded if XDP mode is used instead). Signed-off-by: Simon Sundberg <[email protected]>
Define the BPF program names in the user space component. The strings corresponding to the BPF program names were before inserted in several places, including in multiple string comparison, which is error prone and could leave to subtle errors if the program names are changed and not updated correctly in all places. With the program name string being defined, they only have to be changed in a single place. Currently only the names of the ingress programs occur in multiple places, but also define the name for the egress program to be consistent. Note that even after this change one has the sync the defined values with the actual program names declared in the pping_kern.c file. Ideally, these would all be defined in a single place, but not aware of a convenient way to make that happen (cannot use the defined strings as function names as they are not identifiers, and if defined as identifiers instead it would not be possible to use them as strings). Signed-off-by: Simon Sundberg <[email protected]>
Using the XDP ingress hook requires a newer kernel (needs Toke's patch fixing the verification of global function for BPF_PROG_TYPE_EXT programs) than tc mode, is will likely perform worse than tc if running in generic mode (due to no driver support for XDP). Furthermore, even when XDP works and has driver support, its performance benefit over tc is likely small as the packets are always passed on to the network stack regardless (not creating a fast-path that bypasses the network stack). Therefore, use the tc ingress hook as default instead, and only use XDP if explicitly required by the user (-I/--ingress hook xdp). This partly addresses issue xdp-project#49, as ePPing should no longer by default get the confusing error message from failing verification if the kernel lacks Toke's verifier patch. Signed-off-by: Simon Sundberg <[email protected]>
Add an option to let the user configure which mode to load the XDP program in (unspecified, native or generic). Set the default mode to native (was unspecified previously) as that is what the user most likely wants to use (generic or unpsecified falling back on generic will likely have worse performance). Signed-off-by: Simon Sundberg <[email protected]>
Due to a kernel bug for XDP programs loaded via libxdp that use global functions (see https://lore.kernel.org/bpf/[email protected]/t/), XDP mode only works on relatively recent kernels where the bug is patched (or kernels where the patch has been backported). As many users may not have such a recent kernel they will only see a confusing verifier error like the following: Starting ePPing in standard mode tracking TCP on test123 libbpf: elf: skipping unrecognized data section(7) xdp_metadata libbpf: elf: skipping unrecognized data section(7) xdp_metadata libbpf: prog 'pping_xdp_ingress': BPF program load failed: Invalid argument libbpf: prog 'pping_xdp_ingress': -- BEGIN PROG LOAD LOG -- Func#1 is safe for any args that match its prototype Validating pping_xdp_ingress() func#0... 0: R1=ctx(id=0,off=0,imm=0) R10=fp0 ; int pping_xdp_ingress(struct xdp_md *ctx) 0: (bf) r6 = r1 ; R1=ctx(id=0,off=0,imm=0) R6_w=ctx(id=0,off=0,imm=0) 1: (b7) r7 = 0 ; R7_w=invP0 ; struct packet_info p_info = { 0 }; 2: (7b) *(u64 *)(r10 -8) = r7 ; R7_w=invP0 R10=fp0 fp-8_w=00000000 3: (7b) *(u64 *)(r10 -16) = r7 ; R7_w=invP0 R10=fp0 fp-16_w=00000000 4: (7b) *(u64 *)(r10 -24) = r7 ; R7_w=invP0 R10=fp0 fp-24_w=00000000 5: (7b) *(u64 *)(r10 -32) = r7 ; R7_w=invP0 R10=fp0 fp-32_w=00000000 6: (7b) *(u64 *)(r10 -40) = r7 ; R7_w=invP0 R10=fp0 fp-40_w=00000000 7: (7b) *(u64 *)(r10 -48) = r7 ; R7_w=invP0 R10=fp0 fp-48_w=00000000 8: (7b) *(u64 *)(r10 -56) = r7 ; R7_w=invP0 R10=fp0 fp-56_w=00000000 9: (7b) *(u64 *)(r10 -64) = r7 ; R7_w=invP0 R10=fp0 fp-64_w=00000000 10: (7b) *(u64 *)(r10 -72) = r7 ; R7_w=invP0 R10=fp0 fp-72_w=00000000 11: (7b) *(u64 *)(r10 -80) = r7 ; R7_w=invP0 R10=fp0 fp-80_w=00000000 12: (7b) *(u64 *)(r10 -88) = r7 ; R7_w=invP0 R10=fp0 fp-88_w=00000000 13: (7b) *(u64 *)(r10 -96) = r7 ; R7_w=invP0 R10=fp0 fp-96_w=00000000 14: (7b) *(u64 *)(r10 -104) = r7 ; R7_w=invP0 R10=fp0 fp-104_w=00000000 15: (7b) *(u64 *)(r10 -112) = r7 ; R7_w=invP0 R10=fp0 fp-112_w=00000000 16: (7b) *(u64 *)(r10 -120) = r7 ; R7_w=invP0 R10=fp0 fp-120_w=00000000 17: (7b) *(u64 *)(r10 -128) = r7 ; R7_w=invP0 R10=fp0 fp-128_w=00000000 18: (bf) r2 = r10 ; R2_w=fp0 R10=fp0 ; 19: (07) r2 += -128 ; R2=fp-128 ; if (parse_packet_identifer_xdp(ctx, &p_info) < 0) 20: (85) call pc+13 R1 type=ctx expected=fp Caller passes invalid args into func#1 processed 206542 insns (limit 1000000) max_states_per_insn 32 total_states 13238 peak_states 792 mark_read 40 -- END PROG LOAD LOG -- libbpf: failed to load program 'pping_xdp_ingress' libbpf: failed to load object 'pping_kern.o' Failed attaching ingress BPF program on interface test123: Invalid argument Failed loading and attaching BPF programs in pping_kern.o To help users that run into this issue when loading the program in generic or unspecified mode, add a small hint suggesting to upgrade the kernel or use the tc ingress mode instead in case attaching the XDP program fails. However, if loaded in native mode, instead give the suggestion to try loading in generic mode instead. While libbpf and libxdp already add some messages hinting at this, this hint clarifies how to do this with ePPing (using the --xdp-mode argument). Signed-off-by: Simon Sundberg <[email protected]>
Signed-off-by: Simon Sundberg <[email protected]>
Previously the program would only print out an error message if the cleanup of a map failed, and then keep running. Each time the periodical cleanup failed the error message would be repeated, but no further action taken. Change this behavior so that the program instead terminates the cleanup thread and aborts the rest of the program. Signed-off-by: Simon Sundberg <[email protected]>
simosund
force-pushed
the
pping-minor-userspace-fixes
branch
from
7830efd
to
91d7242
Compare
tohojo
approved these changes
Nov 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A few unrelated minor fixes to the userspace component of ePPing. Threw them together in the same PR to avoid lots of tiny PRs.
bfp_object__close()on shutdown/error. I'm not sure if this is really needed when the program ends directly after anyways, but most recent examples seem to do it so figured I'd better do it as well in case it cleans up some resources that's not automatically freed by the kernel when the program ends.As for the second commit, I'm not sure if it's the best way to address #49. Simone suggested to automatically try to load and attach the tc ingress program in case the XDP one fails. While this should be possible (although a bit more of a hassle) and in some cases quite convenient, I'm not sure if automatically changing user-specified arguments is a good idea (even if it's not done silently) as it might be easy to miss unless paying close attention to stderr, especially if ePPing is started via ex. a script.
A somewhat related discussion is if we should perhaps always try to load the XDP program in native mode (
XDP_MODE_NATIVE), instead of in unspecified mode (XDP_MODE_UNSPEC) which will (silently) fall back on generic XDP if there's no driver support. From some old discussions with Jesper I got the impression that the performance of XDP generic is rather lackluster, and it probably being better to run with tc than with generic XDP. If that's the case I'm not sure if it makes any sense to allow running in XDP generic mode (and also being unclear to the user whether it's is running in native or generic).Finally, should XDP mode really be the default? It's less likely to work (due to requiring a more recent kernel), does (still not?) work with ex. GRO and may offer worse performance than tc if running in generic mode. Even if it's running in native XDP mode do we really expect it to have much of a performance benefit here anyways? As ePPing only monitors the packets it's always letting all packets through to the kernel anyways (always return
XDP_PASS), so we don't really benefit from being able to create a fastpath that bypasses the kernel like some other XDP use cases might. So perhaps it makes more sense to have tc as the default instead (which can be changed to XDP by passing -I/--ingress-hook xdp)?