kvcache-ai/Mooncake

Fork 0

mirror of https://github.com/kvcache-ai/Mooncake.git synced 2026-11-05 01:17:55 +00:00

[GH-ISSUE #3066] [Bug]: Silent data corruption risk on Linux 5.15 when using io_uring offload due to broken short-read validation logic #789

New issue

Closed

opened 2026-10-29 21:19:32 +00:00 by bsakaguchi · 5 comments

bsakaguchi commented

2026-10-29 21:19:32 +00:00

Owner

Originally created by @waliwali777 on GitHub (Jul 23, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/3066

Originally assigned to: @LujhCoconut on GitHub.

Bug Report

Environment

Kernel version: 5.15.0-140-generic (Ubuntu 22.04 Jammy LTS)
Feature flag enabled: MOONCAKE_OFFLOAD_USE_URING=true
Storage: Local NVMe disk with O_DIRECT enabled

Symptom

Non-deterministic data precision / CRC mismatch errors occur randomly during bucket load operations. The issue cannot be reproduced stably, and no explicit IO error code is returned to upper-layer callers, making silent corruption invisible to users.

Trigger Condition

The corruption appears when io_uring short-read events happen under Linux 5.15:

Short reads return successful CQE status without error codes
Unfinished trailing buffer regions retain stale dirty memory data
Truncated valid business payload leads to mismatched CRC / data inconsistency

Root Cause Suspect & Code Reference

File: mooncake-store/src/uring_file.cpp, mooncake-store/src/storage_backend.cpp,
Line:
github.com/kvcache-ai/Mooncake@592f117822/mooncake-store/src/uring_file.cpp (L274),
github.com/kvcache-ai/Mooncake@592f117822/mooncake-store/src/storage_backend.cpp (L1952)

There is a confusing assignment logic after invoking uring_file->read_aligned():

#ifdef USE_URING
          // Try to use read_aligned for O_DIRECT I/O if file is UringFile
          UringFile* uring_file = dynamic_cast<UringFile*>(file.get());
          if (uring_file != nullptr) {
              ... ...
              read_res = uring_file->read_aligned(
                  plan.dest_slice.ptr, aligned_size, aligned_offset);

              if (read_res) {
                  ... ...
                  read_res = plan.dest_slice.size;
              }
          } else
#endif
            ... ...

            if (read_res.value() != plan.dest_slice.size) {
                LOG(ERROR) << "Read size mismatch for key: " << plan.key
                           << ", expected: " << plan.dest_slice.size
                           << ", got: " << read_res.value();
                return tl::make_unexpected(ErrorCode::FILE_READ_FAIL);
            }
        }
    }

The variable read_res which stores the actual IO byte count returned by io_uring is overwritten by the expected full slice size.
All subsequent short-read validation logic based on read_res will always evaluate to false:

The code cannot detect real short reads from the underlying io_uring layer
No ErrorCode::FILE_READ_FAIL will be propagated to upstream
Silent truncated data is passed to business logic without any alert

Questions

What is the design intention of overwriting read_res = plan.dest_slice.size at line 1952? Why override the real IO read result returned by read_aligned()?
How does Mooncake guarantee data correctness for the io_uring offload path?
Are there existing safeguards (buffer cleanup, strict short-read rejection, mandatory retries) to avoid silent stale-buffer data corruption?

Before submitting...

Ensure you searched for relevant issues and read the [documentation]

