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@furiosa.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]> {
+        // 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.
+    #[inline]
+    pub unsafe fn from_raw<'a>(ptr: *const bindings::io_uring_cmd) -> &'a IoUringCmd {
+        // 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)`.
+        unsafe { &*ptr.cast() }
+    }
+
+    // Returns the file that referenced by uring cmd self.
+    #[inline]
+    pub fn file<'a>(&'a self) -> &'a File {
+        // 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) }
+    }
+
+    // Returns the sqe  that referenced by uring cmd self.
+    #[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 };
+        unsafe { IoUringSqe::from_raw(ptr) }
+    }
+
+    // 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) {
+        // 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)
+        }
+    }
+
+    #[inline]
+    pub unsafe fn from_raw(ptr: *const bindings::io_uring_sqe) -> &'a IoUringSqe {
+        // 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.
+        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

On Sat, Jul 19, 2025 at 10:34 AM Sidong Yang <sidong.yang@furiosa.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@furiosa.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
>
>

On Sun, Jul 20, 2025 at 03:10:28PM -0400, Caleb Sander Mateos wrote:
> On Sat, Jul 19, 2025 at 10:34 AM Sidong Yang <sidong.yang@furiosa.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@furiosa.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.

Yes, it should be &mut MaybeUninit.
> 
> > +        // 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?

Yes, it's a mistake in comment and also ptr should be NonNull.
> 
> > +    #[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"?

Agreed. thanks.
> 
> > +    #[inline]
> > +    pub fn file<'a>(&'a self) -> &'a File {
> 
> Could elide the lifetime.

Thanks, I didn't know about this.
> 
> > +        // 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.

Yes, I missed. thanks.
> 
> > +    }
> > +
> > +    // Returns the sqe  that referenced by uring cmd self.
> 
> "Returns a reference to the uring cmd's SQE"?

Agreed, thanks.
> 
> > +    #[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"?

Sounds good.
> 
> > +        unsafe { IoUringSqe::from_raw(ptr) }
> 
> Similar, missing SAFETY comment for IoUringSqe::from_raw()?

Thanks. I missed.
> 
> > +    }
> > +
> > +    // 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.

Understood, Normally the users wouldn't create IoUringCmd than use borrowed cmd
in uring_cmd() callback. How about change to &mut self and also uring_cmd provides
&mut IoUringCmd for arg.

> 
> > +        // 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?

Yes, it should be 16 bytes or 80 bytes. I'll fix this.

> 
> > +        }
> > +    }
> > +
> > +    #[inline]
> > +    pub unsafe fn from_raw(ptr: *const bindings::io_uring_sqe) -> &'a IoUringSqe {
> 
> Take NonNull here too?

Yes, Thanks.
> 
> > +        // 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"?

It's a bad mistake. thanks!

Thank you so much for deatiled review!

Thanks,
Sidong
> 
> 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
> >
> >

On Mon, Jul 21, 2025 at 1:23 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
>
> On Sun, Jul 20, 2025 at 03:10:28PM -0400, Caleb Sander Mateos wrote:
> > On Sat, Jul 19, 2025 at 10:34 AM Sidong Yang <sidong.yang@furiosa.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@furiosa.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.
>
> Yes, it should be &mut MaybeUninit.
> >
> > > +        // 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?
>
> Yes, it's a mistake in comment and also ptr should be NonNull.
> >
> > > +    #[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"?
>
> Agreed. thanks.
> >
> > > +    #[inline]
> > > +    pub fn file<'a>(&'a self) -> &'a File {
> >
> > Could elide the lifetime.
>
> Thanks, I didn't know about this.
> >
> > > +        // 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.
>
> Yes, I missed. thanks.
> >
> > > +    }
> > > +
> > > +    // Returns the sqe  that referenced by uring cmd self.
> >
> > "Returns a reference to the uring cmd's SQE"?
>
> Agreed, thanks.
> >
> > > +    #[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"?
>
> Sounds good.
> >
> > > +        unsafe { IoUringSqe::from_raw(ptr) }
> >
> > Similar, missing SAFETY comment for IoUringSqe::from_raw()?
>
> Thanks. I missed.
> >
> > > +    }
> > > +
> > > +    // 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.
>
> Understood, Normally the users wouldn't create IoUringCmd than use borrowed cmd
> in uring_cmd() callback. How about change to &mut self and also uring_cmd provides
> &mut IoUringCmd for arg.

I'm still a little worried about exposing &mut IoUringCmd without
pinning. It would allow swapping the fields of two IoUringCmd's (and
therefore struct io_uring_cmd's), for example. If a struct
io_uring_cmd belongs to a struct io_kiocb linked into task_list,
swapping it with another struct io_uring_cmd would result in
io_uring_cmd_work() being invoked on the wrong struct io_uring_cmd.
Maybe it would be okay if IoUringCmd had an invariant that the struct
io_uring_cmd is not on the task work list. But I would feel safer with
using Pin<&mut IoUringCmd>. I don't have much experience with Rust in
the kernel, though, so I would welcome other opinions.

Best,
Caleb

>
> >
> > > +        // 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?
>
> Yes, it should be 16 bytes or 80 bytes. I'll fix this.
>
> >
> > > +        }
> > > +    }
> > > +
> > > +    #[inline]
> > > +    pub unsafe fn from_raw(ptr: *const bindings::io_uring_sqe) -> &'a IoUringSqe {
> >
> > Take NonNull here too?
>
> Yes, Thanks.
> >
> > > +        // 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"?
>
> It's a bad mistake. thanks!
>
> Thank you so much for deatiled review!
>
> Thanks,
> Sidong
> >
> > 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
> > >
> > >

On Mon Jul 21, 2025 at 5:04 PM CEST, Caleb Sander Mateos wrote:
> On Mon, Jul 21, 2025 at 1:23 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
>> On Sun, Jul 20, 2025 at 03:10:28PM -0400, Caleb Sander Mateos wrote:
>> > On Sat, Jul 19, 2025 at 10:34 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
>> > > +    }
>> > > +
>> > > +    // 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.
>>
>> Understood, Normally the users wouldn't create IoUringCmd than use borrowed cmd
>> in uring_cmd() callback. How about change to &mut self and also uring_cmd provides
>> &mut IoUringCmd for arg.
>
> I'm still a little worried about exposing &mut IoUringCmd without
> pinning. It would allow swapping the fields of two IoUringCmd's (and
> therefore struct io_uring_cmd's), for example. If a struct
> io_uring_cmd belongs to a struct io_kiocb linked into task_list,
> swapping it with another struct io_uring_cmd would result in
> io_uring_cmd_work() being invoked on the wrong struct io_uring_cmd.
> Maybe it would be okay if IoUringCmd had an invariant that the struct
> io_uring_cmd is not on the task work list. But I would feel safer with
> using Pin<&mut IoUringCmd>. I don't have much experience with Rust in
> the kernel, though, so I would welcome other opinions.

Pinning in the kernel isn't much different from userspace. From your
description of what normally happens with `struct io_uring_cmd`, it
definitely must be pinned.

From a quick glance at the patch series, I don't see a way to create a
`IoUringCmd` by-value, which also means that the `done` function won't
be callable (also the `fn pdu(&mut self)` function won't be callable,
since you only ever create a `&IoUringCmd`). I'm not sure if I'm missing
something, do you plan on further patches in the future?

How (aside from `from_raw`) are `IoUringCmd` values going to be created
or exposed to the user?

---
Cheers,
Benno

