stats: Add support for RTM_NEWSTATS/RTM_GETSTATS#283
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #283 +/- ##
==========================================
+ Coverage 68.10% 68.45% +0.34%
==========================================
Files 144 151 +7
Lines 10103 11297 +1194
==========================================
+ Hits 6881 7733 +852
- Misses 3222 3564 +342 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Introduce support for stats messages in the route netlink protocol, enabling parsing and emission of per-interface statistics. The new stats module provides: - StatsHeader with family, ifindex, and filter_mask fields - StatsMessage with header and NLA attributes - StatsAttribute for individual stats attributes - Xstats support including hardware stats, bridge stats, bond stats and per-address-family stats Unit tests are included. Signed-off-by: Gris Ge <cnfourt@gmail.com>
![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 introduces support for Netlink stats messages (RTM_NEWSTATS and RTM_GETSTATS), implementing the parsing and serialization of various network interface statistics including bridge, bond, offload, and address family-specific stats. The review feedback highlights several critical correctness and performance improvements: correcting the nla_len field in HwSInfo::emit to exclude padding bytes per the Netlink protocol standard, using Stats64Buffer::new_checked to prevent potential out-of-bounds panics on malformed payloads, refactoring duplicated helper functions to ensure endianness safety, and replacing eager ok_or calls with lazy ok_or_else closures to avoid unnecessary error allocations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| pub fn emit(&self, buffer: &mut [u8]) { | ||
| let mut off = 0; | ||
| if let Some(v) = self.request { | ||
| let padded = 4 + 4; // NLA header + 4 bytes (u8 + 3 padding) | ||
| buffer[off..off + 2] | ||
| .copy_from_slice(&(padded as u16).to_ne_bytes()); | ||
| buffer[off + 2..off + 4].copy_from_slice(&1u16.to_ne_bytes()); | ||
| buffer[off + 4] = v; | ||
| off += padded; | ||
| } | ||
| if let Some(v) = self.used { | ||
| let padded = 4 + 4; | ||
| buffer[off..off + 2] | ||
| .copy_from_slice(&(padded as u16).to_ne_bytes()); |
There was a problem hiding this comment.
The nla_len field of a Netlink Attribute (NLA) must not include padding bytes. It should be the sum of the header length (4 bytes) and the payload length (1 byte for u8), which is 5. Currently, the code writes padded (8) as the length, which violates the Netlink protocol standard and can cause parsing issues. Additionally, the padding bytes should be explicitly zeroed out to prevent leaking uninitialized memory.
if let Some(v) = self.request {
let len = 4 + 1;
let padded = 8;
buffer[off..off + 2]
.copy_slice(&(len as u16).to_ne_bytes());
buffer[off + 2..off + 4].copy_from_slice(&1u16.to_ne_bytes());
buffer[off + 4] = v;
buffer[off + 5..off + 8].fill(0);
off += padded;
}
if let Some(v) = self.used {
let len = 4 + 1;
let padded = 8;
buffer[off..off + 2]
.copy_from_slice(&(len as u16).to_ne_bytes());
buffer[off + 2..off + 4].copy_from_slice(&2u16.to_ne_bytes());
buffer[off + 4] = v;
buffer[off + 5..off + 8].fill(0);
}References
- The nla_len field of a Netlink Attribute (NLA) does not include padding bytes. It is the sum of the header length (4 bytes) and the payload length. Padding is added to the buffer for alignment but is not reflected in the nla_len value.
| IFLA_STATS_LINK_64 => Self::Link64( | ||
| Stats64::parse(&Stats64Buffer::new(payload)) | ||
| .context("invalid IFLA_STATS_LINK_64 value")?, | ||
| ), |
There was a problem hiding this comment.
Using Stats64Buffer::new(payload) directly without checking the payload length can lead to out-of-bounds reads or panics if the kernel sends a malformed or shorter payload than LINK_STATS64_LEN (200 bytes). Use Stats64Buffer::new_checked(payload) instead to safely validate the payload length before parsing.
IFLA_STATS_LINK_64 => {
let err = "invalid IFLA_STATS_LINK_64 value";
Self::Link64(
Stats64::parse(&Stats64Buffer::new_checked(payload).context(err)?)
.context(err)?,
)
}| return None; | ||
| } | ||
| fn le64(d: &[u8]) -> u64 { | ||
| let mut b = [0u8; 8]; | ||
| b.copy_from_slice(&d[..8]); |
There was a problem hiding this comment.
The helper function le64 is duplicated 5 times across different parse methods in this file. Additionally, the name le64 is a misnomer because it uses u64::from_ne_bytes (native endianness), which could be big-endian on big-endian architectures. Consider refactoring this into a single file-level helper function named read_u64 or ne_u64 using u64::from_ne_bytes(d[..8].try_into().unwrap()) to improve maintainability and correctness.
| let val = nla.value(); | ||
| result.push(match kind { | ||
| BRIDGE_XSTATS_MCAST => BridgeXstat::Mcast( | ||
| BridgeMcastStats::parse(val) | ||
| .ok_or(DecodeError::from("invalid bridge mcast stats"))?, | ||
| ), | ||
| BRIDGE_XSTATS_STP => BridgeXstat::Stp( | ||
| BridgeStpXstats::parse(val) | ||
| .ok_or(DecodeError::from("invalid bridge stp stats"))?, | ||
| ), | ||
| BRIDGE_XSTATS_VLAN => BridgeXstat::Vlan( | ||
| BridgeVlanXstats::parse(val) |
There was a problem hiding this comment.
Using ok_or with DecodeError::from(...) eagerly evaluates and allocates the error even in the happy path. Use ok_or_else with a closure instead to avoid unnecessary allocations and improve performance.
BRIDGE_XSTATS_MCAST => BridgeXstat::Mcast(
BridgeMcastStats::parse(val)
.ok_or_else(|| DecodeError::from("invalid bridge mcast stats"))?,
),
BRIDGE_XSTATS_STP => BridgeXstat::Stp(
BridgeStpXstats::parse(val)
.ok_or_else(|| DecodeError::from("invalid bridge stp stats"))?,
),
BRIDGE_XSTATS_VLAN => BridgeXstat::Vlan(
BridgeVlanXstats::parse(val)
.ok_or_else(|| DecodeError::from("invalid bridge vlan stats"))?,
)| let kind = nla.kind() & !NLA_F_NESTED; | ||
| let val = nla.value(); | ||
| result.push(match kind { | ||
| IFLA_OFFLOAD_XSTATS_CPU_HIT => OffloadXstat::CpuHit( | ||
| HwStats64::parse(val) | ||
| .ok_or(DecodeError::from("invalid cpu hit stats"))?, | ||
| ), | ||
| IFLA_OFFLOAD_XSTATS_HW_S_INFO => { | ||
| OffloadXstat::HwSInfo(HwSInfo::parse(val)) | ||
| } | ||
| IFLA_OFFLOAD_XSTATS_L3_STATS => OffloadXstat::L3Stats( |
There was a problem hiding this comment.
Using ok_or with DecodeError::from(...) eagerly evaluates and allocates the error even in the happy path. Use ok_or_else with a closure instead to avoid unnecessary allocations and improve performance.
IFLA_OFFLOAD_XSTATS_CPU_HIT => OffloadXstat::CpuHit(
HwStats64::parse(val)
.ok_or_else(|| DecodeError::from("invalid cpu hit stats"))?,
),
IFLA_OFFLOAD_XSTATS_HW_S_INFO => {
OffloadXstat::HwSInfo(HwSInfo::parse(val))
},
IFLA_OFFLOAD_XSTATS_L3_STATS => OffloadXstat::L3Stats(
HwStats64::parse(val)
.ok_or_else(|| DecodeError::from("invalid l3 stats"))?,
)
1 participant
Introduce support for stats messages in the route netlink protocol,
enabling parsing and emission of per-interface statistics.
The new stats module provides:
and per-address-family stats
Unit tests are included.