Message-Id: <BC40C40D-D835-4B5E-927C-A55939110114@collabora.com>
Date: Fri, 1 Aug 2025 11:04:36 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Sidong Yang <sidong.yang@...iosa.ai>
Cc: Caleb Sander Mateos <csander@...estorage.com>,
 Benno Lossin <lossin@...nel.org>,
 Miguel Ojeda <ojeda@...nel.org>,
 Arnd Bergmann <arnd@...db.de>,
 Jens Axboe <axboe@...nel.dk>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 io-uring@...r.kernel.org
Subject: Re: [RFC PATCH v2 3/4] rust: miscdevice: add uring_cmd() for
 MiscDevice trait

Hi Sidong,

> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@...iosa.ai> wrote:
> 
> This patch adds uring_cmd() function for MiscDevice trait and its
> callback implementation. It uses IoUringCmd that io_uring_cmd rust
> abstraction.

I can’t parse this.

> 
> Signed-off-by: Sidong Yang <sidong.yang@...iosa.ai>
> ---
> rust/kernel/miscdevice.rs | 41 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
> 
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index 288f40e79906..54be866ea7ff 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -14,6 +14,7 @@
>     error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
>     ffi::{c_int, c_long, c_uint, c_ulong},
>     fs::File,
> +    io_uring::IoUringCmd,
>     mm::virt::VmaNew,
>     prelude::*,
>     seq_file::SeqFile,
> @@ -175,6 +176,19 @@ fn show_fdinfo(
>     ) {
>         build_error!(VTABLE_DEFAULT_ERROR)
>     }
> +
> +    /// Handler for uring_cmd.
> +    ///
> +    /// This function is invoked when userspace process submits the uring_cmd op
> +    /// on io_uring submission queue. The `io_uring_cmd` would be used for get
> +    /// arguments cmd_op, sqe, cmd_data.

Please improve this. I don’t think that anyone reading this can really get
a good grasp on what this function does.

What does `issue_flags` do?

> +    fn uring_cmd(
> +        _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
> +        _io_uring_cmd: Pin<&mut IoUringCmd>,
> +        _issue_flags: u32,
> +    ) -> Result<i32> {
> +        build_error!(VTABLE_DEFAULT_ERROR)
> +    }
> }
> 
> /// A vtable for the file operations of a Rust miscdevice.
> @@ -332,6 +346,28 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
>         T::show_fdinfo(device, m, file);
>     }
> 
> +    /// # Safety
> +    ///
> +    /// `ioucmd` is not null and points to a valid `bindings::io_uring_cmd`.

Please rewrite this as “the caller must ensure that  `ioucmd` points to a
valid `bindings::io_uring_cmd`” or some variation thereof.

> +    unsafe extern "C" fn uring_cmd(
> +        ioucmd: *mut bindings::io_uring_cmd,
> +        issue_flags: ffi::c_uint,
> +    ) -> ffi::c_int {
> +        // SAFETY: The file is valid for the duration of this call.
> +        let ioucmd = unsafe { IoUringCmd::from_raw(ioucmd) };

What file?

Also, this is what you wrote for IoUringCmd::from_raw:

+
+ /// Constructs a new `IoUringCmd` from a raw `io_uring_cmd`
+ ///
+ /// # Safety
+ ///
+ /// The caller must guarantee that:
+ /// - The pointer `ptr` is not null and points to a valid `bindings::io_uring_cmd`.
+ /// - The memory pointed to by `ptr` remains valid for the duration of the returned reference's lifetime `'a`.
+ /// - The memory will not be moved or freed while the returned `Pin<&mut IoUringCmd>` is alive.
+ #[inline]
+ pub unsafe fn from_raw<'a>(ptr: *mut bindings::io_uring_cmd) -> Pin<&'a mut IoUringCmd> {

Here, you have to mention how the safety requirements above are fulfilled in this call site.

> +        let file = ioucmd.file();
> +
> +        // SAFETY: The file is valid for the duration of this call.

Same here.

> +        let private = unsafe { (*file.as_ptr()).private_data }.cast();

Perhaps this can be hidden away in an accessor?

> +        // SAFETY: uring_cmd calls can borrow the private data of the file.
> +        let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };

This is ForeignOwnable::borrow():

    /// Borrows a foreign-owned object immutably.
    ///
    /// This method provides a way to access a foreign-owned value from Rust immutably. It provides
    /// you with exactly the same abilities as an `&Self` when the value is Rust-owned.
    ///
    /// # Safety
    ///
    /// The provided pointer must have been returned by a previous call to [`into_foreign`], and if
    /// the pointer is ever passed to [`from_foreign`], then that call must happen after the end of
    /// the lifetime `'a`.
    ///
    /// [`into_foreign`]: Self::into_foreign
    /// [`from_foreign`]: Self::from_foreign
    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> Self::Borrowed<'a>;

You must say how the safety requirements above are fulfilled in this call site
as well. In particular, are you sure that this is true? i.e.:

> The provided pointer must have been returned by a previous call to
> [`into_foreign`],


> +
> +        match T::uring_cmd(device, ioucmd, issue_flags) {
> +            Ok(ret) => ret as ffi::c_int,
> +            Err(err) => err.to_errno() as ffi::c_int,

c_int is in the prelude. Also, please have a look at error::from_result().

> +        }
> +    }
> +
>     const VTABLE: bindings::file_operations = bindings::file_operations {
>         open: Some(Self::open),
>         release: Some(Self::release),
> @@ -354,6 +390,11 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
>         } else {
>             None
>         },
> +        uring_cmd: if T::HAS_URING_CMD {
> +            Some(Self::uring_cmd)
> +        } else {
> +            None
> +        },
>         // SAFETY: All zeros is a valid value for `bindings::file_operations`.
>         ..unsafe { MaybeUninit::zeroed().assume_init() }
>     };
> -- 
> 2.43.0
> 
> 

— Daniel

Powered by blists - more mailing lists