Skip to content
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

tools(xdp): add bpf program #1652

Merged
merged 5 commits into from
Mar 13, 2023
Merged

tools(xdp): add bpf program #1652

merged 5 commits into from
Mar 13, 2023

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Mar 2, 2023

Description of changes:

This change adds a default XDP eBPF program to be loaded for the s2n-quic-xdp crate (coming soon). Since BPF programs are portable, the compiled program is committed to the repository to improve the ease of development.

Testing:

I added a CI task that

  • builds the BPF programs
  • disassembles them and performs a diff on the previously committed programs to ensure they are up-to-date
  • loads them into the kernel which will verify them as valid BPF programs

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@camshaft camshaft force-pushed the camshaft/xdp-ebpf branch from 137e9f2 to d093a61 Compare March 3, 2023 18:24
@camshaft camshaft changed the title XDP ebpf tools(xdp): add bpf program Mar 3, 2023
@camshaft camshaft force-pushed the camshaft/xdp-ebpf branch 2 times, most recently from 379cc14 to 0a14401 Compare March 3, 2023 18:42
@camshaft camshaft marked this pull request as ready for review March 3, 2023 21:48
{
use aya_log_ebpf as log;
match action {
xdp_action::XDP_DROP => log::trace!(&ctx, "ACTION: DROP"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I guess this log is stored in a map? Or emitted to a file via bpf_trace_printk

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses a pair of maps: https://github.com/aya-rs/aya/blob/main/bpf/aya-log-ebpf/src/lib.rs.

But note that this isn't enabled by default and is really only for debugging.

use std::io;

pub fn run() -> Result<(), anyhow::Error> {
let before = dump()?;
Copy link
Contributor

@jon-chuang jon-chuang Mar 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, where is the before build from? Are we assuming the resultant BPF bytecodes should always be identical to that of the LFS files committed in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this check is to ensure the LFS build is in sync with what the code says. I didn't want to diff the actual binary since there can be some nondeterminism so we're just diffing the disassembly output, which should be good enough for this purpose.


- uses: camshaft/rust-cache@v1

- name: Build ebpf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we only test that our build is identical and that we can load the program, but we do not yet do an e2e test that the XDP BPF module correctly routes the packets?

Will we do so subsequently by sending in malformed packets and show they are not received, while the valid ones are?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The XDP IO provider will be coming soon. This PR is just focused on the default BPF program that ships with said IO provider.

working-directory: tools/xdp
env:
RUST_LOG: trace
run: cargo xtask ci
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does xtask do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

## Run the kernel verifier

```bash
RUST_LOG=trace cargo xtask run -- -i lo --trace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you explain some of the command line options (-i lo) in the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

@@ -0,0 +1,6 @@
[toolchain]
# pin the version to prevent random breakage
channel = "nightly-2023-02-20"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do more recent versions not work, or is this just when you started this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's when I started it. I can update if you want.

Comment on lines +62 to +63
let not_found_action = xdp_action::XDP_PASS as _;
SOCKETS.redirect(queue_id, not_found_action)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this not_found_action? why is this considered not found?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's defining what to do if the entry isn't in the map. So if there isn't a socket associated with the queue index, we'll forward on to the OS.

impl Validator for PortValidator {
#[inline(always)]
fn validate_local_port(&self, port: u16) -> bool {
// The destination isn't in the port map so forward it to the OS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this more saying "If the destination isn't in the port map, forward it to the OS"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think I copy/pasted this comment from when I was actually doing a if. I'll fix it.

Comment on lines +80 to +87
#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! {
unsafe { core::hint::unreachable_unchecked() }
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's needed when you build a no_std binary. But the code should be panic free so it's not actually used in the end.

@camshaft camshaft merged commit e3a9009 into main Mar 13, 2023
@camshaft camshaft deleted the camshaft/xdp-ebpf branch March 13, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants