7901: fs: add unstable io_uring uring_cmd support for File#65
7901: fs: add unstable io_uring uring_cmd support for File#65martin-augment wants to merge 1 commit intomasterfrom
Conversation
Summary of ChangesHello @martin-augment, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces unstable support for the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughThis pull request adds io_uring command support to Tokio's File API on Linux. A new ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pull Request Review: Add unstable io_uring uring_cmd support for FileSummaryThis PR adds support for Code Quality ✅Structure and Organization
Documentation
Potential Issues
|
![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
The pull request introduces io_uring command support for File operations in Tokio, specifically IORING_OP_URING_CMD, leveraging low-level kernel features on Linux. While the implementation appears well-structured, a critical security vulnerability has been identified. The current implementation exposes this powerful functionality through a safe API, which violates Rust's memory safety guarantees. Since the command payload can contain arbitrary pointers that the kernel will dereference, the uring_cmd method must be marked as unsafe to ensure callers are aware of the responsibility to provide valid memory addresses.
| pub async fn uring_cmd( | ||
| &self, | ||
| cmd_op: u32, | ||
| cmd: [u8; 16], | ||
| buf_index: Option<u16>, | ||
| ) -> io::Result<u32> { |
There was a problem hiding this comment.
The uring_cmd method is a public, safe API that allows submitting arbitrary 16-byte command payloads to the kernel via io_uring's IORING_OP_URING_CMD. Many implementations of this operation (such as the NVMe driver) interpret the 16-byte payload as containing pointers to memory buffers. Because uring_cmd is marked as a safe function, it allows a user to provide a payload that contains arbitrary memory addresses, which the kernel will then attempt to access. This can lead to memory safety violations (e.g., use-after-free or data corruption) if the addresses point to invalid or out-of-scope memory. In Rust, any function that can lead to memory unsafety depending on its input parameters must be marked as unsafe.
| pub async fn uring_cmd( | |
| &self, | |
| cmd_op: u32, | |
| cmd: [u8; 16], | |
| buf_index: Option<u16>, | |
| ) -> io::Result<u32> { | |
| pub async unsafe fn uring_cmd( | |
| &self, | |
| cmd_op: u32, | |
| cmd: [u8; 16], | |
| buf_index: Option<u16>, | |
| ) -> io::Result<u32> { |
There was a problem hiding this comment.
value:useful; category:documentation; feedback: The Gemini AI reviewer is correct! It seems the new method would be quite powerful and it will be possible to execute privileged operations thru it. Marking it as unsafe would signal to its users that they should make sure that it does what they need.
| )); | ||
| } | ||
|
|
||
| let fd: OwnedFd = self.std.try_clone()?.into(); |
There was a problem hiding this comment.
The fd variable is created as an OwnedFd but then immediately converted to types::Fd (which is RawFd) in Op::uring_cmd16. The OwnedFd is then moved into the UringCmd struct. This seems redundant. It would be more efficient to pass the OwnedFd directly to Op::uring_cmd16 and let UringCmd own it from the start, avoiding the try_clone() and into() calls if Op::uring_cmd16 can accept OwnedFd directly or handle the conversion internally without an extra clone.
There was a problem hiding this comment.
value:annoying; category:documentation; feedback: The Gemini AI reviewer is not correct! The proposed improvement is exactly the same as it is implemented.
| } | ||
|
|
||
| let fd: OwnedFd = self.std.try_clone()?.into(); | ||
| let (res, _fd) = Op::uring_cmd16(fd, cmd_op, cmd, buf_index).await; |
There was a problem hiding this comment.
The _fd variable is unused. While it's common to prefix unused variables with an underscore, it's good practice to remove them if they serve no purpose to avoid confusion and potential resource leaks if the OwnedFd was not properly handled by the Op.
| let (res, _fd) = Op::uring_cmd16(fd, cmd_op, cmd, buf_index).await; | |
| let res = Op::uring_cmd16(fd, cmd_op, cmd, buf_index).await.0; | |
There was a problem hiding this comment.
value:valid-but-wont-fix; category:documentation; feedback: The Gemini AI reviewer is not correct! The method returns a tuple and the used syntax is explicit that the second item is not needed and it will be ignored. Both solutions work the same way. It is matter of taste.
| type Output = (io::Result<u32>, OwnedFd); | ||
|
|
||
| fn complete(self, cqe: CqeResult) -> Self::Output { | ||
| (cqe.result, self.fd) |
There was a problem hiding this comment.
The cqe.result is directly returned as io::Result<u32>. It's important to ensure that cqe.result correctly represents the return value of the IORING_OP_URING_CMD operation. The io_uring documentation states that the result field contains the return value of the system call, which for URING_CMD is typically an integer. This seems correct, but it's worth double-checking the expected return type and potential error mapping for IORING_OP_URING_CMD to ensure it aligns with u32.
| let sqe = uring_cmd.build(); | ||
|
|
||
| // SAFETY: Parameters are valid for the entire duration of the operation | ||
| unsafe { Op::new(sqe, UringCmd { fd }) } |
There was a problem hiding this comment.
The Op::new function is marked as unsafe. The comment states "Parameters are valid for the entire duration of the operation." This is a critical safety invariant. It's important to explicitly document how UringCmd ensures the fd (which is OwnedFd) remains valid and is not dropped prematurely or used after being moved, especially since OwnedFd is moved into UringCmd and then UringCmd is moved into Op. The OwnedFd is correctly returned in complete and complete_with_error, ensuring its lifetime is managed. This looks correct, but a more detailed comment on how the OwnedFd's lifetime is managed within the Op and UringCmd would be beneficial for future maintainers.
There was a problem hiding this comment.
value:good-to-have; category:documentation; feedback: The Gemini AI reviewer is correct! More information why the parameters are valid thru the entire duration of the operation would be helpful for the maintainers and the users of the API.
| let (tmp_file, _path): (Vec<NamedTempFile>, Vec<PathBuf>) = create_tmp_files(1); | ||
| let file = File::open(tmp_file[0].path()).await.unwrap(); | ||
|
|
||
| let _ = file.uring_cmd(0, [0; 16], None).await; |
There was a problem hiding this comment.
The test uring_cmd_is_available_via_file calls uring_cmd but discards the result with let _ = .... While this test primarily checks for availability and successful execution without panicking, it would be more robust to assert something about the result, even if it's just is_ok(). This would confirm that the operation completed successfully from the kernel's perspective, rather than just not crashing.
let res = file.uring_cmd(0, [0; 16], None).await;
assert!(res.is_ok(), "uring_cmd operation failed: {:?}", res);There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! Currently the test ignores the result. It would be good to assert that it is successful or not, at least.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tokio/tests/fs_uring.rs (1)
148-154: Test only verifies compilation, not behavior.The result is silently discarded (
let _ = ...), so this test passes even if the operation returns an error. Consider asserting at least that the result is anErr(since a regular file doesn't supporturing_cmd), to guard against silent regressions:- let _ = file.uring_cmd(0, [0; 16], None).await; + // Regular files do not support uring_cmd; expect Unsupported or a kernel error. + let res = file.uring_cmd(0, [0; 16], None).await; + assert!(res.is_err(), "uring_cmd on a plain file should fail");This would also catch the scenario where
check_and_initincorrectly returnstruefor an unsupported opcode.
🤖 Augment PR SummarySummary: Adds unstable support for submitting io_uring Changes:
Technical Notes: The implementation probes runtime support via 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
|
|
||
| let mut inner = self.inner.lock().await; | ||
| inner.complete_inflight().await; | ||
| drop(inner); |
There was a problem hiding this comment.
Dropping the inner lock before awaiting the io_uring op means other File ops can start while the uring command is still in-flight, so the earlier complete_inflight() doesn’t actually ensure exclusivity. Is it intended that uring_cmd can overlap with subsequent reads/writes/seeks on the same File?
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Augment AI reviewer is correct! The lock is dropped early and this allows other ops to be executed in the same time. This moves the responsibility to the application developer to make sure the operations are properly synchronized when needed.
| drop(inner); | ||
|
|
||
| let handle = crate::runtime::Handle::current(); | ||
| let driver_handle = handle.inner.driver().io(); |
There was a problem hiding this comment.
handle.inner.driver().io() will panic if the runtime was built without the I/O driver enabled, but the docstring only describes returning ErrorKind::Unsupported when io_uring/opcode isn’t available. Consider clarifying that uring_cmd also requires the runtime I/O driver to be enabled (or otherwise ensuring it fails with an io::Error rather than panicking).
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Augment AI reviewer is correct! The docstring of the method lists most of the prerequisites but it misses the Builder::enable_io() call. Without it the IO driver won't be usable.
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! Currently the test ignores the result. It would be good to assert that it is successful or not, at least. |
2 participants
7901: To review by AI