On Mon, Jul 21, 2025 at 05:52:41PM +0200, Benno Lossin wrote:
> On Mon Jul 21, 2025 at 5:04 PM CEST, Caleb Sander Mateos wrote:
> > On Mon, Jul 21, 2025 at 1:23 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >> On Sun, Jul 20, 2025 at 03:10:28PM -0400, Caleb Sander Mateos wrote:
> >> > On Sat, Jul 19, 2025 at 10:34 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >> > > +    }
> >> > > +
> >> > > +    // 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.
> >>
> >> Understood, Normally the users wouldn't create IoUringCmd than use borrowed cmd
> >> in uring_cmd() callback. How about change to &mut self and also uring_cmd provides
> >> &mut IoUringCmd for arg.
> >
> > I'm still a little worried about exposing &mut IoUringCmd without
> > pinning. It would allow swapping the fields of two IoUringCmd's (and
> > therefore struct io_uring_cmd's), for example. If a struct
> > io_uring_cmd belongs to a struct io_kiocb linked into task_list,
> > swapping it with another struct io_uring_cmd would result in
> > io_uring_cmd_work() being invoked on the wrong struct io_uring_cmd.
> > Maybe it would be okay if IoUringCmd had an invariant that the struct
> > io_uring_cmd is not on the task work list. But I would feel safer with
> > using Pin<&mut IoUringCmd>. I don't have much experience with Rust in
> > the kernel, though, so I would welcome other opinions.
> 
> Pinning in the kernel isn't much different from userspace. From your
> description of what normally happens with `struct io_uring_cmd`, it
> definitely must be pinned.
> 
> From a quick glance at the patch series, I don't see a way to create a
> `IoUringCmd` by-value, which also means that the `done` function won't
> be callable (also the `fn pdu(&mut self)` function won't be callable,
> since you only ever create a `&IoUringCmd`). I'm not sure if I'm missing
> something, do you plan on further patches in the future?

Sure, this version is full of nonsence. v2 will be better than this.

> 
> How (aside from `from_raw`) are `IoUringCmd` values going to be created
> or exposed to the user?

Nomrally user would gets Pin<&mut IoUringCmd> from MiscDevice::uring_cmd().

Thanks,
Sidong

> 
> ---
> Cheers,
> Benno

On Mon, Jul 21, 2025 at 11:04:31AM -0400, Caleb Sander Mateos wrote:
> On Mon, Jul 21, 2025 at 1:23 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >
> > On Sun, Jul 20, 2025 at 03:10:28PM -0400, Caleb Sander Mateos wrote:
> > > On Sat, Jul 19, 2025 at 10:34 AM Sidong Yang <sidong.yang@furiosa.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@furiosa.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.
> >
> > Yes, it should be &mut MaybeUninit.
> > >
> > > > +        // 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?
> >
> > Yes, it's a mistake in comment and also ptr should be NonNull.
> > >
> > > > +    #[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"?
> >
> > Agreed. thanks.
> > >
> > > > +    #[inline]
> > > > +    pub fn file<'a>(&'a self) -> &'a File {
> > >
> > > Could elide the lifetime.
> >
> > Thanks, I didn't know about this.
> > >
> > > > +        // 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.
> >
> > Yes, I missed. thanks.
> > >
> > > > +    }
> > > > +
> > > > +    // Returns the sqe  that referenced by uring cmd self.
> > >
> > > "Returns a reference to the uring cmd's SQE"?
> >
> > Agreed, thanks.
> > >
> > > > +    #[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"?
> >
> > Sounds good.
> > >
> > > > +        unsafe { IoUringSqe::from_raw(ptr) }
> > >
> > > Similar, missing SAFETY comment for IoUringSqe::from_raw()?
> >
> > Thanks. I missed.
A> > >
> > > > +    }
> > > > +
> > > > +    // 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.
> >
> > Understood, Normally the users wouldn't create IoUringCmd than use borrowed cmd
> > in uring_cmd() callback. How about change to &mut self and also uring_cmd provides
> > &mut IoUringCmd for arg.
> 
> I'm still a little worried about exposing &mut IoUringCmd without
> pinning. It would allow swapping the fields of two IoUringCmd's (and
> therefore struct io_uring_cmd's), for example. If a struct
> io_uring_cmd belongs to a struct io_kiocb linked into task_list,
> swapping it with another struct io_uring_cmd would result in
> io_uring_cmd_work() being invoked on the wrong struct io_uring_cmd.
> Maybe it would be okay if IoUringCmd had an invariant that the struct
> io_uring_cmd is not on the task work list. But I would feel safer with
> using Pin<&mut IoUringCmd>. I don't have much experience with Rust in
> the kernel, though, so I would welcome other opinions.