Originally created by @waliwali777 on GitHub (Jul 23, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/3066 Originally assigned to: @LujhCoconut on GitHub. ### Bug Report ## Environment - Kernel version: `5.15.0-140-generic` (Ubuntu 22.04 Jammy LTS) - Feature flag enabled: `MOONCAKE_OFFLOAD_USE_URING=true` - Storage: Local NVMe disk with O_DIRECT enabled ## Symptom Non-deterministic data precision / CRC mismatch errors occur randomly during bucket load operations. The issue cannot be reproduced stably, and no explicit IO error code is returned to upper-layer callers, making silent corruption invisible to users. ### Trigger Condition The corruption appears when io_uring short-read events happen under Linux 5.15: 1. Short reads return successful CQE status without error codes 2. Unfinished trailing buffer regions retain stale dirty memory data 3. Truncated valid business payload leads to mismatched CRC / data inconsistency ## Root Cause Suspect & Code Reference File: `mooncake-store/src/uring_file.cpp`, `mooncake-store/src/storage_backend.cpp`, Line: https://github.com/kvcache-ai/Mooncake/blob/592f117822adde8991d4577545ae3a4b0694f870/mooncake-store/src/uring_file.cpp#L274, https://github.com/kvcache-ai/Mooncake/blob/592f117822adde8991d4577545ae3a4b0694f870/mooncake-store/src/storage_backend.cpp#L1952 There is a confusing assignment logic after invoking `uring_file->read_aligned()`: ```C++ #ifdef USE_URING // Try to use read_aligned for O_DIRECT I/O if file is UringFile UringFile* uring_file = dynamic_cast<UringFile*>(file.get()); if (uring_file != nullptr) { ... ... read_res = uring_file->read_aligned( plan.dest_slice.ptr, aligned_size, aligned_offset); if (read_res) { ... ... read_res = plan.dest_slice.size; } } else #endif ... ... if (read_res.value() != plan.dest_slice.size) { LOG(ERROR) << "Read size mismatch for key: " << plan.key << ", expected: " << plan.dest_slice.size << ", got: " << read_res.value(); return tl::make_unexpected(ErrorCode::FILE_READ_FAIL); } } } ``` The variable `read_res` which stores the actual IO byte count returned by io_uring is overwritten by the expected full slice size. All subsequent short-read validation logic based on `read_res` will always evaluate to `false`: - The code cannot detect real short reads from the underlying io_uring layer - No `ErrorCode::FILE_READ_FAIL` will be propagated to upstream - Silent truncated data is passed to business logic without any alert ## Questions 1. What is the design intention of overwriting `read_res = plan.dest_slice.size` at line 1952? Why override the real IO read result returned by `read_aligned()`? 2. How does Mooncake guarantee data correctness for the io_uring offload path? 3. Are there existing safeguards (buffer cleanup, strict short-read rejection, mandatory retries) to avoid silent stale-buffer data corruption? ### Before submitting... - [x] Ensure you searched for relevant issues and read the [documentation]

bsakaguchi

2026-10-29 21:19:32 +00:00

closed this issue
added the
bug
label

bsakaguchi commented

2026-10-29 21:19:33 +00:00

Author

Owner

@github-actions[bot] commented on GitHub (Jul 23, 2026):

Thanks for opening this issue, @waliwali777!

Field	Value
Issue	#3066
GitHub user ID	`51405922`
Reporter	@waliwali777

A maintainer will triage this when possible. To help us respond faster, please include:

Mooncake version or commit SHA
Environment (OS, CUDA/driver, RDMA stack if relevant)
Steps to reproduce and expected vs. actual behavior

Useful links: Documentation · Contributing guide

This message was posted automatically by the issue bot.

@github-actions[bot] commented on GitHub (Jul 23, 2026): Thanks for opening this issue, @waliwali777! | Field | Value | |-------|-------| | **Issue** | #3066 | | **GitHub user ID** | `51405922` | | **Reporter** | @waliwali777 | A maintainer will triage this when possible. To help us respond faster, please include: - Mooncake version or commit SHA - Environment (OS, CUDA/driver, RDMA stack if relevant) - Steps to reproduce and expected vs. actual behavior Useful links: [Documentation](https://kvcache-ai.github.io/Mooncake/) · [Contributing guide](https://github.com/kvcache-ai/Mooncake/blob/main/CONTRIBUTING.md) > This message was posted automatically by the issue bot.

bsakaguchi commented

2026-10-29 21:19:33 +00:00

Author

Owner

@waliwali777 commented on GitHub (Jul 23, 2026):

@zhangzuo21 、@ykwd
https://github.com/kvcache-ai/Mooncake/pull/1500
hi～能帮忙看一下这个 issue 吗？感谢！

@waliwali777 commented on GitHub (Jul 23, 2026): @zhangzuo21 、@ykwd https://github.com/kvcache-ai/Mooncake/pull/1500 hi～能帮忙看一下这个 issue 吗？感谢！

bsakaguchi commented

2026-10-29 21:19:33 +00:00

Author

Owner

@LujhCoconut commented on GitHub (Jul 23, 2026):

Thanks for the detailed report. Confirmed this is a real issue.

I actually addressed this in PR #2502, but that PR bundled additional optimizations that introduced a design trade-off (replacing zero-copy with a temp buffer + memcpy), which needs further consideration before merging. I'll extract just the two core fixes — stale CQE filtering in collect() and expected_total validation in BatchLoad — from #2502 into a separate, focused PR to resolve this issue.

For context, my fix in #2502 worked at two layers:

At the io_uring layer (uring_file.cpp): Tagged each batch submission with a monotonic op_id via sqe->user_data, and rewrote collect() to use io_uring_wait_cqe in a loop — filtering out CQEs whose user_data didn't match. This prevents stale CQEs left by a previously failed batch_read from contaminating the current operation's byte count.
At the storage backend layer (storage_backend.cpp): After batch_read returns, compare batch_res.value() against a pre-computed expected_total (sum of all ReadDesc.len). If they don't match, fail with FILE_READ_FAIL instead of silently proceeding with truncated data.

These two guard each other: the expected_total check is the safety net for short reads; the user_data filter ensures that safety net isn't bypassed by stale CQE noise making the numbers coincidentally add up.

Happy to discuss further.

@LujhCoconut commented on GitHub (Jul 23, 2026): Thanks for the detailed report. Confirmed this is a real issue. I actually addressed this in PR #2502, but that PR bundled additional optimizations that introduced a design trade-off (replacing zero-copy with a temp buffer + memcpy), which needs further consideration before merging. I'll extract just the two core fixes — stale CQE filtering in `collect()` and `expected_total` validation in BatchLoad — from #2502 into a separate, focused PR to resolve this issue. For context, my fix in #2502 worked at two layers: * At the io_uring layer (`uring_file.cpp`): Tagged each batch submission with a monotonic `op_id` via `sqe->user_data`, and rewrote `collect()` to use `io_uring_wait_cqe` in a loop — filtering out CQEs whose `user_data` didn't match. This prevents stale CQEs left by a previously failed `batch_read` from contaminating the current operation's byte count. * At the storage backend layer (`storage_backend.cpp`): After `batch_read` returns, compare `batch_res.value()` against a pre-computed `expected_total` (sum of all `ReadDesc.len`). If they don't match, fail with `FILE_READ_FAIL` instead of silently proceeding with truncated data. These two guard each other: the `expected_total` check is the safety net for short reads; the `user_data` filter ensures that safety net isn't bypassed by stale CQE noise making the numbers coincidentally add up. Happy to discuss further.

bsakaguchi commented

2026-10-29 21:19:34 +00:00

Author

Owner

@waliwali777 commented on GitHub (Jul 23, 2026):

Thanks a lot for your response and the corresponding fixes.

Besides short-read induced silent data corruption, we are also facing identical invisible data inconsistency issues caused by abnormal over-length reads in our test environment.

We have observed a counterintuitive behavior occasionally: the value of cqe->res (actual bytes read from the kernel) can be larger than the chunk length stored in cqe->user_data. We are confused about what causes this scenario. Could you clarify whether the current fixes will also resolve this correctness risk?

@waliwali777 commented on GitHub (Jul 23, 2026): Thanks a lot for your response and the corresponding fixes. Besides short-read induced silent data corruption, we are also facing identical invisible data inconsistency issues caused by abnormal over-length reads in our test environment. We have observed a counterintuitive behavior occasionally: the value of `cqe->res` (actual bytes read from the kernel) can be larger than the chunk length stored in `cqe->user_data`. We are confused about what causes this scenario. Could you clarify whether the current fixes will also resolve this correctness risk?

bsakaguchi commented

2026-10-29 21:19:34 +00:00

Author

Owner

@LujhCoconut commented on GitHub (Jul 23, 2026):

@waliwali777

Yes, the same fixes also resolve the over-length-read case.

The observed cqe->res > current chunk size is also caused by stale CQEs, not by a kernel bug. Clarification: in the fix cqe->user_data stores a per-operation op_id, not the chunk size.

Typical sequence:

A previous submit_rw / batch_read submits N SQEs and returns early because collect() reports an I/O error. Note that collect() itself consumes the expected CQEs before returning; it is the caller that aborts, leaving in-flight SQEs from the failed operation.
Those late completions land in the CQ as stale CQEs tagged with the old op_id.
The next operation submits smaller reads (e.g., 4 KB chunks). The old collect() used io_uring_for_each_cqe without filtering, so it picked up the stale CQE whose original request was much larger (e.g., 1 MB).
That stale cqe->res (up to 1 MB) was added to the current operation's total, giving the illusion that a single current chunk returned more bytes than requested. The original 1 MB read completion may also have overflowed a buffer that has since been freed or reused.

Fix #2 stamps every SQE with a monotonic op_id and collect() discards any CQE whose user_data does not match:

if (cqe->user_data != op_id) {
    io_uring_cq_advance(&ring_, 1);
    continue;
}

So stale CQEs can no longer inflate the current operation's byte count, which eliminates both the short-read and the over-length-read silent corruptions.

Combined with Fix #1 (min_required validation in storage_backend.cpp), the current operation only sees completions it actually submitted and must satisfy the real length requirement.

@LujhCoconut commented on GitHub (Jul 23, 2026): @waliwali777 Yes, the same fixes also resolve the over-length-read case. The observed `cqe->res > current chunk size` is also caused by stale CQEs, not by a kernel bug. Clarification: in the fix `cqe->user_data` stores a per-operation `op_id`, not the chunk size. Typical sequence: 1. A previous `submit_rw` / `batch_read` submits N SQEs and returns early because `collect()` reports an I/O error. Note that `collect()` itself consumes the expected CQEs before returning; it is the caller that aborts, leaving in-flight SQEs from the failed operation. 2. Those late completions land in the CQ as stale CQEs tagged with the old `op_id`. 3. The next operation submits smaller reads (e.g., 4 KB chunks). The old `collect()` used `io_uring_for_each_cqe` without filtering, so it picked up the stale CQE whose original request was much larger (e.g., 1 MB). 4. That stale `cqe->res` (up to 1 MB) was added to the current operation's `total`, giving the illusion that a single current chunk returned more bytes than requested. The original 1 MB read completion may also have overflowed a buffer that has since been freed or reused. Fix #2 stamps every SQE with a monotonic `op_id` and `collect()` discards any CQE whose `user_data` does not match: ```cpp if (cqe->user_data != op_id) { io_uring_cq_advance(&ring_, 1); continue; } ``` So stale CQEs can no longer inflate the current operation's byte count, which eliminates both the short-read and the over-length-read silent corruptions. Combined with Fix #1 (min_required validation in storage_backend.cpp), the current operation only sees completions it actually submitted and must satisfy the real length requirement.

bsakaguchi referenced this issue

2026-10-29 21:30:38 +00:00

[PR #789] [MERGED] [coro_rpc] use client pool and enable rdma #1824