Fix false positive GSO detection by inetic · Pull Request #2248 · quinn-rs/quinn

inetic · 2025-05-17T18:30:26Z

On some devices with kernel <4.18 the existing checks for GSO not being enabled on the system don't trigger. Quinn then continues to use GSO which results in eventual disconnection as discussed in issue #2246.

This PR adds a high level test to showcase the disconnection and a runtime check based on parsing Linux kernel version string to disable GSO in quinn-udp on kernels where it hasn't yet been implemented (4.18 torvalds/linux@cb586c6).

thomaseizinger

Left a few comments with food for thought!

thomaseizinger · 2025-05-17T22:29:05Z

    const UDP_SEGMENT: libc::c_int = 103;

+    // Global mutex to avoid calling `libc::uname` for each socket.
+    static SUPPORTED_BY_OS: Mutex<Option<bool>> = Mutex::new(None);


Perhaps a OnceLock is more convenient here?

We can then get rid of the KernelVersion struct and have a stateless function that checks if the version is < 4.18

I don't think we can use OnceLock for MSRV reasons, but we should be able to use once_cell?

We can then get rid of the KernelVersion struct and have a stateless function that checks if the version is < 4.18

I had the opposite plan to expand the KernelVersion struct by further spliting the parsing code into KernelVersion::from_str which would be called from KernelVersion::from_uname and write small tests with sting samples. The test could be a way of tracking if some kernels have inconsistent version strings as you noted here #2246 (comment).

This wouldn't be possible with a single function returning a boolean.

Let me know which way you prefer.

This wouldn't be possible with a single function returning a boolean.

You can split it into multiple functions and test the pure ones separately:

1 function that calls libc::uname and returns a string

1 function that checks if a given string is a kernel version > 4.18

1 function that combines the above too or perhaps just do that inline in the closure passed to once_cell

The 2nd one can then be easily unit-tested with multiple inputs.

I don't have a strong preference but if we can avoid intermediary structs, I think it is easier to understand.

I don't think we can use OnceLock for MSRV reasons,

Running cargo msrv find I get the result 1.74.1 (log: msrv-find.log). OnceLock seems to be supported since 1.70.0. So unless there is a plan to decrease the current MSRV I think OnceLock should be OK to use.

Ah, okay, good.

I don't have a strong preference but if we can avoid intermediary structs, I think it is easier to understand.

I did modify the code a bit, though I left the KernelVersion struct there. The reason was that I felt that the tests would be more to the point if they tested the parsing alone instead of parsing and checking >=4.18. It might be a matter of taste/style, if so, then perhaps it'd be better to leave that part to you (the code owners)?

djc · 2025-05-19T07:26:06Z

(Please rebase to get rid of the merge commit.)

The issue on Android with Linux kernel 4.14 is more directly reproducible by the `gso` test.

inetic · 2025-05-19T11:01:18Z

One outstanding question to resolve is whether to enable GSO if uname or the parsing fails, as mentioned here #2246 (comment). There are pros and cons:

Fallback is to enable:

pros: fast bandwidth and low cycles on modern kernels
cons: broken connections on old kernels

Fallback is to disable

pros: connections work on old kernels
cons: slow bandwidth and high cycles on modern kernels

thomaseizinger

Nice work, looks great! I left a few comments, nothing blocking :)

Ralith · 2025-05-19T16:46:18Z

whether to enable GSO if uname or the parsing fails

Are we aware of any cases where these could fail with a modern kernel? Especially uname failing suggests to me that we'd be in a very exotic environment where it might be better to be conservative, although maybe syscall filtering could cause that?

thomaseizinger · 2025-05-19T22:37:15Z

whether to enable GSO if uname or the parsing fails

Are we aware of any cases where these could fail with a modern kernel? Especially uname failing suggests to me that we'd be in a very exotic environment where it might be better to be conservative, although maybe syscall filtering could cause that?

I am not aware of any cases. The idea of falling back to GSO enabled is that in the rare(?) case of an error, this doesn't cause a performance regression by disabling GSO.

Conservative in my eyes means just that: Preserving the current behaviour of quinn-udp.

