| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CADUfDZoBrnDpnTOxiDq6pBkctJ3NDJq7Wcqm2pUu_ooqMy8yyw@mail.gmail.com>
Date: Sun, 20 Jul 2025 15:10:28 -0400
From: Caleb Sander Mateos <csander@...estorage.com>
To: Sidong Yang <sidong.yang@...iosa.ai>
Cc: Miguel Ojeda <ojeda@...nel.org>, Arnd Bergmann <arnd@...db.de>, Jens Axboe <axboe@...nel.dk>,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
io-uring@...r.kernel.org
Subject: Re: [PATCH 2/4] rust: io_uring: introduce rust abstraction for
io-uring cmd
On Sat, Jul 19, 2025 at 10:34 AM Sidong Yang <sidong.yang@...iosa.ai> wrote:
>
> This patch introduces rust abstraction for io-uring sqe, cmd. IoUringSqe
> abstracts io_uring_sqe and it has cmd_data(). and IoUringCmd is
> abstraction for io_uring_cmd. From this, user can get cmd_op, flags,
> pdu and also sqe.
>
> Signed-off-by: Sidong Yang <sidong.yang@...iosa.ai>
> ---
> rust/kernel/io_uring.rs | 114 ++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 1 +
> 2 files changed, 115 insertions(+)
> create mode 100644 rust/kernel/io_uring.rs
>
> diff --git a/rust/kernel/io_uring.rs b/rust/kernel/io_uring.rs
> new file mode 100644
> index 000000000000..7843effbedb4
> --- /dev/null
> +++ b/rust/kernel/io_uring.rs
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2025 Furiosa AI.
> +
> +//! Files and file descriptors.
> +//!
> +//! C headers: [`include/linux/io_uring/cmd.h`](srctree/include/linux/io_uring/cmd.h) and
> +//! [`include/linux/file.h`](srctree/include/linux/file.h)
> +
> +use core::mem::MaybeUninit;
> +
> +use crate::{fs::File, types::Opaque};
> +
> +pub mod flags {
> + pub const COMPLETE_DEFER: i32 = bindings::io_uring_cmd_flags_IO_URING_F_COMPLETE_DEFER;
> + pub const UNLOCKED: i32 = bindings::io_uring_cmd_flags_IO_URING_F_UNLOCKED;
> +
> + pub const MULTISHOT: i32 = bindings::io_uring_cmd_flags_IO_URING_F_MULTISHOT;
> + pub const IOWQ: i32 = bindings::io_uring_cmd_flags_IO_URING_F_IOWQ;
> + pub const NONBLOCK: i32 = bindings::io_uring_cmd_flags_IO_URING_F_NONBLOCK;
> +
> + pub const SQE128: i32 = bindings::io_uring_cmd_flags_IO_URING_F_SQE128;
> + pub const CQE32: i32 = bindings::io_uring_cmd_flags_IO_URING_F_CQE32;
> + pub const IOPOLL: i32 = bindings::io_uring_cmd_flags_IO_URING_F_IOPOLL;
> +
> + pub const CANCEL: i32 = bindings::io_uring_cmd_flags_IO_URING_F_CANCEL;
> + pub const COMPAT: i32 = bindings::io_uring_cmd_flags_IO_URING_F_COMPAT;
> + pub const TASK_DEAD: i32 = bindings::io_uring_cmd_flags_IO_URING_F_TASK_DEAD;
> +}
> +
> +#[repr(transparent)]
> +pub struct IoUringCmd {
> + inner: Opaque<bindings::io_uring_cmd>,
> +}
> +
> +impl IoUringCmd {
> + /// Returns the cmd_op with associated with the io_uring_cmd.
> + #[inline]
> + pub fn cmd_op(&self) -> u32 {
> + // SAFETY: The call guarantees that the pointer is not dangling and stays valid
> + unsafe { (*self.inner.get()).cmd_op }
> + }
> +
> + /// Returns the flags with associated with the io_uring_cmd.
> + #[inline]
> + pub fn flags(&self) -> u32 {
> + // SAFETY: The call guarantees that the pointer is not dangling and stays valid
> + unsafe { (*self.inner.get()).flags }
> + }
> +
> + /// Returns the ref pdu for free use.
> + #[inline]
> + pub fn pdu(&mut self) -> MaybeUninit<&mut [u8; 32]> {
Should be &mut MaybeUninit, right? It's the bytes that may be
uninitialized, not the reference.
> + // SAFETY: The call guarantees that the pointer is not dangling and stays valid
> + unsafe { MaybeUninit::new(&mut (*self.inner.get()).pdu) }
> + }
> +
> + /// Constructs a new `struct io_uring_cmd` wrapper from a file descriptor.
Why "from a file descriptor"?
Also, missing a comment documenting the safety preconditions?
> + #[inline]
> + pub unsafe fn from_raw<'a>(ptr: *const bindings::io_uring_cmd) -> &'a IoUringCmd {
Could take NonNull instead of a raw pointer.
> + // SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the
> + // duration of 'a. The cast is okay because `File` is `repr(transparent)`.
"File" -> "IoUringCmd"?
> + unsafe { &*ptr.cast() }
> + }
> +
> + // Returns the file that referenced by uring cmd self.
I had a hard time parsing this comment. How about "Returns a reference
to the uring cmd's file object"?
> + #[inline]
> + pub fn file<'a>(&'a self) -> &'a File {
Could elide the lifetime.
> + // SAFETY: The call guarantees that the pointer is not dangling and stays valid
> + let file = unsafe { (*self.inner.get()).file };
> + unsafe { File::from_raw_file(file) }
Missing a SAFETY comment for File::from_raw_file()? I would expect
something about io_uring_cmd's file field storing a non-null pointer
to a struct file on which a reference is held for the duration of the
uring cmd.
> + }
> +
> + // Returns the sqe that referenced by uring cmd self.
"Returns a reference to the uring cmd's SQE"?
> + #[inline]
> + pub fn sqe(&self) -> &IoUringSqe {
> + // SAFETY: The call guarantees that the pointer is not dangling and stays valid
> + let ptr = unsafe { (*self.inner.get()).sqe };
"ptr" isn't very descriptive. How about "sqe"?
> + unsafe { IoUringSqe::from_raw(ptr) }
Similar, missing SAFETY comment for IoUringSqe::from_raw()?
> + }
> +
> + // Called by consumers of io_uring_cmd, if they originally returned -EIOCBQUEUED upon receiving the command
> + #[inline]
> + pub fn done(self, ret: isize, res2: u64, issue_flags: u32) {
I don't think it's safe to move io_uring_cmd. io_uring_cmd_done(), for
example, calls cmd_to_io_kiocb() to turn struct io_uring_cmd *ioucmd
into struct io_kiocb *req via a pointer cast. And struct io_kiocb's
definitely need to be pinned in memory. For example,
io_req_normal_work_add() inserts the struct io_kiocb into a linked
list. Probably some sort of pinning is necessary for IoUringCmd.
> + // SAFETY: The call guarantees that the pointer is not dangling and stays valid
> + unsafe {
> + bindings::io_uring_cmd_done(self.inner.get(), ret, res2, issue_flags);
> + }
> + }
> +}
> +
> +#[repr(transparent)]
> +pub struct IoUringSqe {
> + inner: Opaque<bindings::io_uring_sqe>,
> +}
> +
> +impl<'a> IoUringSqe {
> + pub fn cmd_data(&'a self) -> &'a [Opaque<u8>] {
> + // SAFETY: The call guarantees that the pointer is not dangling and stays valid
> + unsafe {
> + let cmd = (*self.inner.get()).__bindgen_anon_6.cmd.as_ref();
> + core::slice::from_raw_parts(cmd.as_ptr() as *const Opaque<u8>, 8)
Why 8? Should be 16 bytes for a 64-byte SQE and 80 bytes for a
128-byte SQE, right?
> + }
> + }
> +
> + #[inline]
> + pub unsafe fn from_raw(ptr: *const bindings::io_uring_sqe) -> &'a IoUringSqe {
Take NonNull here too?
> + // SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the
> + // duration of 'a. The cast is okay because `File` is `repr(transparent)`.
> + //
> + // INVARIANT: The caller guarantees that there are no problematic `fdget_pos` calls.
Why "File" and "fdget_pos"?
Best,
Caleb
> + unsafe { &*ptr.cast() }
> + }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6b4774b2b1c3..fb310e78d51d 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -80,6 +80,7 @@
> pub mod fs;
> pub mod init;
> pub mod io;
> +pub mod io_uring;
> pub mod ioctl;
> pub mod jump_label;
> #[cfg(CONFIG_KUNIT)]
> --
> 2.43.0
>
>
Powered by blists - more mailing lists