Message-ID: <4742691b-0d36-4d9d-a629-95cd046f4f35@gmail.com>
Date: Sat, 25 Jan 2025 00:42:38 +0100
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Alice Ryhl <aliceryhl@...gle.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Arnd Bergmann <arnd@...db.de>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: miscdevice: change how f_ops vtable is constructed

On 17.01.25 3:22 PM, Alice Ryhl wrote:
> I was helping someone with writing a new Rust abstraction, and we were
> using the miscdevice abstraction as an example. While doing this, it
> became clear to me that the way I implemented the f_ops vtable is
> confusing to new Rust users, and that the approach used by the block
> abstractions is less confusing.
> 
> Thus, update the miscdevice abstractions to use the same approach as
> rust/kernel/block/mq/operations.rs.
> 
> Sorry about the large diff. This changes the indentation of a large
> amount of code.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---

Reviewed-by: Christian Schrefl <chrisi.schrefl@...il.com>

>  rust/kernel/miscdevice.rs | 295 ++++++++++++++++++++++------------------------
>  1 file changed, 141 insertions(+), 154 deletions(-)
> 
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index dfb363630c70..ad02434cbeaf 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -39,7 +39,7 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
>          let mut result: bindings::miscdevice = unsafe { MaybeUninit::zeroed().assume_init() };
>          result.minor = bindings::MISC_DYNAMIC_MINOR as _;
>          result.name = self.name.as_char_ptr();
> -        result.fops = create_vtable::<T>();
> +        result.fops = MiscdeviceVTable::<T>::build();
>          result
>      }
>  }
> @@ -164,171 +164,158 @@ fn show_fdinfo(
>      }
>  }
>  
> -const fn create_vtable<T: MiscDevice>() -> &'static bindings::file_operations {
> -    const fn maybe_fn<T: Copy>(check: bool, func: T) -> Option<T> {
> -        if check {
> -            Some(func)
> -        } else {
> -            None
> +/// A vtable for the file operations of a Rust miscdevice.
> +struct MiscdeviceVTable<T: MiscDevice>(PhantomData<T>);
> +
> +impl<T: MiscDevice> MiscdeviceVTable<T> {
> +    /// # Safety
> +    ///
> +    /// `file` and `inode` must be the file and inode for a file that is undergoing initialization.
> +    /// The file must be associated with a `MiscDeviceRegistration<T>`.
> +    unsafe extern "C" fn open(inode: *mut bindings::inode, raw_file: *mut bindings::file) -> c_int {
> +        // SAFETY: The pointers are valid and for a file being opened.
> +        let ret = unsafe { bindings::generic_file_open(inode, raw_file) };
> +        if ret != 0 {
> +            return ret;
>          }
> -    }
>  
> -    struct VtableHelper<T: MiscDevice> {
> -        _t: PhantomData<T>,
> -    }
> -    impl<T: MiscDevice> VtableHelper<T> {
> -        const VTABLE: bindings::file_operations = bindings::file_operations {
> -            open: Some(fops_open::<T>),
> -            release: Some(fops_release::<T>),
> -            unlocked_ioctl: maybe_fn(T::HAS_IOCTL, fops_ioctl::<T>),
> -            #[cfg(CONFIG_COMPAT)]
> -            compat_ioctl: if T::HAS_COMPAT_IOCTL {
> -                Some(fops_compat_ioctl::<T>)
> -            } else if T::HAS_IOCTL {
> -                Some(bindings::compat_ptr_ioctl)
> -            } else {
> -                None
> -            },
> -            show_fdinfo: maybe_fn(T::HAS_SHOW_FDINFO, fops_show_fdinfo::<T>),
> -            // SAFETY: All zeros is a valid value for `bindings::file_operations`.
> -            ..unsafe { MaybeUninit::zeroed().assume_init() }
> -        };
> -    }
> +        // SAFETY: The open call of a file can access the private data.
> +        let misc_ptr = unsafe { (*raw_file).private_data };
>  
> -    &VtableHelper::<T>::VTABLE
> -}
> +        // SAFETY: This is a miscdevice, so `misc_open()` set the private data to a pointer to the
> +        // associated `struct miscdevice` before calling into this method. Furthermore, `misc_open()`
> +        // ensures that the miscdevice can't be unregistered and freed during this call to `fops_open`.
> +        let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
>  
> -/// # Safety
> -///
> -/// `file` and `inode` must be the file and inode for a file that is undergoing initialization.
> -/// The file must be associated with a `MiscDeviceRegistration<T>`.
> -unsafe extern "C" fn fops_open<T: MiscDevice>(
> -    inode: *mut bindings::inode,
> -    raw_file: *mut bindings::file,
> -) -> c_int {
> -    // SAFETY: The pointers are valid and for a file being opened.
> -    let ret = unsafe { bindings::generic_file_open(inode, raw_file) };
> -    if ret != 0 {
> -        return ret;
> -    }
> +        // SAFETY:
> +        // * This underlying file is valid for (much longer than) the duration of `T::open`.
> +        // * There is no active fdget_pos region on the file on this thread.
> +        let file = unsafe { File::from_raw_file(raw_file) };
>  
> -    // SAFETY: The open call of a file can access the private data.
> -    let misc_ptr = unsafe { (*raw_file).private_data };
> -
> -    // SAFETY: This is a miscdevice, so `misc_open()` set the private data to a pointer to the
> -    // associated `struct miscdevice` before calling into this method. Furthermore, `misc_open()`
> -    // ensures that the miscdevice can't be unregistered and freed during this call to `fops_open`.
> -    let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
> +        let ptr = match T::open(file, misc) {
> +            Ok(ptr) => ptr,
> +            Err(err) => return err.to_errno(),
> +        };
>  
> -    // SAFETY:
> -    // * This underlying file is valid for (much longer than) the duration of `T::open`.
> -    // * There is no active fdget_pos region on the file on this thread.
> -    let file = unsafe { File::from_raw_file(raw_file) };
> +        // This overwrites the private data with the value specified by the user, changing the type of
> +        // this file's private data. All future accesses to the private data is performed by other
> +        // fops_* methods in this file, which all correctly cast the private data to the new type.
> +        //
> +        // SAFETY: The open call of a file can access the private data.
> +        unsafe { (*raw_file).private_data = ptr.into_foreign().cast_mut() };
>  
> -    let ptr = match T::open(file, misc) {
> -        Ok(ptr) => ptr,
> -        Err(err) => return err.to_errno(),
> -    };
> -
> -    // This overwrites the private data with the value specified by the user, changing the type of
> -    // this file's private data. All future accesses to the private data is performed by other
> -    // fops_* methods in this file, which all correctly cast the private data to the new type.
> -    //
> -    // SAFETY: The open call of a file can access the private data.
> -    unsafe { (*raw_file).private_data = ptr.into_foreign().cast_mut() };
> +        0
> +    }
>  
> -    0
> -}
> +    /// # Safety
> +    ///
> +    /// `file` and `inode` must be the file and inode for a file that is being released. The file must
> +    /// be associated with a `MiscDeviceRegistration<T>`.
> +    unsafe extern "C" fn release(_inode: *mut bindings::inode, file: *mut bindings::file) -> c_int {
> +        // SAFETY: The release call of a file owns the private data.
> +        let private = unsafe { (*file).private_data };
> +        // SAFETY: The release call of a file owns the private data.
> +        let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
> +
> +        // SAFETY:
> +        // * The file is valid for the duration of this call.
> +        // * There is no active fdget_pos region on the file on this thread.
> +        T::release(ptr, unsafe { File::from_raw_file(file) });
> +
> +        0
> +    }
>  
> -/// # Safety
> -///
> -/// `file` and `inode` must be the file and inode for a file that is being released. The file must
> -/// be associated with a `MiscDeviceRegistration<T>`.
> -unsafe extern "C" fn fops_release<T: MiscDevice>(
> -    _inode: *mut bindings::inode,
> -    file: *mut bindings::file,
> -) -> c_int {
> -    // SAFETY: The release call of a file owns the private data.
> -    let private = unsafe { (*file).private_data };
> -    // SAFETY: The release call of a file owns the private data.
> -    let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
> -
> -    // SAFETY:
> -    // * The file is valid for the duration of this call.
> -    // * There is no active fdget_pos region on the file on this thread.
> -    T::release(ptr, unsafe { File::from_raw_file(file) });
> -
> -    0
> -}
> +    /// # Safety
> +    ///
> +    /// `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`.
> +    unsafe extern "C" fn ioctl(file: *mut bindings::file, cmd: c_uint, arg: c_ulong) -> c_long {
> +        // SAFETY: The ioctl call of a file can access the private data.
> +        let private = unsafe { (*file).private_data };
> +        // SAFETY: Ioctl calls can borrow the private data of the file.
> +        let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
> +
> +        // SAFETY:
> +        // * The file is valid for the duration of this call.
> +        // * There is no active fdget_pos region on the file on this thread.
> +        let file = unsafe { File::from_raw_file(file) };
> +
> +        match T::ioctl(device, file, cmd, arg as usize) {
> +            Ok(ret) => ret as c_long,
> +            Err(err) => err.to_errno() as c_long,
> +        }
> +    }
>  
> -/// # Safety
> -///
> -/// `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`.
> -unsafe extern "C" fn fops_ioctl<T: MiscDevice>(
> -    file: *mut bindings::file,
> -    cmd: c_uint,
> -    arg: c_ulong,
> -) -> c_long {
> -    // SAFETY: The ioctl call of a file can access the private data.
> -    let private = unsafe { (*file).private_data };
> -    // SAFETY: Ioctl calls can borrow the private data of the file.
> -    let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
> -
> -    // SAFETY:
> -    // * The file is valid for the duration of this call.
> -    // * There is no active fdget_pos region on the file on this thread.
> -    let file = unsafe { File::from_raw_file(file) };
> -
> -    match T::ioctl(device, file, cmd, arg as usize) {
> -        Ok(ret) => ret as c_long,
> -        Err(err) => err.to_errno() as c_long,
> +    /// # Safety
> +    ///
> +    /// `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`.
> +    #[cfg(CONFIG_COMPAT)]
> +    unsafe extern "C" fn compat_ioctl(
> +        file: *mut bindings::file,
> +        cmd: c_uint,
> +        arg: c_ulong,
> +    ) -> c_long {
> +        // SAFETY: The compat ioctl call of a file can access the private data.
> +        let private = unsafe { (*file).private_data };
> +        // SAFETY: Ioctl calls can borrow the private data of the file.
> +        let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
> +
> +        // SAFETY:
> +        // * The file is valid for the duration of this call.
> +        // * There is no active fdget_pos region on the file on this thread.
> +        let file = unsafe { File::from_raw_file(file) };
> +
> +        match T::compat_ioctl(device, file, cmd, arg as usize) {
> +            Ok(ret) => ret as c_long,
> +            Err(err) => err.to_errno() as c_long,
> +        }
>      }
> -}
>  
> -/// # Safety
> -///
> -/// `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`.
> -#[cfg(CONFIG_COMPAT)]
> -unsafe extern "C" fn fops_compat_ioctl<T: MiscDevice>(
> -    file: *mut bindings::file,
> -    cmd: c_uint,
> -    arg: c_ulong,
> -) -> c_long {
> -    // SAFETY: The compat ioctl call of a file can access the private data.
> -    let private = unsafe { (*file).private_data };
> -    // SAFETY: Ioctl calls can borrow the private data of the file.
> -    let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
> -
> -    // SAFETY:
> -    // * The file is valid for the duration of this call.
> -    // * There is no active fdget_pos region on the file on this thread.
> -    let file = unsafe { File::from_raw_file(file) };
> -
> -    match T::compat_ioctl(device, file, cmd, arg as usize) {
> -        Ok(ret) => ret as c_long,
> -        Err(err) => err.to_errno() as c_long,
> +    /// # Safety
> +    ///
> +    /// - `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`.
> +    /// - `seq_file` must be a valid `struct seq_file` that we can write to.
> +    unsafe extern "C" fn show_fdinfo(seq_file: *mut bindings::seq_file, file: *mut bindings::file) {
> +        // SAFETY: The release call of a file owns the private data.
> +        let private = unsafe { (*file).private_data };
> +        // SAFETY: Ioctl calls can borrow the private data of the file.
> +        let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
> +        // SAFETY:
> +        // * The file is valid for the duration of this call.
> +        // * There is no active fdget_pos region on the file on this thread.
> +        let file = unsafe { File::from_raw_file(file) };
> +        // SAFETY: The caller ensures that the pointer is valid and exclusive for the duration in which
> +        // this method is called.
> +        let m = unsafe { SeqFile::from_raw(seq_file) };
> +
> +        T::show_fdinfo(device, m, file);
>      }
> -}
>  
> -/// # Safety
> -///
> -/// - `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`.
> -/// - `seq_file` must be a valid `struct seq_file` that we can write to.
> -unsafe extern "C" fn fops_show_fdinfo<T: MiscDevice>(
> -    seq_file: *mut bindings::seq_file,
> -    file: *mut bindings::file,
> -) {
> -    // SAFETY: The release call of a file owns the private data.
> -    let private = unsafe { (*file).private_data };
> -    // SAFETY: Ioctl calls can borrow the private data of the file.
> -    let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
> -    // SAFETY:
> -    // * The file is valid for the duration of this call.
> -    // * There is no active fdget_pos region on the file on this thread.
> -    let file = unsafe { File::from_raw_file(file) };
> -    // SAFETY: The caller ensures that the pointer is valid and exclusive for the duration in which
> -    // this method is called.
> -    let m = unsafe { SeqFile::from_raw(seq_file) };
> -
> -    T::show_fdinfo(device, m, file);
> +    const VTABLE: bindings::file_operations = bindings::file_operations {
> +        open: Some(Self::open),
> +        release: Some(Self::release),
> +        unlocked_ioctl: if T::HAS_IOCTL {
> +            Some(Self::ioctl)
> +        } else {
> +            None
> +        },
> +        #[cfg(CONFIG_COMPAT)]
> +        compat_ioctl: if T::HAS_COMPAT_IOCTL {
> +            Some(Self::compat_ioctl)
> +        } else if T::HAS_IOCTL {
> +            Some(Self::bindings::compat_ptr_ioctl)
> +        } else {
> +            None
> +        },
> +        show_fdinfo: if T::HAS_SHOW_FDINFO {
> +            Some(Self::show_fdinfo)
> +        } else {
> +            None
> +        },
> +        // SAFETY: All zeros is a valid value for `bindings::file_operations`.
> +        ..unsafe { MaybeUninit::zeroed().assume_init() }
> +    };
> +
> +    const fn build() -> &'static bindings::file_operations {
> +        &Self::VTABLE
> +    }
>  }
> 
> ---
> base-commit: 01b3cb620815fc3feb90ee117d9445a5b608a9f7
> change-id: 20250117-miscdevice-fops-change-260352fd8957
> 
> Best regards,

Powered by blists - more mailing lists