hypervisor: drop tap TX frames on EIO instead of exiting the VM by rogelioRuiz · Pull Request #1240 · TencentCloud/CubeSandbox

rogelioRuiz · 2026-07-31T10:26:12Z

Part of #1239.

A writev to the tap that fails with EIO (device down, or carrier not up yet) kills the net worker thread and the VM exits. Sandboxes resumed from a memory snapshot hit this routinely: the guest transmits within ~300ms of resume, racing host tap wiring after a network-agent restart or any external tap churn. One refused frame becomes guest death.

Physical NICs drop the frame when the link is down and the guest stack retransmits. This change does the same for EIO: consume the descriptor chain as a dropped frame (used length 0), count it in the new TxVirtio::dropped_frames, log the first drop and every 1000th, and keep the queue alive. EAGAIN keeps its existing retry behaviour; all other errnos remain fatal.

Test plan:

cargo check -p net_util (rust 1.77.2): clean
cargo test -p net_util (with /dev/net/tun + NET_ADMIN): 12/12 pass
cargo fmt --check / clippy: clean, no new warnings
Live repro from [Bug Report] guest VM exits on first TX when its tap is down or was replaced (EIO from writev treated as fatal) #1239: with this patch a guest whose tap is set down survives (drops counted), where before it exited within ~300ms.

cubesandboxbot · 2026-07-31T10:30:38Z

    pub limit_frames: Wrapping<u64>,
+    /// Frames the tap refused with EIO and we dropped instead of killing the
+    /// VM (see the EIO branch in `process_desc_chain`).
+    pub dropped_frames: Wrapping<u64>,


