Message-ID: <CADUfDZrLKrf6evTXQ03cJ1W4kj0gxsF9Bopu+i2SjkBObXKnMA@mail.gmail.com>
Date: Mon, 21 Jul 2025 11:04:31 -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 Mon, Jul 21, 2025 at 1:23 AM Sidong Yang <sidong.yang@...iosa.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@...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.
>
> 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
> > >
> > >

Powered by blists - more mailing lists