Tsi: handle protocol field to make DGRAM+ICMP ping work #690
Conversation
Read the protocol field from TsiProxyCreate (backward compatible — old guests that don't send it get 0, treated as default/UDP). When protocol is IPPROTO_ICMP or IPPROTO_ICMPV6, create a ping socket (SOCK_DGRAM + IPPROTO_ICMP) on the host instead of a plain UDP socket. This enables rootless ping through TSI. Only works for ping implementations using SOCK_DGRAM ping sockets (iputils on Fedora, Ubuntu, etc.). SOCK_RAW-based ping (busybox, alpine) is not supported as TSI only hijacks SOCK_STREAM and SOCK_DGRAM. Assisted-by: OpenCode:claude-opus-4.6 Signed-off-by: Matej Hrica <mhrica@redhat.com>
Tests that need real network connectivity (e.g. external ping) can return true from needs_host_network() to skip the unshare --net namespace isolation automatically. Assisted-by: OpenCode:claude-opus-4.6 Signed-off-by: Matej Hrica <mhrica@redhat.com>
Run Fedora's /usr/bin/ping against 8.8.8.8 from inside the guest to verify SOCK_DGRAM+IPPROTO_ICMP is properly proxied through TSI. Uses needs_host_network() since the test requires real connectivity. Assisted-by: OpenCode:claude-opus-4.6 Signed-off-by: Matej Hrica <mhrica@redhat.com>
Set /proc/sys/net/ipv4/ping_group_range to allow all GIDs when TSI is enabled. Without this, the guest kernel rejects SOCK_DGRAM + IPPROTO_ICMP sockets with EACCES, preventing TSI from hijacking them. Assisted-by: OpenCode:claude-opus-4.6 Signed-off-by: Matej Hrica <mhrica@redhat.com>
macOS DGRAM ICMP sockets include the IP header in recv, unlike Linux which strips it. Detect this and remove the IP header before forwarding to the guest so the guest sees the same format as a Linux ping socket. Assisted-by: OpenCode:claude-opus-4.6 Signed-off-by: Matej Hrica <mhrica@redhat.com>
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for proxying unprivileged ICMP ping sockets (SOCK_DGRAM with IPPROTO_ICMP or IPPROTO_ICMPV6) via TSI. It configures the ping group range during initialization, extends the TSI protocol to transmit the socket protocol, and implements macOS-specific IP header stripping for received ICMP packets. A new integration test is also introduced. Review feedback points out that the macOS IP header stripping logic should not be applied to ICMPv6, as macOS does not prepend the IPv6 header to ICMPv6 socket data. Additionally, the test code should use String::from_utf8(...).unwrap() instead of String::from_utf8_lossy(...) to ensure immediate failure on invalid UTF-8.
| if matches!( | ||
| self.protocol as _, | ||
| libc::IPPROTO_ICMP | libc::IPPROTO_ICMPV6 | ||
| ) && cnt >= 20 |
There was a problem hiding this comment.
On macOS, SOCK_DGRAM sockets with IPPROTO_ICMPV6 do not prepend the IPv6 header to the received data (unlike IPv4 ICMP sockets which prepend the IPv4 header). Furthermore, the IPv6 header does not have an Internet Header Length (IHL) field at buf[0] & 0x0F.
Including IPPROTO_ICMPV6 in this stripping logic will cause the code to incorrectly strip bytes from the actual ICMPv6 header (for example, corrupting the ICMPv6 Echo Reply type/code), leading to broken IPv6 ping functionality on macOS. This stripping logic should only be applied to IPPROTO_ICMP (IPv4).
if self.protocol as i32 == libc::IPPROTO_ICMP && cnt >= 20| let stderr = String::from_utf8_lossy(&output.stderr); | ||
| let stdout = String::from_utf8_lossy(&output.stdout); |
There was a problem hiding this comment.
According to the repository's general rules, test code should prefer using String::from_utf8(...).unwrap() over String::from_utf8_lossy(...) when processing command output. This ensures that the test fails immediately if the output contains invalid UTF-8.
| let stderr = String::from_utf8_lossy(&output.stderr); | |
| let stdout = String::from_utf8_lossy(&output.stdout); | |
| let stderr = String::from_utf8(output.stderr).unwrap(); | |
| let stdout = String::from_utf8(output.stdout).unwrap(); |
References
- In test code, prefer using String::from_utf8(...).unwrap() over String::from_utf8_lossy(...) when processing command output to ensure the test fails immediately if the output contains invalid UTF-8.
1 participant
Three things were missing for ping to work through TSI:
Host used to create UDP sockets instead of ICMP — now reads the protocol field from the guest - needs tsi: forward protocol field in proxy create enabling DGRAM+ICMP ping libkrunfw#127
Guest used to reject SOCK_DGRAM+ICMP with EACCES — init now sets ping_group_range
macOS host includes the IP header in ICMP recv — we need to strip it before forwarding to guest
NOTE: This only works with modern ping implementations using DGRAM+ICMP "ping" sockets ( works on modern Fedora, Ubuntu). Busybox ping uses SOCK_RAW which TSI doesn't hijack.