My guess would be that quinn-udp is predominantely run on newer Linux kernels so defaulting to true seems like the better option. If it does indeed fail on an older kernel, the user will see it in the logs and can report a bug.

djc · 2025-05-20T10:26:45Z

 pub(crate) const BATCH_SIZE: usize = 1;

+#[cfg(any(target_os = "linux", target_os = "android"))]
+mod linux {


Nit: there's no point in making a separate module for this, just move the contents of this into the existing Linux gso module. Try to keep the additions in top-down order, so callers come before callees.

djc · 2025-05-20T10:27:11Z

+
+            let version = split
+                .next()
+                .and_then(|s| s.parse().ok())


Nit: please use u8::from_str() instead of .parse(), and avoid capitalizing error messages.

use u8::from_str() instead of .parse()

This will require use std::str::FromStr;, are you ok with it?

Yes; that's the same stuff that parse() uses.

avoid capitalizing error messages

Also for "GSO" in "GSO disabled:..."?

Sorry, no -- just the first letter.

djc · 2025-05-20T10:27:37Z

Thanks for working on this!

inetic · 2025-05-21T12:01:05Z

Thanks for working on this!

Thanks for all the feedback :)

djc

A bunch of style feedback, I think we're close!

djc · 2025-05-21T15:13:04Z

+        const fn new(version: u8, major_revision: u8) -> Self {
+            Self {
+                version,
+                major_revision,
+            }
+        }


Nit: let's drop this trivial method.

djc · 2025-05-21T15:13:17Z

+        }
+
+        fn from_str(release: &str) -> Result<Self, String> {
+            use std::str::FromStr;


Nit: import this at the top of the module.

djc · 2025-05-21T15:14:09Z

+            }
+        }
+
+        fn from_str(release: &str) -> Result<Self, String> {


Nit: suggest returning Option<Self> from this.

That is what we originally had and I suggested returning an error to consolidate the places where we log what went wrong :)

I guess as long as we print the original string returned from uname as part of the WARN log, we can always reproduce it if this gets hit in the wild.

(Our app is instrumented with crash reporting so we get sent WARNs and ERRORs automatically.)

djc · 2025-05-21T15:15:02Z

+            let mut split = release
+                .split_once('-')
+                .map(|pair| pair.0)
+                .unwrap_or(release)
+                .split('.');


... then this can be release.split_once('-')?.0.split('.').

The .unwrap_or(release) part is there to avoid failing on strings which don't have "-", like "4.18" for example. I don't know whether such release strings exist in real life, but might be good to keep it just in case?

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 .split_once('-') part all together because the formats that I've seen so far were always "X.Y.Z-...", for which let mut split = release.split('.'); would suffice.

djc · 2025-05-21T15:15:30Z

+                .unwrap_or(release)
+                .split('.');
+
+            let version = split


And this can be u8::from_str(split.next()?).ok()?.

djc · 2025-05-21T15:15:39Z

+            let major_revision = split
+                .next()
+                .and_then(|s| u8::from_str(s).ok())
+                .ok_or_else(|| format!("failed to parse kernel major revision from {release:?}"))?;


And similar here.

djc · 2025-05-21T15:16:19Z

