netvsp: Split out RNDIS message size errors to identify individual causes. (#4160) - #4174
Conversation
…uses. (microsoft#4160) This is currently the top-hitting error in logs for netvsp, but it combines [known Linux kernel issues](https://lkml.org/lkml/2025/5/12/1565) with actual malformed RNDIS messages, either missing data, missing PPI, or other format issues. The traces we have don't allow us to identify these different failure modes, so this conflates a known defect in guest kernels with potential actual errors. The changes in this PR split that single error into a set of more targeted buckets for better reporting. --------- Co-authored-by: Ben Lewis <Ben.Lewis@microsoft.com> (cherry picked from commit 9eff6f9)
github-actions
Bot
added
the
release_1.8.2607
There was a problem hiding this comment.
🟡 Changes recommended
The new RndisBadHeaders bucket can be bypassed by short-first-range headers, which will still surface as WorkerError::Access and undermine the intended error categorization.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR refines netvsp’s RNDIS error reporting by splitting the previously broad “message too small” failure into more specific buckets, helping distinguish known guest kernel issues from genuinely malformed RNDIS messages.
Changes:
- Introduces
MessageComponentErrorto label which part of an RNDIS message was missing/too small. - Updates
WorkerError::RndisMessageTooSmallto carry the specific component cause. - Adds a dedicated
WorkerError::RndisBadHeadersbucket for the known “headers missing/split across a page” guest behavior.
File summaries
| File | Description |
|---|---|
| vm/devices/net/netvsp/src/lib.rs | Splits RNDIS message size failures into component-specific errors and adds a new bucket for known bad/split header cases. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Changes recommended
RndisBadHeaders currently won’t reliably bucket “headers split across a page” cases (they still surface as AccessError::OutOfRange), undermining the stated reporting goal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
vm/devices/net/netvsp/src/lib.rs:2594
RndisBadHeadersis intended to cover headers that are missing or split across a page, but this code only returns it when there are no paged ranges at all. If the header is split acrossPagedRanges,headers.reader(mem).read_plain()will instead fail withAccessError::OutOfRangeand be reported asWorkerError::Access, losing the intended bucketing. Consider detecting an undersized first range and mapping it toRndisBadHeadersbefore attempting to read the header.
let headers = reader
.clone()
.into_inner()
.paged_ranges()
.next()
vm/devices/net/netvsp/src/lib.rs:2059
MessageComponentErroris used as a categorization label, not an underlying error. Marking it as#[source]makes it show up in the error cause chain, which can confuse error reporters/telemetry that treat sources as causal failures. Consider storing it as a plain field (it is already included in the display message via{0}).
#[error("rndis message too small: {0}")]
RndisMessageTooSmall(#[source] MessageComponentError),
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Ben Lewis (@ben-zen): I created this cherry pick via script to help move required code into the 1.8 branch. Please review the copilot comments on this cherry pick and let me know if you think it's still good to go into the release, or if we need to take some fix-ups. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
vm/devices/net/netvsp/src/lib.rs:2596
RndisBadHeadersis described as "headers missing or split across a page", but thisok_or(RndisBadHeaders)only triggers when there are no ranges at all. If the header is present but split across multiple GPA ranges (the LKML issue this is trying to identify),headers.reader(mem).read_plain()?will still fail later withWorkerError::Access, so this bucket may not capture the intended failure mode. Consider explicitly detecting "first range too small for the packet header" and mapping that toRndisBadHeaders, while keeping genuine truncation asRndisMessageTooSmall(Header).
// Headers are guaranteed to be in a single PagedRange.
let headers = reader
.clone()
.into_inner()
.paged_ranges()
.next()
.ok_or(WorkerError::RndisBadHeaders)?;
let mut data = reader.into_inner();
vm/devices/net/netvsp/src/lib.rs:2060
MessageComponentErroris used as a label to bucket "too small" failures, but it is marked as#[source]onRndisMessageTooSmall. That makes the bucket show up as an error cause in error-chain formatting (potentially duplicating the component in logs) even though there is no underlying failure being wrapped here. Consider storing it as a normal field (not a source) and relying on the formatted message to carry the component.
#[error("rndis message too small: {0}")]
RndisMessageTooSmall(#[source] MessageComponentError),
// See https://lkml.org/lkml/2025/5/12/1565 for more information.
I think it's good to go in as-is; as we see more clearly what paths are actually noisy, we can clean these errors up more. |
Clean cherry pick of PR #4160
This is currently the top-hitting error in logs for netvsp, but it combines known Linux kernel issues with actual malformed RNDIS messages, either missing data, missing PPI, or other format issues. The traces we have don't allow us to identify these different failure modes, so this conflates a known defect in guest kernels with potential actual errors.
The changes in this PR split that single error into a set of more targeted buckets for better reporting.