dropped_frames is counted but never surfaced: unlike counter_frames/limit_frames/limit_bytes, it is not flushed into NetCounters in NetQueuePair::process_tx and never reset, so it never appears in Net::counters() (hypervisor/virtio-devices/src/net.rs:800) or the vm counter CLI. During a down-tap period tx_frames/tx_bytes drop to ~0 and the only signal an operator gets is the throttled warn log — the drop count is invisible to monitoring. Consider adding a tx_dropped_frames field to NetCounters, flushing/resetting it in process_tx alongside the other counters, and exposing it in the counters() map (noting the log's "total dropped" would then need to read the cumulative atomic rather than self.dropped_frames.0).

cubesandboxbot · 2026-07-31T10:30:57Z

Review: drop tap TX frames on EIO instead of exiting the VM (#1240)

Verdict: approve with nits. This is a well-scoped, well-tested fix for a real failure mode (#1239), and the mechanics are correct:

On EIO, the descriptor chain is consumed with a used length of 0, so the virtio TX queue keeps making forward progress instead of wedging, and the guest stack retransmits like it would against a real NIC with the link down.
EAGAIN keeps its existing retry path; every errno other than EIO remains fatal, so the change is narrowly targeted.
tx_dropped_frames is plumbed end to end: TxVirtio → NetCounters → the virtio-net counters() map. NetCounters::default() and TxVirtio::new() pick up the new field automatically, and no other constructor sites need updating (verified: vhost_user_net and net.rs both build the pair via TxVirtio::new() / NetCounters::default()).
The log throttling (should_log_dropped: first drop, then per-1000-boundary) is sensible and the unit tests cover the boundary cases well, including the "one log per batch even when it spans several boundaries" behavior.
Because process_tx is shared, vhost-user-net backends get the same resilience for free.
The counter API consumer (get_counters in the integration test) reads specific keys, so the new key is backward compatible.

Findings, none blocking:

1. The drop log is at `debug!`, so it's invisible at default log levels

The VMM defaults to LevelFilter::Info (hypervisor/src/vmm_config.rs:33), and Warn with no -v flags. The debug! in process_tx is therefore filtered out in production, meaning the PR's stated "log the first drop and every 1000th" behavior never actually fires by default — the drop path is completely silent and only visible through the counter. Since the first drop is a one-time, low-volume signal, I'd suggest emitting it at info! (the existing EAGAIN "listening for TAP to become writable" log is info!, so this is consistent) while keeping the per-1000-boundary logs at debug!. [inline on the debug!]

2. All EIO is treated as "link down", which also masks a genuinely dead tap

EIO from a tap write isn't only the carrier-not-up case; the kernel also returns it when the tap device is being torn down or otherwise malfunctioning. Those cases no longer fail fast — the VM stays up and TX is silently discarded, which can look like a healthy VM with no egress. It's a deliberate, documented trade-off and the counter makes it observable, but consider noting in the comment that this path also covers device teardown, and making sure monitoring/alerting keys off tx_dropped_frames. [inline on the EIO drop]

3. Batch-local drop count is lost if the same batch hits a fatal error (minor)

process_tx flushes self.tx.dropped_frames into the atomic counter only after process_desc_chain returns Ok. If a batch drops one or more frames via EIO and then hits a fatal non-EIO error, the ? propagates before the flush: those drops are never counted, and self.tx.dropped_frames is left non-zero (a stale value that a hypothetical retry on the same NetQueuePair would double-count). Since the worker thread exits on a fatal error this is mostly theoretical, but resetting the local counter unconditionally would make the flush idempotent.

4. Doc nit on `should_log_dropped`

The doc comment says "once per 1000-frame boundary crossed", but the implementation emits at most one log per process_tx batch even when the batch spans several boundaries (as logs_on_each_thousand_boundary_crossed asserts). Suggest wording like "once per batch that crosses a 1000-frame boundary".

This review was produced by an automated AI reviewer.

cubesandboxbot · 2026-07-31T10:43:59Z

+                        {
+                            break;
+                        }
+                        continue;


This continue jumps past the rate_limiter.consume(1, TokenType::Ops) call on the success path, so EIO-dropped frames are never charged to the TX rate limiter. That is arguably correct — nothing was transmitted, and charging the bucket during a down tap would drain it and delay recovery once the tap returns. But the ops budget also doubles as host-CPU protection for guest-driven TX processing, so a guest flooding TX while the tap is down gets unlimited drop-work with no throttling, and limit_frames/limit_bytes won't reflect the surge. Not a blocker, but worth an explicit comment (or a deliberate choice) documenting the tradeoff.

cubesandboxbot · 2026-07-31T10:44:00Z

+                    // drops the frame and lets the guest stack retransmit; do
+                    // the same: consume the chain as a dropped frame and keep
+                    // the queue alive.
+                    if e.raw_os_error() == Some(libc::EIO) {


This new VM-survival path has no automated coverage — it's validated only by the manual live repro in the PR description. A unit test would need a tap whose writev returns EIO (kernel-dependent, so possibly impractical in CI). If it isn't feasible, consider factoring the pure, easily-testable logic out of this change (at minimum the before / 1000 != after / 1000 log-throttle decision in process_tx) into a helper with a unit test, since the errno-ordering (EAGAIN before EIO before fatal) is exactly the kind of thing a future refactor could quietly regress.

cubesandboxbot · 2026-07-31T10:55:43Z

+                    // marks the descriptor used and charges one op to the rate
+                    // limiter. Charging ops keeps guest-driven drop-work
+                    // CPU-throttled while a len-0 byte charge leaves the byte
+                    // budget intact, so tap recovery is not delayed.


Consider the no-backpressure behavior this path introduces: with EAGAIN the worker parks on an EPOLLOUT listener until the tap drains, but with EIO every writev fails instantly, so the loop keeps draining the entire avail ring at the guest's full kick rate for as long as the tap stays down. The op-budget charge only paces this when a TX rate limiter is configured; without one, a guest that keeps transmitting against a down tap can hold this worker thread at sustained high CPU with no EAGAIN to fall back on. This is a reasonable tradeoff versus VM exit, but worth documenting (and possibly bounding the drop work per kick, e.g. break out of the chain loop after the first EIO and rely on the next kick).

cubesandboxbot · 2026-07-31T10:55:44Z

@@ -496,3 +544,35 @@ impl NetQueuePair {
            .map_err(NetQueuePairError::QueueNeedsNotification)
    }
 }


These tests cover only should_log_dropped. The actual EIO behavior (writev → EIO → descriptor marked used with len 0, dropped_frames incremented, process_desc_chain returns Ok, queue survives) is exercised only by the manual repro in the PR description. net_util already has tap-backed tests in tap.rs that run with /dev/net/tun + NET_ADMIN (and Tap::enable() at tap.rs:490 shows the ioctl pattern); a test that brings a tap down so writes return EIO and asserts a frame is dropped rather than the queue erroring would guard this regression.

cubesandboxbot · 2026-07-31T20:52:30Z

+/// 1000-frame boundary crossed, so a persistently down tap cannot flood the
+/// log at line rate.
+fn should_log_dropped(before: u64, after: u64) -> bool {
+    (before == 0 && after > 0) || before / 1000 != after / 1000


Nit — the throttle is keyed to the cumulative counter, which never resets, so a new incident after the total has passed the first 1000-boundary can go completely unlogged: should_log_dropped(1500, 1501) is false, and the "first drop" branch only ever fires once in the VM's lifetime. So a tap that flaps once, months after an earlier burst left the total at, say, 1500, produces no log at all (an operator watching logs would miss it until the total crosses 2000). Consider also triggering on "first drop after a quiet period" (e.g., log when the previous value had been static for some time), or at least documenting that tx_dropped_frames is the source of truth and the log is best-effort.

cubesandboxbot · 2026-07-31T20:52:32Z

-                    error!("net: tx: failed writing to tap: {}", e);
-                    return Err(NetQueuePairError::WriteTap(e));
-                }



Coverage gap: the new #[cfg(test)] module only tests should_log_dropped. This EIO drop path (drop-and-continue with used length 0) and the flush/log block in process_tx are untested. Since tap.rs already has NET_ADMIN-gated integration-style tests, a test that takes a tap down, pushes a TX descriptor chain through the queue, and asserts the frame is dropped (counter incremented, descriptor marked used, worker does not error) would lock in this exact behavior and would have caught a regression in the drain/break logic.

chenhengqi · 2026-08-03T07:52:36Z

This turns a transient host-side condition into guest death. Sandboxes
resumed from a memory snapshot hit it routinely: the guest transmits
within ~300ms of resume, racing host tap wiring after a network-agent
restart or any external tap churn.

How to reproduce this racing host tap wiring?

rogelioRuiz · 2026-08-03T19:45:21Z

The deterministic version is step 2 of #1239 — ip link set z<sandbox-ip> down on the node just before the guest transmits, then any guest TX gets EIO and the VM exits. That's the minimal repro; you don't need the race for it.

The "racing host tap wiring" is how we hit it in prod without touching anything manually. Two timing windows, both on a single-node k3s with a memory-snapshot template (resume transmits within ~300ms of boot):

network-agent restart / tap churn. When network-agent restarts (or an external tap GC removes idle pooled taps), a tap can be momentarily down or freshly re-created by the TUNSETIFF reopen path in GetTapFile — admin-DOWN, no TC filter/carrier yet. A guest resumed in that window transmits before the host finishes bringing the tap up. First TX = EIO = VM exit.
Pool exhausted → on-demand tap. With tap_init_num low/0, a create allocates a tap on demand; it's born state DOWN and not yet enslaved to the bridge when the resumed guest's ~300ms TX fires.

To force the race deterministically without the manual down: restart network-agent (or delete+recreate the tap via the reopen path) while resuming a snapshot sandbox in a loop — the guest that lands in the re-wire window dies. The manual ip link set down in #1239 just collapses that window to 100%.

chenhengqi · 2026-08-04T01:48:23Z

it's born state DOWN and not yet enslaved to the bridge when the resumed guest's ~300ms TX fires.

This is wrong. On-demand TAP holds the fd so its state will be UP. And there is no bridge in CubeSandbox.

ip link set z down

This is also wrong. Please don't do that in production.

rogelioRuiz · 2026-08-04T10:14:25Z

You're right on both, thanks for the correction. There's no bridge; taps are TC-redirected onto cubeDev, not enslaved. And a pooled tap holds its config and stays UP, so "born DOWN" was wrong. Also disregard the ip link set ... down line; that was a diagnostic shortcut, not a production suggestion.

On the trigger: point taken in #1239 that the deleted-tap case is our own GC's doing, and we've dropped that GC per your suggestion. I'd frame #1240 as independent of that though: exiting the guest on a tap-write EIO is a disproportionate response to a transient link error whatever its cause, where a physical NIC would just drop the frame. That's the whole change here.

up2wing · 2026-08-12T02:48:10Z

+                    // nothing to re-arm the worker, stalling the virtqueue. Each
+                    // kick therefore drains one ring's worth of drops (bounded
+                    // by ring depth, and by the ops budget when a TX rate
+                    // limiter is configured).


I suppose the code is clear enough, these comments should be in the git comment, not here

up2wing · 2026-08-12T02:54:28Z

+                .fetch_add(self.tx.dropped_frames.0, Ordering::AcqRel);
+            let after = before + self.tx.dropped_frames.0;
+            if should_log_dropped(before, after) {
+                warn!(


Maybe debug seems better

up2wing · 2026-08-12T03:05:54Z

The code looks good to me with only some nit.

Also, this PR looks somewhat similar to this one[1] in the upstream repository; could you submit a PR to the upstream first?

net: Prevent worker thread death on malformed guest descriptors cloud-hypervisor/cloud-hypervisor#8142

A writev to the tap that fails with EIO (device down, or carrier not up yet) kills the net worker thread and the VM exits: net: tx: failed writing to tap: Input/output error (os error 5) Error running worker: HandleEvent(Error processing TX queue: ...) VM exit event This turns a transient host-side condition into guest death: whatever put the tap in that state, a single refused frame should not be fatal. Sandboxes resumed from a memory snapshot are the most exposed, since the guest transmits within ~300ms of resume and is the first to hit a tap that is momentarily not ready. Real hardware drops the frame and lets the guest stack retransmit. Do the same for EIO: tally the drop and fall through with len 0, so the shared tail still marks the descriptor used and charges one op (but zero bytes) to the rate limiter. Charging ops keeps guest-driven drop-work CPU-throttled during a down tap while the zero-byte charge leaves the byte budget intact, so tap recovery is not delayed. EAGAIN keeps its retry behaviour; all other errnos remain fatal. On EIO we keep draining the ring rather than break out early: unlike EAGAIN there is no EPOLLOUT edge to wake the worker when a *down* tap recovers, so stopping early would leave the already-available descriptors uncompleted with nothing to re-arm the worker, stalling the virtqueue. Each kick drains at most one ring's worth of drops, bounded by ring depth and by the ops budget when a TX rate limiter is configured. The per-batch tally is flushed into a new NetCounters.tx_dropped_frames in process_tx alongside the existing tx counters, exposed in Net::counters() as "tx_dropped_frames", and a throttled debug log (first drop, then each 1000-frame boundary) reports the cumulative total, so a down-tap period is visible to `vm counter` and not only the log. Signed-off-by: rruiz <rruiz@techxagon.io>

rogelioRuiz · 2026-08-13T01:21:36Z

Thanks for the review. Fixed both: moved the comments into the commit message and changed the log to debug.

Also opened the upstream PR you suggested: cloud-hypervisor/cloud-hypervisor#8723 (issue cloud-hypervisor/cloud-hypervisor#8722). Their tree has diverged from this one, so that patch is just the EIO drop next to the EINVAL branch from #8142, no counter. If you'd rather this PR match whatever upstream lands, I can trim it down.

cubesandboxbot · 2026-08-13T01:28:21Z

+                .fetch_add(self.tx.dropped_frames.0, Ordering::AcqRel);
+            let after = before + self.tx.dropped_frames.0;
+            if should_log_dropped(before, after) {
+                debug!(


This log is at debug!, but the hypervisor defaults to LevelFilter::Info (vmm_config.rs:33), and to Warn with no -v. So in a default deployment neither the first drop nor the per-1000-boundary log is ever emitted — the drop path becomes completely silent in production and is only observable through the tx_dropped_frames counter.

Since the PR's stated goal is to make these otherwise-silent drops visible, consider emitting the first drop at info! (the existing EAGAIN "listening for TAP to become writable" log uses info!, so this would be consistent) and keeping only the per-1000-boundary logs at debug!. The first drop is a one-time, low-volume signal; the boundary logs are the flood risk that belongs at debug!.

cubesandboxbot · 2026-08-13T01:28:27Z

-                result as u32
+                    // EIO: the tap refused the frame (device down / carrier
+                    // not up). Drop it like real hardware would and keep the
+                    // queue alive; see the commit message for the rationale.


EIO from a tap write is not exclusively the "carrier not up" case — the kernel also returns EIO when the tap device is being torn down or is otherwise broken. With this branch, those cases no longer fail fast: the VM stays up and TX is silently discarded, which can look like a healthy VM with no egress. The counter makes this observable and it's a deliberate trade-off (matching physical-NIC drop behavior), but it's worth noting in the comment that this path also covers device teardown, and making sure any monitoring/alerting keys off tx_dropped_frames rather than assuming the VM is healthy because it's still running.

rogelioRuiz · 2026-08-15T05:40:03Z

Upstream merged: cloud-hypervisor/cloud-hypervisor#8723

rogelioRuiz requested review from lisongqian and up2wing as code owners July 31, 2026 10:26

cubesandboxbot Bot reviewed Jul 31, 2026

View reviewed changes

rogelioRuiz force-pushed the fix/tx-eio-drop branch from 54e8aca to 0749d70 Compare July 31, 2026 10:38

cubesandboxbot Bot reviewed Jul 31, 2026

View reviewed changes

rogelioRuiz force-pushed the fix/tx-eio-drop branch from 0749d70 to 83df13b Compare July 31, 2026 10:49

cubesandboxbot Bot reviewed Jul 31, 2026

View reviewed changes

lkml-likexu assigned chenhengqi, up2wing and lisongqian Jul 31, 2026

rogelioRuiz force-pushed the fix/tx-eio-drop branch from 83df13b to 46a86bf Compare July 31, 2026 20:43

cubesandboxbot Bot reviewed Jul 31, 2026

View reviewed changes

rogelioRuiz mentioned this pull request Jul 31, 2026

[Bug Report] guest VM exits on first TX when its tap is down or was replaced (EIO from writev treated as fatal) #1239

Closed

up2wing reviewed Aug 12, 2026

View reviewed changes

rogelioRuiz force-pushed the fix/tx-eio-drop branch from 46a86bf to 487d577 Compare August 13, 2026 01:21

cubesandboxbot Bot reviewed Aug 13, 2026

View reviewed changes

Uh oh!

Conversation

rogelioRuiz commented Jul 31, 2026

Uh oh!

cubesandboxbot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: drop tap TX frames on EIO instead of exiting the VM (#1240)

1. The drop log is at debug!, so it's invisible at default log levels

2. All EIO is treated as "link down", which also masks a genuinely dead tap

3. Batch-local drop count is lost if the same batch hits a fatal error (minor)

4. Doc nit on should_log_dropped

Uh oh!

cubesandboxbot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

chenhengqi commented Aug 3, 2026

Uh oh!

rogelioRuiz commented Aug 3, 2026

Uh oh!

chenhengqi commented Aug 4, 2026

Uh oh!

rogelioRuiz commented Aug 4, 2026

Uh oh!

up2wing Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

up2wing Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

up2wing commented Aug 12, 2026

Uh oh!

rogelioRuiz commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cubesandboxbot Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

cubesandboxbot Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

rogelioRuiz commented Aug 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cubesandboxbot Bot commented Jul 31, 2026 •

edited

Loading

1. The drop log is at `debug!`, so it's invisible at default log levels

4. Doc nit on `should_log_dropped`

rogelioRuiz commented Aug 13, 2026 •

edited

Loading