I've thought about this deeply. You're right. exposing &mut without
pinning make it unsafe. User also can make *mut and memmove to anywhere
without unsafe block. It's safest to get NonNull from from_raw and it
returns Pin<&mut IoUringCmd>. from_raw() name is weird. it should be
from_nonnnull()? Also, done() would get Pin<&mut Self>.

Thanks,
Sidong
> 
> Best,
> Caleb
> 
> >
> > >
> > > > +        // 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?
> >
> > Yes, it should be 16 bytes or 80 bytes. I'll fix this.
> >
> > >
> > > > +        }
> > > > +    }
> > > > +
> > > > +    #[inline]
> > > > +    pub unsafe fn from_raw(ptr: *const bindings::io_uring_sqe) -> &'a IoUringSqe {
> > >
> > > Take NonNull here too?
> >
> > Yes, Thanks.
> > >
> > > > +        // 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"?
> >
> > It's a bad mistake. thanks!
> >
> > Thank you so much for deatiled review!
> >
> > Thanks,
> > Sidong
> > >
> > > 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
> > > >
> > > >

On Mon Jul 21, 2025 at 5:47 PM CEST, Sidong Yang wrote:
> On Mon, Jul 21, 2025 at 11:04:31AM -0400, Caleb Sander Mateos wrote:
>> On Mon, Jul 21, 2025 at 1:23 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
>> > On Sun, Jul 20, 2025 at 03:10:28PM -0400, Caleb Sander Mateos wrote:
>> > > On Sat, Jul 19, 2025 at 10:34 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
>> > > > +    }
>> > > > +
>> > > > +    // 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.
>> >
>> > Understood, Normally the users wouldn't create IoUringCmd than use borrowed cmd
>> > in uring_cmd() callback. How about change to &mut self and also uring_cmd provides
>> > &mut IoUringCmd for arg.
>> 
>> I'm still a little worried about exposing &mut IoUringCmd without
>> pinning. It would allow swapping the fields of two IoUringCmd's (and
>> therefore struct io_uring_cmd's), for example. If a struct
>> io_uring_cmd belongs to a struct io_kiocb linked into task_list,
>> swapping it with another struct io_uring_cmd would result in
>> io_uring_cmd_work() being invoked on the wrong struct io_uring_cmd.
>> Maybe it would be okay if IoUringCmd had an invariant that the struct
>> io_uring_cmd is not on the task work list. But I would feel safer with
>> using Pin<&mut IoUringCmd>. I don't have much experience with Rust in
>> the kernel, though, so I would welcome other opinions.
>
> I've thought about this deeply. You're right. exposing &mut without
> pinning make it unsafe.

> User also can make *mut and memmove to anywhere without unsafe block.

How so? Using `*mut T` always needs unsafe.

> It's safest to get NonNull from from_raw and it returns
> Pin<&mut IoUringCmd>.

I don't think you need `NonNull<T>`.

> from_raw() name is weird. it should be from_nonnnull()? Also, done()
> would get Pin<&mut Self>.

That sounds reasonable.

Are you certain that it's an exclusive reference?

---
Cheers,
Benno

On Mon, Jul 21, 2025 at 06:28:09PM +0200, Benno Lossin wrote:
> On Mon Jul 21, 2025 at 5:47 PM CEST, Sidong Yang wrote:
> > On Mon, Jul 21, 2025 at 11:04:31AM -0400, Caleb Sander Mateos wrote:
> >> On Mon, Jul 21, 2025 at 1:23 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >> > On Sun, Jul 20, 2025 at 03:10:28PM -0400, Caleb Sander Mateos wrote:
> >> > > On Sat, Jul 19, 2025 at 10:34 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >> > > > +    }
> >> > > > +
> >> > > > +    // 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.
> >> >
> >> > Understood, Normally the users wouldn't create IoUringCmd than use borrowed cmd
> >> > in uring_cmd() callback. How about change to &mut self and also uring_cmd provides
> >> > &mut IoUringCmd for arg.
> >> 
> >> I'm still a little worried about exposing &mut IoUringCmd without
> >> pinning. It would allow swapping the fields of two IoUringCmd's (and
> >> therefore struct io_uring_cmd's), for example. If a struct
> >> io_uring_cmd belongs to a struct io_kiocb linked into task_list,
> >> swapping it with another struct io_uring_cmd would result in
> >> io_uring_cmd_work() being invoked on the wrong struct io_uring_cmd.
> >> Maybe it would be okay if IoUringCmd had an invariant that the struct
> >> io_uring_cmd is not on the task work list. But I would feel safer with
> >> using Pin<&mut IoUringCmd>. I don't have much experience with Rust in
> >> the kernel, though, so I would welcome other opinions.
> >
> > I've thought about this deeply. You're right. exposing &mut without
> > pinning make it unsafe.
> 
> > User also can make *mut and memmove to anywhere without unsafe block.
> 
> How so? Using `*mut T` always needs unsafe.

