-
-
Notifications
You must be signed in to change notification settings - Fork 603
Fix false positive GSO detection #2248
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
Changes from 10 commits
e906553
9095600
0f5608e
ace8387
66af0d1
8d1f98e
383c256
974010d
9f110b9
aab94a1
b00a1d1
f0e69d6
4cc470a
8a142c8
7235e3f
458690e
132de11
4daa187
d3bfe6f
92d2ae2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| #[cfg(not(any(apple, target_os = "openbsd", solarish)))] | ||
| use std::ptr; | ||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| use std::sync::OnceLock; | ||
| use std::{ | ||
| io::{self, IoSliceMut}, | ||
| mem::{self, MaybeUninit}, | ||
|
|
@@ -794,6 +796,91 @@ pub(crate) const BATCH_SIZE: usize = 32; | |
| #[cfg(apple_slow)] | ||
| pub(crate) const BATCH_SIZE: usize = 1; | ||
|
|
||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| mod linux { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: there's no point in making a separate module for this, just move the contents of this into the existing Linux |
||
| pub(crate) fn kernel_version_string() -> Result<String, libc::c_int> { | ||
| let mut n = unsafe { std::mem::zeroed() }; | ||
| let r = unsafe { libc::uname(&mut n) }; | ||
| if r != 0 { | ||
| return Err(r); | ||
| } | ||
|
Comment on lines
+878
to
+881
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand libc's
https://man7.org/linux/man-pages/man2/uname.2.html Would it make sense to retrieve the actual error code here?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| Ok(unsafe { | ||
| std::ffi::CStr::from_ptr(n.release[..].as_ptr()) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: import |
||
| .to_string_lossy() | ||
| .into_owned() | ||
| }) | ||
| } | ||
|
|
||
| // https://www.linfo.org/kernel_version_numbering.html | ||
| #[derive(Eq, PartialEq, Ord, PartialOrd, Debug)] | ||
| pub(crate) struct KernelVersion { | ||
| version: u8, | ||
| major_revision: u8, | ||
| } | ||
|
|
||
| impl KernelVersion { | ||
| pub(crate) const fn new(version: u8, major_revision: u8) -> Self { | ||
| Self { | ||
| version, | ||
| major_revision, | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn from_str(release: &str) -> Result<Self, String> { | ||
| let mut split = release | ||
| .split_once('-') | ||
| .map(|pair| pair.0) | ||
| .unwrap_or(release) | ||
| .split('.'); | ||
|
Comment on lines
+898
to
+902
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... then this can be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The On the other hand if we go with a "let's not worry about what we don't know" approach, then we could get rid of the |
||
|
|
||
| let Some(version) = split.next().and_then(|s| s.parse().ok()) else { | ||
| return Err(format!("Failed to parse kernel version from {release:?}")); | ||
| }; | ||
| let Some(major_revision) = split.next().and_then(|s| s.parse().ok()) else { | ||
|
inetic marked this conversation as resolved.
Outdated
|
||
| return Err(format!( | ||
| "Failed to parse kernel major revision from {release:?}" | ||
| )); | ||
| }; | ||
|
|
||
| Ok(Self { | ||
| version, | ||
| major_revision, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn parse_current_kernel_version_release_string() { | ||
| let release = kernel_version_string().unwrap(); | ||
| KernelVersion::from_str(&release).unwrap(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn parse_kernel_version_release_string() { | ||
| assert_eq!( | ||
| KernelVersion::from_str("4.14"), | ||
| Ok(KernelVersion::new(4, 14)) | ||
| ); | ||
| assert_eq!( | ||
| KernelVersion::from_str("4.18"), | ||
| Ok(KernelVersion::new(4, 18)) | ||
| ); | ||
| assert_eq!( | ||
| KernelVersion::from_str("4.14.186-27095505"), | ||
| Ok(KernelVersion::new(4, 14)) | ||
| ); | ||
| assert_eq!( | ||
| KernelVersion::from_str("6.8.0-59-generic"), | ||
| Ok(KernelVersion::new(6, 8)) | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| mod gso { | ||
| use super::*; | ||
|
|
@@ -803,12 +890,21 @@ mod gso { | |
| #[cfg(target_os = "android")] | ||
| // TODO: Add this to libc | ||
| const UDP_SEGMENT: libc::c_int = 103; | ||
|
|
||
| /// Checks whether GSO support is available by setting the UDP_SEGMENT | ||
| /// option on a socket | ||
| // Support for UDP GSO has been added to linux kernel in version 4.18 | ||
| // https://github.com/torvalds/linux/commit/cb586c63e3fc5b227c51fd8c4cb40b34d3750645 | ||
| const SUPPORTED_SINCE: linux::KernelVersion = linux::KernelVersion::new(4, 18); | ||
| // Avoid calling `supported_by_current_kernel` for each socket by using `OnceLock`. | ||
| static SUPPORTED_BY_CURRENT_KERNEL: OnceLock<bool> = OnceLock::new(); | ||
|
|
||
| /// Checks whether GSO support is available by checking the kernel version followed by setting | ||
| /// the UDP_SEGMENT option on a socket | ||
| pub(crate) fn max_gso_segments() -> usize { | ||
| const GSO_SIZE: libc::c_int = 1500; | ||
|
|
||
| if !SUPPORTED_BY_CURRENT_KERNEL.get_or_init(supported_by_current_kernel) { | ||
| return 1; | ||
| } | ||
|
|
||
| let socket = match std::net::UdpSocket::bind("[::]:0") | ||
| .or_else(|_| std::net::UdpSocket::bind((Ipv4Addr::LOCALHOST, 0))) | ||
| { | ||
|
|
@@ -830,6 +926,36 @@ mod gso { | |
| } | ||
| } | ||
|
|
||
| fn supported_by_current_kernel() -> bool { | ||
| let kernel_version_string = match linux::kernel_version_string() { | ||
| Ok(kernel_version_string) => kernel_version_string, | ||
| Err(_errno) => { | ||
| crate::log::warn!( | ||
| "Failed to retrieve kernel version string, GSO not enabled ({_errno})" | ||
|
inetic marked this conversation as resolved.
Outdated
|
||
| ); | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| let kernel_version = match linux::KernelVersion::from_str(&kernel_version_string) { | ||
| Ok(kernel_version) => kernel_version, | ||
| Err(_reason) => { | ||
| crate::log::warn!("GSO not enabled: {}", _reason); | ||
|
inetic marked this conversation as resolved.
Outdated
|
||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| if kernel_version < SUPPORTED_SINCE { | ||
| crate::log::info!( | ||
| "GSO supported on Linux kernels 4.18+, current is {:?}", | ||
|
inetic marked this conversation as resolved.
Outdated
|
||
| kernel_version_string | ||
| ); | ||
| false | ||
|
inetic marked this conversation as resolved.
Outdated
|
||
| } else { | ||
| true | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn set_segment_size(encoder: &mut cmsg::Encoder<libc::msghdr>, segment_size: u16) { | ||
| encoder.push(libc::SOL_UDP, UDP_SEGMENT, segment_size); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.