+            return Err(r);
+        }
+        Ok(unsafe {
+            std::ffi::CStr::from_ptr(n.release[..].as_ptr())


Nit: import CStr at the top of the module, and mem too.

djc · 2025-05-21T15:16:48Z

+    fn supported_by_current_kernel() -> bool {
+        let kernel_version_string = match kernel_version_string() {
+            Ok(kernel_version_string) => kernel_version_string,
+            Err(_errno) => {


Why do these need _ prefixes?

When the log macros are configured out (no features active), they'd otherwise show up as unused variables.

djc · 2025-05-21T15:16:52Z

+    // https://github.com/torvalds/linux/commit/cb586c63e3fc5b227c51fd8c4cb40b34d3750645
+    const SUPPORTED_SINCE: KernelVersion = 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();


Nit: move SUPPORTED_BY_CURRENT_KERNEL below set_segment_size() (it should be below max_gso_segments() and above supported_by_current_kernel()).

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

mxinden

One comment, otherwise looks good to me.

Thank you for the work!

mxinden · 2025-05-22T15:49:04Z

+        let r = unsafe { libc::uname(&mut n) };
+        if r != 0 {
+            return Err(r);
+        }


If I understand libc's uname correctly, the return value is either 0 or -1. On -1 the actual error code can be retrieved via errno:

On success, zero is returned. On error, -1 is returned, and errno
is set to indicate the error.

https://man7.org/linux/man-pages/man2/uname.2.html

Would it make sense to retrieve the actual error code here?

See also: https://doc.rust-lang.org/std/io/struct.Error.html#method.last_os_error

djc

Okay, going to merge this and do some follow up. Thanks for all the iterations!

inetic requested review from Ralith, djc, gretchenfrage and mxinden as code owners May 17, 2025 18:30

inetic mentioned this pull request May 17, 2025

Sending/receiving of a >~100KB of data fails on some Android devices #2246

Open

thomaseizinger reviewed May 17, 2025

View reviewed changes

inetic added 8 commits May 19, 2025 12:36

Add a test failing on a particular Samsung Android

e906553

Don't enable GSO on Linux kernel lower than 4.18

9095600

Remove the send_receive test

0f5608e

The issue on Android with Linux kernel 4.14 is more directly reproducible by the `gso` test.

Separate string parsing from KernelVersion::from_uname

ace8387

Log from one place

66af0d1

Replace Mutex with OnceLock

8d1f98e

Remove the string member from KernelVersion

383c256

Add kernel release version parsing tests

974010d

inetic force-pushed the samsung-issue branch from 36d13c8 to 974010d Compare May 19, 2025 10:37

inetic added 2 commits May 19, 2025 13:05

Guard OnceLock use with target_os = linux or android

9f110b9

Fix CI --no-default-features check

aab94a1

thomaseizinger approved these changes May 19, 2025

View reviewed changes

Comment thread quinn-udp/src/unix.rs Outdated

Comment thread quinn-udp/src/unix.rs Outdated

Comment thread quinn-udp/src/unix.rs Outdated

Comment thread quinn-udp/src/unix.rs Outdated

Comment thread quinn-udp/src/unix.rs Outdated

Comment thread quinn-udp/src/unix.rs Outdated

Small improvements

b00a1d1

djc reviewed May 20, 2025

View reviewed changes

Move kernel version parsing code from linux to gso module

f0e69d6

inetic added 2 commits May 21, 2025 14:04

Replace parse with u8::from_str

4cc470a

Avoid capitalization in log messages

8a142c8

djc reviewed May 21, 2025

View reviewed changes

inetic added 4 commits May 22, 2025 11:38

Remove KernelVersion::new

7235e3f

Move imports to top of the gso module

458690e

Simplify KernelVersion::from_str

132de11

Move SUPPORTED_BY_CURRENT_KERNEL

4daa187

thomaseizinger reviewed May 22, 2025

View reviewed changes

Comment thread quinn-udp/src/unix.rs Outdated

Use Display for printing kernel_version_string

d3bfe6f

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

mxinden reviewed May 22, 2025

View reviewed changes

Fix error returned from kernel_version_string()

92d2ae2

djc approved these changes May 23, 2025

View reviewed changes

djc merged commit 81f9cd9 into quinn-rs:main May 23, 2025

This was referenced Jun 18, 2025

chore(udp): increase patch version to v0.5.13 #2270

Merged

perf: use GSO mozilla/neqo#2593

Merged

Uh oh!

Uh oh!

Conversation

inetic commented May 17, 2025

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inetic May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc commented May 19, 2025

Uh oh!

inetic commented May 19, 2025

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ralith commented May 19, 2025

Uh oh!

thomaseizinger commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djc commented May 20, 2025

Uh oh!

inetic commented May 21, 2025

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

inetic May 19, 2025 •

edited

Loading

thomaseizinger commented May 19, 2025 •

edited

Loading

thomaseizinger May 21, 2025 •

edited

Loading

inetic May 22, 2025 •

edited

Loading