You're right. Please forget about this.
> 
> > It's safest to get NonNull from from_raw and it returns
> > Pin<&mut IoUringCmd>.
> 
> I don't think you need `NonNull<T>`.

NonNull<T> gurantees that it's not null. It could be also dangling but it's
safer than *mut T. Could you tell me why I don't need it?

> 
> > from_raw() name is weird. it should be from_nonnnull()? Also, done()
> > would get Pin<&mut Self>.
> 
> That sounds reasonable.
> 
> Are you certain that it's an exclusive reference?

As far as I know, yes.

Thanks,
Sidong
> 
> ---
> Cheers,
> Benno

On Tue Jul 22, 2025 at 4:30 PM CEST, Sidong Yang wrote:
> On Mon, Jul 21, 2025 at 06:28:09PM +0200, Benno Lossin wrote:
>> On Mon Jul 21, 2025 at 5:47 PM CEST, Sidong Yang wrote:
>> > It's safest to get NonNull from from_raw and it returns
>> > Pin<&mut IoUringCmd>.
>> 
>> I don't think you need `NonNull<T>`.
>
> NonNull<T> gurantees that it's not null. It could be also dangling but it's
> safer than *mut T. Could you tell me why I don't need it?

Raw pointers have better ergonomics and if you're just passing it back
into ffi, I don't see the point of using `NonNull`...

>> > from_raw() name is weird. it should be from_nonnnull()? Also, done()
>> > would get Pin<&mut Self>.
>> 
>> That sounds reasonable.
>> 
>> Are you certain that it's an exclusive reference?
>
> As far as I know, yes.

So the `IoUringCmd` is not refcounted and it is also not owned by the
`done` callee?

---
Cheers,
Benno

On Tue, Jul 22, 2025 at 08:52:05PM +0200, Benno Lossin wrote:
> On Tue Jul 22, 2025 at 4:30 PM CEST, Sidong Yang wrote:
> > On Mon, Jul 21, 2025 at 06:28:09PM +0200, Benno Lossin wrote:
> >> On Mon Jul 21, 2025 at 5:47 PM CEST, Sidong Yang wrote:
> >> > It's safest to get NonNull from from_raw and it returns
> >> > Pin<&mut IoUringCmd>.
> >> 
> >> I don't think you need `NonNull<T>`.
> >
> > NonNull<T> gurantees that it's not null. It could be also dangling but it's
> > safer than *mut T. Could you tell me why I don't need it?
> 
> Raw pointers have better ergonomics and if you're just passing it back
> into ffi, I don't see the point of using `NonNull`...

Agreed, from_raw() would be called in rust kernel unsafe condition not in safe
driver. Thinking it over, using a raw pointer would be convenient.

> 
> >> > from_raw() name is weird. it should be from_nonnnull()? Also, done()
> >> > would get Pin<&mut Self>.
> >> 
> >> That sounds reasonable.
> >> 
> >> Are you certain that it's an exclusive reference?
> >
> > As far as I know, yes.
> 
> So the `IoUringCmd` is not refcounted and it is also not owned by the
> `done` callee?

Yes, io_uring_cmd would be allocated in io_submit_sqes() and it's not accessed
concurrently. io_uring_cmd_done() sets its result to io_uring_cmd and prepare
some field to completion.

Thanks,
Sidong

> 
> ---
> Cheers,
> Benno