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

feat(s2n-quic-xdp): add syscall module #1704

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Apr 11, 2023

Description of changes:

This PR adds a syscall module which contains all of the required interactions for AF_XDP sockets. I've tried to link to all of the relevant parts in libxdp. I also implemented a small mmap abstraction that cleans itself up on drop to make it easier to test some of the syscalls.

Call-outs:

The syscalls are relatively low level, still. Higher-level abstractions are in the works.

Testing:

I updated the xtask ci script to include tests that run under sudo, which has permission to open raw sockets.

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

@camshaft camshaft marked this pull request as ready for review April 12, 2023 00:13
tools/xdp/s2n-quic-xdp/src/syscall.rs Show resolved Hide resolved
tools/xdp/s2n-quic-xdp/src/syscall.rs Show resolved Hide resolved
tools/xdp/s2n-quic-xdp/src/syscall.rs Show resolved Hide resolved
return;
};

// the ring sizes need to be a power of 2
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be enforced in the set_*_ring_size functions?

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 enforced by the kernel when you call those functions. You get a invalid input error if it's not.

tools/xdp/s2n-quic-xdp/src/syscall.rs Show resolved Hide resolved
Comment on lines +73 to +80
let status = Command::new("sudo")
.arg(Path::new("../").join(path))
.arg("--nocapture")
.current_dir("s2n-quic-xdp")
.env("CAP_NET_RAW_ENABLED", "1")
.status()
.expect("failed to run test case");
assert!(status.success());
Copy link
Contributor

Choose a reason for hiding this comment

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

So our CI can support sudo and AF_XDP? Also is this command missing a cargo test? Not sure what its running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you can see the CI task is passing with this. It doesn't need a cargo test because it's invoking the test binary directly. Running sudo cargo test makes a mess of permissions and ends up fetching a new registry and recompiling the whole thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

WARNING: CAP_NET_RAW capabilities missing; skipping syscall_test

I dont think its being tested in our CI atm.

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 is. First it tests it without. Then it runs it with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 normal tests first
test()?;
// run the CAP_NET_RAW tests after
cap_net_raw_tests()?;

@camshaft camshaft merged commit 8d5f444 into main Apr 12, 2023
@camshaft camshaft deleted the camshaft/xdp-syscalls branch April 12, 2023 19:20
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.

2 participants