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

refactor(s2n-quic-platform): move socket config to separate module #1737

Merged
merged 1 commit into from
May 2, 2023

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Apr 28, 2023

Description of changes:

While implementing the AF_XDP IO provider, I need a way to create a configured UDP socket outside of the tokio IO provider to reserve the port for the endpoint.

This change moves all of the syscalls out of the tokio IO provider into a separate module.

Call-outs:

While I was here, I realized that we were allowing MTU probing to occur even on platforms where we can't guarantee the DF bit is set. I've gone ahead and added logic to set the max MTU to the minimum value in these cases:

if !syscall::configure_mtu_disc(&tx_socket) {
// disable MTU probing if we can't prevent fragmentation
max_mtu = MaxMtu::MIN;
}

Testing:

Since this is just extracting function calls into a separate module, the existing tests should cover the differences. I've opened an issue to implement a test that the MTU sizes are correct on non-linux platforms: #1739.

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/platform-syscalls branch 2 times, most recently from cba69de to f1beb3d Compare May 1, 2023 16:11
@camshaft camshaft marked this pull request as ready for review May 1, 2023 16:19
@camshaft camshaft force-pushed the camshaft/platform-syscalls branch from f1beb3d to 6022be0 Compare May 2, 2023 18:01
Comment on lines +156 to +172
success |= libc!(setsockopt(
rx_socket.as_raw_fd(),
libc::IPPROTO_IP,
libc::IP_PKTINFO,
&enabled as *const _ as _,
core::mem::size_of_val(&enabled) as _,
))
.is_ok();

success |= libc!(setsockopt(
rx_socket.as_raw_fd(),
libc::IPPROTO_IPV6,
libc::IPV6_RECVPKTINFO,
&enabled as *const _ as _,
core::mem::size_of_val(&enabled) as _,
))
.is_ok();
Copy link
Contributor

Choose a reason for hiding this comment

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

this previously had the check for if rx_addr.is_ipv4(). Why did that get removed?

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 not really needed to guard against. If the call fails because it's only a single IP-version stack, then that single call with fail. In this case, we consider the configuration successful if either/both of these calls succeed.

use std::os::unix::io::AsRawFd;
let enabled: libc::c_int = 1;

success |= libc!(setsockopt(
Copy link
Contributor

Choose a reason for hiding this comment

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

this previously had the check for if rx_addr.is_ipv4() || !cfg!(any(target_os = "macos", target_os = "ios")). Why is that removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're no longer bailing on errors; just returning if they succeeded or not. this new approach is more robust since we try our best to enable everything we can and fall back if it wasn't successful.

@camshaft camshaft merged commit 6b2b018 into main May 2, 2023
@camshaft camshaft deleted the camshaft/platform-syscalls branch May 2, 2023 22:07
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