[PATCH 0/3] Miscdevices in Rust

[PATCH 0/3] Miscdevices in Rust

[Alice Ryhl]

A misc device is generally the best place to start with your first Rust
driver, so having abstractions for miscdevice in Rust will be important
for our ability to teach Rust to kernel developers.

I intend to add a sample driver using these abstractions, and I also
intend to use it in Rust Binder to handle the case where binderfs is
turned off.

I know that the patchset is still a bit rough. It could use some work on
the file position aspect. But I'm sending this out now to get feedback
on the overall approach.

This patchset depends on files [1] and vma [2].

Link: https://lore.kernel.org/all/20240915-alice-file-v10-0-88484f7a3dcf@google.com/ [1]
Link: https://lore.kernel.org/all/20240806-vma-v5-1-04018f05de2b@google.com/ [2]
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Alice Ryhl (3):
      rust: types: add Opaque::try_ffi_init
      rust: file: add f_pos and set_f_pos
      rust: miscdevice: add abstraction for defining miscdevices

 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/fs/file.rs          |  20 ++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/miscdevice.rs       | 401 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/types.rs            |  16 ++
 5 files changed, 439 insertions(+)
---
base-commit: a6266fcab443f4b6ae31016bd6c3872f8200d5e1
change-id: 20240926-b4-miscdevice-29a0fd8438b1

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

[PATCH 1/3] rust: types: add Opaque::try_ffi_init

[Alice Ryhl]

This will be used by the miscdevice abstractions, as the C function
`misc_register` is fallible.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/types.rs | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 3238ffaab031..dd9c606c515c 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -299,6 +299,22 @@ pub fn ffi_init(init_func: impl FnOnce(*mut T)) -> impl PinInit<Self> {
         }
     }
 
+    /// Creates a fallible pin-initializer from the given initializer closure.
+    ///
+    /// The returned initializer calls the given closure with the pointer to the inner `T` of this
+    /// `Opaque`. Since this memory is uninitialized, the closure is not allowed to read from it.
+    ///
+    /// This function is safe, because the `T` inside of an `Opaque` is allowed to be
+    /// uninitialized. Additionally, access to the inner `T` requires `unsafe`, so the caller needs
+    /// to verify at that point that the inner value is valid.
+    pub fn try_ffi_init<E>(
+        init_func: impl FnOnce(*mut T) -> Result<(), E>,
+    ) -> impl PinInit<Self, E> {
+        // SAFETY: We contain a `MaybeUninit`, so it is OK for the `init_func` to not fully
+        // initialize the `T`.
+        unsafe { init::pin_init_from_closure::<_, E>(move |slot| init_func(Self::raw_get(slot))) }
+    }
+
     /// Returns a raw pointer to the opaque data.
     pub const fn get(&self) -> *mut T {
         UnsafeCell::get(&self.value).cast::<T>()

-- 
2.46.0.792.g87dc391469-goog

[PATCH 2/3] rust: file: add f_pos and set_f_pos

[Alice Ryhl]

Add accessors for the file position. Most of the time, you should not
use these methods directly, and you should instead use a guard for the
file position to prove that you hold the fpos lock. However, under
limited circumstances, files are allowed to choose a different locking
strategy for their file position. These accessors can be used to handle
that case.

For now, these accessors are the only way to access the file position
within the llseek and read_iter callbacks.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/fs/file.rs | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs
index e03dbe14d62a..c896a3b1d182 100644
--- a/rust/kernel/fs/file.rs
+++ b/rust/kernel/fs/file.rs
@@ -333,6 +333,26 @@ pub fn flags(&self) -> u32 {
         // FIXME(read_once): Replace with `read_once` when available on the Rust side.
         unsafe { core::ptr::addr_of!((*self.as_ptr()).f_flags).read_volatile() }
     }
+
+    /// Read the file position.
+    ///
+    /// # Safety
+    ///
+    /// You must hold the fpos lock or otherwise ensure that no data race will happen.
+    #[inline]
+    pub unsafe fn f_pos(&self) -> i64 {
+        unsafe { (*self.as_ptr()).f_pos }
+    }
+
+    /// Set the file position.
+    ///
+    /// # Safety
+    ///
+    /// You must hold the fpos lock or otherwise ensure that no data race will happen.
+    #[inline]
+    pub unsafe fn set_f_pos(&self, value: i64) {
+        unsafe { (*self.as_ptr()).f_pos = value };
+    }
 }
 
 impl File {

-- 
2.46.0.792.g87dc391469-goog

[PATCH 3/3] rust: miscdevice: add abstraction for defining miscdevices

[Alice Ryhl]

Provide a `MiscDevice` trait that lets you specify the file operations
that you wish to provide for your misc device. Most operations are
optional.

In the future, the file operations in the `MiscDevice` can be moved to a
general `FileOperations` trait so that it is useful for other file
systems too.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/lib.rs              |   1 +
 rust/kernel/miscdevice.rs       | 401 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 403 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ca13659ded4c..ec2e28ef6568 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -18,6 +18,7 @@
 #include <linux/fs.h>
 #include <linux/jiffies.h>
 #include <linux/mdio.h>
+#include <linux/miscdevice.h>
 #include <linux/phy.h>
 #include <linux/pid_namespace.h>
 #include <linux/poll.h>
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 819126fc2d89..bf97817842c9 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -41,6 +41,7 @@
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
 pub mod list;
+pub mod miscdevice;
 pub mod mm;
 #[cfg(CONFIG_NET)]
 pub mod net;
diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
new file mode 100644
index 000000000000..6c4e33d77c58
--- /dev/null
+++ b/rust/kernel/miscdevice.rs
@@ -0,0 +1,401 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Miscdevice support.
+//!
+//! C headers: [`include/linux/miscdevice.h`](srctree/include/linux/miscdevice.h).
+//!
+//! Reference: <https://www.kernel.org/doc/html/latest/driver-api/misc_devices.html>
+
+use crate::{
+    bindings,
+    error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
+    fs::{File, LocalFile},
+    mm::virt::VmArea,
+    prelude::*,
+    str::CStr,
+    types::{ForeignOwnable, Opaque},
+};
+use core::{
+    ffi::{c_int, c_long, c_uint, c_ulong},
+    marker::PhantomData,
+    mem::MaybeUninit,
+    pin::Pin,
+    ptr::NonNull,
+};
+
+/// The kernel `loff_t` type.
+#[allow(non_camel_case_types)]
+pub type loff_t = bindings::loff_t;
+
+/// Options for creating a misc device.
+#[derive(Copy, Clone)]
+pub struct MiscDeviceOptions {
+    /// The name of the miscdevice.
+    pub name: &'static CStr,
+}
+
+impl MiscDeviceOptions {
+    /// Create a raw `struct miscdev` ready for registration.
+    pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
+        // SAFETY: All zeros is valid for this C type.
+        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
+    }
+}
+
+/// A registration of a miscdevice.
+///
+/// # Invariants
+///
+/// `inner` is a registered misc device.
+#[repr(transparent)]
+#[pin_data(PinnedDrop)]
+pub struct MiscDeviceRegistration<T> {
+    #[pin]
+    inner: Opaque<bindings::miscdevice>,
+    _t: PhantomData<T>,
+}
+
+unsafe impl<T> Send for MiscDeviceRegistration<T> {}
+unsafe impl<T> Sync for MiscDeviceRegistration<T> {}
+
+impl<T: MiscDevice> MiscDeviceRegistration<T> {
+    /// Register a misc device.
+    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
+        try_pin_init!(Self {
+            inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
+                // SAFETY: The initializer can write to the provided `slot`.
+                unsafe { slot.write(opts.into_raw::<T>()) };
+
+                // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
+                // get unregistered before `slot` is deallocated because the memory is pinned and
+                // the destructor of this type deallocates the memory.
+                // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
+                // misc device.
+                to_result(unsafe { bindings::misc_register(slot) })
+            }),
+            _t: PhantomData,
+        })
+    }
+
+    /// Returns the private data associated with the provided file.
+    ///
+    /// Returns `None` if the file is not associated with this misc device.
+    pub fn try_get_private_data<'a>(
+        &self,
+        file: &'a LocalFile,
+    ) -> Option<<T::Ptr as ForeignOwnable>::Borrowed<'a>> {
+        // SAFETY: `fops` of a miscdevice is immutable after initialization.
+        let fops_this = unsafe { (*self.as_raw()).fops };
+        // SAFETY: `f_op` of a file is immutable after initialization.
+        let fops_file = unsafe { (*file.as_ptr()).f_op };
+
+        if core::ptr::eq(fops_this, fops_file) {
+            // SAFETY: We know that `file` is associated with a `MiscDeviceRegistration<T>`, so
+            // `private_data` is immutable.
+            let private_data = unsafe { (*file.as_ptr()).private_data };
+            // SAFETY:
+            // * The fops match, so the file's private date has the right type.
+            // * The returned borrow cannot outlive the file.
+            Some(unsafe { <T::Ptr as ForeignOwnable>::borrow(private_data) })
+        } else {
+            None
+        }
+    }
+
+    /// Returns a raw pointer to the misc device.
+    pub fn as_raw(&self) -> *mut bindings::miscdevice {
+        self.inner.get()
+    }
+}
+
+#[pinned_drop]
+impl<T> PinnedDrop for MiscDeviceRegistration<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY: We know that the device is registered by the type invariants.
+        unsafe { bindings::misc_deregister(self.inner.get()) };
+    }
+}
+
+/// Trait implemented by the private data of an open misc device.
+#[vtable]
+pub trait MiscDevice {
+    /// What kind of pointer should `Self` be wrapped in.
+    type Ptr: ForeignOwnable + Send + Sync;
+
+    /// Called when the misc device is opened.
+    ///
+    /// The returned pointer will be stored as the private data for the file.
+    fn open(_file: &File) -> Result<Self::Ptr>;
+
+    /// Called when the misc device is released.
+    fn release(device: Self::Ptr, _file: &File) {
+        drop(device);
+    }
+
+    /// Handle for mmap.
+    fn mmap(
+        _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
+        _file: &File,
+        _vma: Pin<&mut VmArea>,
+    ) -> Result<()> {
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Seeks this miscdevice.
+    fn llseek(
+        _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
+        _file: &LocalFile,
+        _offset: loff_t,
+        _whence: c_int,
+    ) -> Result<loff_t> {
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Read from this miscdevice.
+    fn read_iter(_kiocb: Kiocb<'_, Self::Ptr>, _iov: &mut IovIter) -> Result<usize> {
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Handler for ioctls
+    ///
+    /// The `cmd` argument is usually manipulated using the utilties in [`kernel::ioctl`].
+    ///
+    /// [`kernel::ioctl`]: mod@crate::ioctl
+    fn ioctl(
+        _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
+        _file: &File,
+        _cmd: u32,
+        _arg: usize,
+    ) -> Result<c_long> {
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Handler for ioctls
+    ///
+    /// Used for 32-bit userspace on 64-bit platforms.
+    #[cfg(CONFIG_COMPAT)]
+    fn compat_ioctl(
+        device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
+        file: &File,
+        cmd: u32,
+        arg: usize,
+    ) -> Result<c_long> {
+        Self::ioctl(device, file, cmd, arg)
+    }
+}
+
+/// Wrapper for the kernel's `struct kiocb`.
+///
+/// The type `T` represents the private data of the file.
+pub struct Kiocb<'a, T> {
+    inner: NonNull<bindings::kiocb>,
+    _phantom: PhantomData<&'a T>,
+}
+
+impl<'a, T: ForeignOwnable> Kiocb<'a, T> {
+    /// Get the private data in this kiocb.
+    pub fn private_data(&self) -> <T as ForeignOwnable>::Borrowed<'a> {
+        // SAFETY: The `kiocb` lets us access the private data.
+        let private = unsafe { (*(*self.inner.as_ptr()).ki_filp).private_data };
+        // SAFETY: The kiocb has shared access to the private data.
+        unsafe { <T as ForeignOwnable>::borrow(private) }
+    }
+
+    /// Gets the current value of `ki_pos`.
+    pub fn ki_pos(&self) -> loff_t {
+        // SAFETY: The `kiocb` can access `ki_pos`.
+        unsafe { (*self.inner.as_ptr()).ki_pos }
+    }
+
+    /// Gets a mutable reference to the `ki_pos` field.
+    pub fn ki_pos_mut(&mut self) -> &mut loff_t {
+        // SAFETY: The `kiocb` can access `ki_pos`.
+        unsafe { &mut (*self.inner.as_ptr()).ki_pos }
+    }
+}
+
+/// Wrapper for the kernel's `struct iov_iter`.
+pub struct IovIter {
+    inner: Opaque<bindings::iov_iter>,
+}
+
+impl IovIter {
+    /// Gets a raw pointer to the contents.
+    pub fn as_raw(&self) -> *mut bindings::iov_iter {
+        self.inner.get()
+    }
+}
+
+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
+        }
+    }
+
+    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>),
+            mmap: maybe_fn(T::HAS_MMAP, fops_mmap::<T>),
+            llseek: maybe_fn(T::HAS_LLSEEK, fops_llseek::<T>),
+            read_iter: maybe_fn(T::HAS_READ_ITER, fops_read_iter::<T>),
+            unlocked_ioctl: maybe_fn(T::HAS_IOCTL, fops_ioctl::<T>),
+            #[cfg(CONFIG_COMPAT)]
+            compat_ioctl: maybe_fn(T::HAS_IOCTL || T::HAS_COMPAT_IOCTL, fops_compat_ioctl::<T>),
+            ..unsafe { MaybeUninit::zeroed().assume_init() }
+        };
+    }
+
+    &VtableHelper::<T>::VTABLE
+}
+
+unsafe extern "C" fn fops_open<T: MiscDevice>(
+    inode: *mut bindings::inode,
+    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, file) };
+    if ret != 0 {
+        return ret;
+    }
+
+    // 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 ptr = match T::open(unsafe { File::from_raw_file(file) }) {
+        Ok(ptr) => ptr,
+        Err(err) => return err.to_errno(),
+    };
+
+    // SAFETY: The open call of a file owns the private data.
+    unsafe { (*file).private_data = ptr.into_foreign().cast_mut() };
+
+    0
+}
+
+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: We are taking ownership of the private data, so we can drop it.
+    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
+}
+
+unsafe extern "C" fn fops_mmap<T: MiscDevice>(
+    file: *mut bindings::file,
+    vma: *mut bindings::vm_area_struct,
+) -> c_int {
+    // 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 vma is valid.
+    let area = unsafe { kernel::mm::virt::VmArea::from_raw_mut(vma) };
+
+    match T::mmap(device, file, area) {
+        Ok(()) => 0,
+        Err(err) => err.to_errno() as c_int,
+    }
+}
+
+unsafe extern "C" fn fops_llseek<T: MiscDevice>(
+    file: *mut bindings::file,
+    offset: loff_t,
+    whence: c_int,
+) -> loff_t {
+    // 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.
+    // * We are inside an fdget_pos region, so there cannot be any active fdget_pos regions on
+    //   other threads.
+    let file = unsafe { LocalFile::from_raw_file(file) };
+
+    match T::llseek(device, file, offset, whence) {
+        Ok(res) => res as loff_t,
+        Err(err) => err.to_errno() as loff_t,
+    }
+}
+
+unsafe extern "C" fn fops_read_iter<T: MiscDevice>(
+    kiocb: *mut bindings::kiocb,
+    iter: *mut bindings::iov_iter,
+) -> isize {
+    let kiocb = Kiocb {
+        inner: unsafe { NonNull::new_unchecked(kiocb) },
+        _phantom: PhantomData,
+    };
+    let iov = unsafe { &mut *iter.cast::<IovIter>() };
+
+    match T::read_iter(kiocb, iov) {
+        Ok(res) => res as isize,
+        Err(err) => err.to_errno() as isize,
+    }
+}
+
+unsafe extern "C" fn fops_ioctl<T: MiscDevice>(
+    file: *mut bindings::file,
+    cmd: c_uint,
+    arg: c_ulong,
+) -> c_long {
+    // 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) };
+
+    match T::ioctl(device, file, cmd as u32, arg as usize) {
+        Ok(ret) => ret,
+        Err(err) => err.to_errno() as c_long,
+    }
+}
+
+#[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 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) };
+
+    match T::compat_ioctl(device, file, cmd as u32, arg as usize) {
+        Ok(ret) => ret,
+        Err(err) => err.to_errno() as c_long,
+    }
+}

-- 
2.46.0.792.g87dc391469-goog

Re: [PATCH 0/3] Miscdevices in Rust

[Greg Kroah-Hartman]

On Thu, Sep 26, 2024 at 02:58:54PM +0000, Alice Ryhl wrote:
> A misc device is generally the best place to start with your first Rust
> driver, so having abstractions for miscdevice in Rust will be important
> for our ability to teach Rust to kernel developers.
> 
> I intend to add a sample driver using these abstractions, and I also
> intend to use it in Rust Binder to handle the case where binderfs is
> turned off.
> 
> I know that the patchset is still a bit rough. It could use some work on
> the file position aspect. But I'm sending this out now to get feedback
> on the overall approach.

Very cool!

> This patchset depends on files [1] and vma [2].
>
> Link: https://lore.kernel.org/all/20240915-alice-file-v10-0-88484f7a3dcf@google.com/ [1]
> Link: https://lore.kernel.org/all/20240806-vma-v5-1-04018f05de2b@google.com/ [2]
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Does it really need all of those dependencies?  I know your development
stack is deep here, but maybe I can unwind a bit of the file stuff to
get this in for the next merge window (6.13-rc1) if those two aren't
going to be planned for there.

I'll look into this some more next week, thanks!

greg k-h

Re: [PATCH 0/3] Miscdevices in Rust

[Miguel Ojeda]

On Thu, Sep 26, 2024 at 5:05 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> stack is deep here, but maybe I can unwind a bit of the file stuff to
> get this in for the next merge window (6.13-rc1) if those two aren't
> going to be planned for there.

I think Christian wanted to merge the file abstractions in 6.13, he
has them here:

    https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.rust.file

Cheers,
Miguel

Re: [PATCH 0/3] Miscdevices in Rust

[Alice Ryhl]

On Thu, Sep 26, 2024 at 5:05 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Sep 26, 2024 at 02:58:54PM +0000, Alice Ryhl wrote:
> > A misc device is generally the best place to start with your first Rust
> > driver, so having abstractions for miscdevice in Rust will be important
> > for our ability to teach Rust to kernel developers.
> >
> > I intend to add a sample driver using these abstractions, and I also
> > intend to use it in Rust Binder to handle the case where binderfs is
> > turned off.
> >
> > I know that the patchset is still a bit rough. It could use some work on
> > the file position aspect. But I'm sending this out now to get feedback
> > on the overall approach.
>
> Very cool!
>
> > This patchset depends on files [1] and vma [2].
> >
> > Link: https://lore.kernel.org/all/20240915-alice-file-v10-0-88484f7a3dcf@google.com/ [1]
> > Link: https://lore.kernel.org/all/20240806-vma-v5-1-04018f05de2b@google.com/ [2]
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>
> Does it really need all of those dependencies?  I know your development
> stack is deep here, but maybe I can unwind a bit of the file stuff to
> get this in for the next merge window (6.13-rc1) if those two aren't
> going to be planned for there.

Ah, maybe not. The dependency on files is necessary to allow the file
to look at its own fields, e.g. whether O_NONBLOCK is set or what the
file position is. But we can take that out for now and add it once
both miscdevice and file have landed. I'm hoping that file will land
for 6.13, but untangling them allows both to land in 6.13.

As for vma, it's needed for mmap, but if I take out the ability to
define an mmap operation, I don't need it. We can always add back mmap
once both miscdevice and vma have landed.

Thank you for the suggestion on untangling the dependencies.

> I'll look into this some more next week, thanks!

Thanks!

Alice

Re: [PATCH 0/3] Miscdevices in Rust

[Greg Kroah-Hartman]

On Thu, Sep 26, 2024 at 05:20:15PM +0200, Alice Ryhl wrote:
> On Thu, Sep 26, 2024 at 5:05 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Sep 26, 2024 at 02:58:54PM +0000, Alice Ryhl wrote:
> > > A misc device is generally the best place to start with your first Rust
> > > driver, so having abstractions for miscdevice in Rust will be important
> > > for our ability to teach Rust to kernel developers.
> > >
> > > I intend to add a sample driver using these abstractions, and I also
> > > intend to use it in Rust Binder to handle the case where binderfs is
> > > turned off.
> > >
> > > I know that the patchset is still a bit rough. It could use some work on
> > > the file position aspect. But I'm sending this out now to get feedback
> > > on the overall approach.
> >
> > Very cool!
> >
> > > This patchset depends on files [1] and vma [2].
> > >
> > > Link: https://lore.kernel.org/all/20240915-alice-file-v10-0-88484f7a3dcf@google.com/ [1]
> > > Link: https://lore.kernel.org/all/20240806-vma-v5-1-04018f05de2b@google.com/ [2]
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> >
> > Does it really need all of those dependencies?  I know your development
> > stack is deep here, but maybe I can unwind a bit of the file stuff to
> > get this in for the next merge window (6.13-rc1) if those two aren't
> > going to be planned for there.
> 
> Ah, maybe not. The dependency on files is necessary to allow the file
> to look at its own fields, e.g. whether O_NONBLOCK is set or what the
> file position is. But we can take that out for now and add it once
> both miscdevice and file have landed. I'm hoping that file will land
> for 6.13, but untangling them allows both to land in 6.13.
> 
> As for vma, it's needed for mmap, but if I take out the ability to
> define an mmap operation, I don't need it. We can always add back mmap
> once both miscdevice and vma have landed.

Yes, let's drop the mmap interface for now, and probably the seek stuff
too as most "normal" misc devices do not mess with them at all.

If that makes the dependencies simpler, that would be great.

thanks,

greg k-h

Re: [PATCH 0/3] Miscdevices in Rust

[Benno Lossin]

On 26.09.24 16:58, Alice Ryhl wrote:
> A misc device is generally the best place to start with your first Rust
> driver, so having abstractions for miscdevice in Rust will be important
> for our ability to teach Rust to kernel developers.

Sounds good!

> I intend to add a sample driver using these abstractions, and I also
> intend to use it in Rust Binder to handle the case where binderfs is
> turned off.
> 
> I know that the patchset is still a bit rough. It could use some work on
> the file position aspect. But I'm sending this out now to get feedback
> on the overall approach.
> 
> This patchset depends on files [1] and vma [2].
> 
> Link: https://lore.kernel.org/all/20240915-alice-file-v10-0-88484f7a3dcf@google.com/ [1]
> Link: https://lore.kernel.org/all/20240806-vma-v5-1-04018f05de2b@google.com/ [2]
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> Alice Ryhl (3):
>       rust: types: add Opaque::try_ffi_init
>       rust: file: add f_pos and set_f_pos
>       rust: miscdevice: add abstraction for defining miscdevices

I recall that we had a sample miscdev driver in the old rust branch. Can
you include that in this series, or is there still some stuff missing? I
think it would be really useful for people that want to implement such a
driver to have something to look at.

---
Cheers,
Benno

Re: [PATCH 2/3] rust: file: add f_pos and set_f_pos

[Al Viro]

On Thu, Sep 26, 2024 at 02:58:56PM +0000, Alice Ryhl wrote:
> Add accessors for the file position. Most of the time, you should not
> use these methods directly, and you should instead use a guard for the
> file position to prove that you hold the fpos lock. However, under
> limited circumstances, files are allowed to choose a different locking
> strategy for their file position. These accessors can be used to handle
> that case.
> 
> For now, these accessors are the only way to access the file position
> within the llseek and read_iter callbacks.

You really should not do that within ->read_iter().  If your method
does that, it has the wrong signature.

If nothing else, it should be usable for preadv(2), so what file position
are you talking about?

Re: [PATCH 2/3] rust: file: add f_pos and set_f_pos

[Al Viro]

On Thu, Sep 26, 2024 at 11:08:21PM +0100, Al Viro wrote:
> On Thu, Sep 26, 2024 at 02:58:56PM +0000, Alice Ryhl wrote:
> > Add accessors for the file position. Most of the time, you should not
> > use these methods directly, and you should instead use a guard for the
> > file position to prove that you hold the fpos lock. However, under
> > limited circumstances, files are allowed to choose a different locking
> > strategy for their file position. These accessors can be used to handle
> > that case.
> > 
> > For now, these accessors are the only way to access the file position
> > within the llseek and read_iter callbacks.
> 
> You really should not do that within ->read_iter().  If your method
> does that, it has the wrong signature.
> 
> If nothing else, it should be usable for preadv(2), so what file position
> are you talking about?

To elaborate: ->llseek() is the only method that has any business accessing
->f_pos (and that - possibly not forever).  Note, BTW, that most of the
time ->llseek() should be using one of the safe instances from fs/libfs.c
or helpers from the same place; direct ->f_pos access in drivers is
basically for things like
static loff_t cfam_llseek(struct file *file, loff_t offset, int whence)
{
        switch (whence) {
	case SEEK_CUR:
		break;
	case SEEK_SET:
		file->f_pos = offset;
		break;
	default:
		return -EINVAL;
	}

	return offset;
}
which is... really special.  Translation: lseek(fd, n, SEEK_CUR) - return n
and do nothing.  lseek(fd, n, SEEK_SET) - usual semantics.  Anything else
- fail with EINVAL.  The mind-boggling part is SEEK_CUR, but that's
userland ABI of that particular driver; if the authors can be convinced that
we don't need to preserve that wart, it can be replaced with use of
no_seek_end_llseek.  If their very special userland relies upon it...
not much we can do.

Anything else outside of core VFS should not touch the damn thing, unless
they have a very good reason and are willing to explain what makes them
special.

From quick grep through the tree, we seem to have grown a bunch of bogosities
in vfio (including one in samples, presumably responsible for that infestation),
there's a few strange ioctls that reset it to 0 or do other unnatural things
(hell, VFAT has readdir() variant called that way), there are _really_ shitty
cases in HFS, HFS+ and HPFS, where things like unlink() while somebody has the
parent directory open will modify the current position(s), and then there's
whatever ksmbd is playing at.

We really should not expose ->f_pos - that can't be done on the C side (yet),
but let's not spread that idiocy.

Re: [PATCH 2/3] rust: file: add f_pos and set_f_pos

[Al Viro]

On Thu, Sep 26, 2024 at 11:47:33PM +0100, Al Viro wrote:
> time ->llseek() should be using one of the safe instances from fs/libfs.c

d'oh... s/libfs.c/read_write.c/ - sorry.

Re: [PATCH 0/3] Miscdevices in Rust

[Greg Kroah-Hartman]

On Thu, Sep 26, 2024 at 06:58:10PM +0000, Benno Lossin wrote:
> On 26.09.24 16:58, Alice Ryhl wrote:
> > A misc device is generally the best place to start with your first Rust
> > driver, so having abstractions for miscdevice in Rust will be important
> > for our ability to teach Rust to kernel developers.
> 
> Sounds good!
> 
> > I intend to add a sample driver using these abstractions, and I also
> > intend to use it in Rust Binder to handle the case where binderfs is
> > turned off.
> > 
> > I know that the patchset is still a bit rough. It could use some work on
> > the file position aspect. But I'm sending this out now to get feedback
> > on the overall approach.
> > 
> > This patchset depends on files [1] and vma [2].
> > 
> > Link: https://lore.kernel.org/all/20240915-alice-file-v10-0-88484f7a3dcf@google.com/ [1]
> > Link: https://lore.kernel.org/all/20240806-vma-v5-1-04018f05de2b@google.com/ [2]
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> > Alice Ryhl (3):
> >       rust: types: add Opaque::try_ffi_init
> >       rust: file: add f_pos and set_f_pos
> >       rust: miscdevice: add abstraction for defining miscdevices
> 
> I recall that we had a sample miscdev driver in the old rust branch. Can
> you include that in this series, or is there still some stuff missing? I
> think it would be really useful for people that want to implement such a
> driver to have something to look at.

I agree, I'll dig that up after -rc1 is out and add it to this series.

thanks,

greg k-h

Re: [PATCH 2/3] rust: file: add f_pos and set_f_pos

[Alice Ryhl]

On Fri, Sep 27, 2024 at 12:08 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Sep 26, 2024 at 02:58:56PM +0000, Alice Ryhl wrote:
> > Add accessors for the file position. Most of the time, you should not
> > use these methods directly, and you should instead use a guard for the
> > file position to prove that you hold the fpos lock. However, under
> > limited circumstances, files are allowed to choose a different locking
> > strategy for their file position. These accessors can be used to handle
> > that case.
> >
> > For now, these accessors are the only way to access the file position
> > within the llseek and read_iter callbacks.
>
> You really should not do that within ->read_iter().  If your method
> does that, it has the wrong signature.
>
> If nothing else, it should be usable for preadv(2), so what file position
> are you talking about?

Sorry, you are right. In read_iter we can access the file position via
the kiocb. It only makes sense for seek.

Alice

Re: [PATCH 2/3] rust: file: add f_pos and set_f_pos

[Alice Ryhl]

On Fri, Sep 27, 2024 at 12:47 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Sep 26, 2024 at 11:08:21PM +0100, Al Viro wrote:
> > On Thu, Sep 26, 2024 at 02:58:56PM +0000, Alice Ryhl wrote:
> > > Add accessors for the file position. Most of the time, you should not
> > > use these methods directly, and you should instead use a guard for the
> > > file position to prove that you hold the fpos lock. However, under
> > > limited circumstances, files are allowed to choose a different locking
> > > strategy for their file position. These accessors can be used to handle
> > > that case.
> > >
> > > For now, these accessors are the only way to access the file position
> > > within the llseek and read_iter callbacks.
> >
> > You really should not do that within ->read_iter().  If your method
> > does that, it has the wrong signature.
> >
> > If nothing else, it should be usable for preadv(2), so what file position
> > are you talking about?
>
> To elaborate: ->llseek() is the only method that has any business accessing
> ->f_pos (and that - possibly not forever).  Note, BTW, that most of the
> time ->llseek() should be using one of the safe instances from fs/libfs.c
> or helpers from the same place; direct ->f_pos access in drivers is
> basically for things like
> static loff_t cfam_llseek(struct file *file, loff_t offset, int whence)
> {
>         switch (whence) {
>         case SEEK_CUR:
>                 break;
>         case SEEK_SET:
>                 file->f_pos = offset;
>                 break;
>         default:
>                 return -EINVAL;
>         }
>
>         return offset;
> }
> which is... really special.  Translation: lseek(fd, n, SEEK_CUR) - return n
> and do nothing.  lseek(fd, n, SEEK_SET) - usual semantics.  Anything else
> - fail with EINVAL.  The mind-boggling part is SEEK_CUR, but that's
> userland ABI of that particular driver; if the authors can be convinced that
> we don't need to preserve that wart, it can be replaced with use of
> no_seek_end_llseek.  If their very special userland relies upon it...
> not much we can do.
>
> Anything else outside of core VFS should not touch the damn thing, unless
> they have a very good reason and are willing to explain what makes them
> special.
>
> From quick grep through the tree, we seem to have grown a bunch of bogosities
> in vfio (including one in samples, presumably responsible for that infestation),
> there's a few strange ioctls that reset it to 0 or do other unnatural things
> (hell, VFAT has readdir() variant called that way), there are _really_ shitty
> cases in HFS, HFS+ and HPFS, where things like unlink() while somebody has the
> parent directory open will modify the current position(s), and then there's
> whatever ksmbd is playing at.
>
> We really should not expose ->f_pos - that can't be done on the C side (yet),
> but let's not spread that idiocy.

Okay, interesting. I did not know about all of these llseek helpers.
I'm definitely happy to make the Rust API force users to do the right
thing if we can.

It sounds like we basically have a few different seeking behaviors
that the driver can choose between, and we want to force the user to
use one of them?

Alice

Re: [PATCH 2/3] rust: file: add f_pos and set_f_pos

[Christian Brauner]

On Thu, Sep 26, 2024 at 02:58:56PM GMT, Alice Ryhl wrote:
> Add accessors for the file position. Most of the time, you should not
> use these methods directly, and you should instead use a guard for the
> file position to prove that you hold the fpos lock. However, under
> limited circumstances, files are allowed to choose a different locking
> strategy for their file position. These accessors can be used to handle
> that case.
> 
> For now, these accessors are the only way to access the file position
> within the llseek and read_iter callbacks.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/fs/file.rs | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs
> index e03dbe14d62a..c896a3b1d182 100644
> --- a/rust/kernel/fs/file.rs
> +++ b/rust/kernel/fs/file.rs
> @@ -333,6 +333,26 @@ pub fn flags(&self) -> u32 {
>          // FIXME(read_once): Replace with `read_once` when available on the Rust side.
>          unsafe { core::ptr::addr_of!((*self.as_ptr()).f_flags).read_volatile() }
>      }
> +
> +    /// Read the file position.
> +    ///
> +    /// # Safety
> +    ///
> +    /// You must hold the fpos lock or otherwise ensure that no data race will happen.
> +    #[inline]
> +    pub unsafe fn f_pos(&self) -> i64 {
> +        unsafe { (*self.as_ptr()).f_pos }
> +    }
> +
> +    /// Set the file position.
> +    ///
> +    /// # Safety
> +    ///
> +    /// You must hold the fpos lock or otherwise ensure that no data race will happen.
> +    #[inline]
> +    pub unsafe fn set_f_pos(&self, value: i64) {
> +        unsafe { (*self.as_ptr()).f_pos = value };
> +    }

I don't think we want to expose f_pos with its weird locking rule
through rust wrappers. Ideally, it's completely opaque to the callers
and not accessed directly at all.

Re: [PATCH 1/3] rust: types: add Opaque::try_ffi_init

[Fiona Behrens]

On 26 Sep 2024, at 16:58, Alice Ryhl wrote:

> This will be used by the miscdevice abstractions, as the C function
> `misc_register` is fallible.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---

Reviewed-by: Fiona Behrens <me@kloenk.dev>


>  rust/kernel/types.rs | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 3238ffaab031..dd9c606c515c 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -299,6 +299,22 @@ pub fn ffi_init(init_func: impl FnOnce(*mut T)) -> impl PinInit<Self> {
>          }
>      }
>
> +    /// Creates a fallible pin-initializer from the given initializer closure.
> +    ///
> +    /// The returned initializer calls the given closure with the pointer to the inner `T` of this
> +    /// `Opaque`. Since this memory is uninitialized, the closure is not allowed to read from it.
> +    ///
> +    /// This function is safe, because the `T` inside of an `Opaque` is allowed to be
> +    /// uninitialized. Additionally, access to the inner `T` requires `unsafe`, so the caller needs
> +    /// to verify at that point that the inner value is valid.
> +    pub fn try_ffi_init<E>(
> +        init_func: impl FnOnce(*mut T) -> Result<(), E>,
> +    ) -> impl PinInit<Self, E> {
> +        // SAFETY: We contain a `MaybeUninit`, so it is OK for the `init_func` to not fully
> +        // initialize the `T`.
> +        unsafe { init::pin_init_from_closure::<_, E>(move |slot| init_func(Self::raw_get(slot))) }
> +    }
> +
>      /// Returns a raw pointer to the opaque data.
>      pub const fn get(&self) -> *mut T {
>          UnsafeCell::get(&self.value).cast::<T>()
>
> -- 
> 2.46.0.792.g87dc391469-goog

Re: [PATCH 2/3] rust: file: add f_pos and set_f_pos

[Al Viro]

On Fri, Sep 27, 2024 at 08:56:50AM +0200, Alice Ryhl wrote:

> Okay, interesting. I did not know about all of these llseek helpers.
> I'm definitely happy to make the Rust API force users to do the right
> thing if we can.
> 
> It sounds like we basically have a few different seeking behaviors
> that the driver can choose between, and we want to force the user to
> use one of them?

Depends...  Basically, SEEK_HOLE/SEEK_DATA is seriously fs-specific
(unsurprisingly), and pretty much everything wants the usual relation
between SEEK_SET and SEEK_CUR (<SEEK_CUR,n> is the same as <SEEK_SET,
current position + n>).  SEEK_END availability varies - the simplest
variant is <SEEK_END, n> == <SEEK_SET, size + n>, but there are
cases that genuinely have nothing resembling end-relative seek
(e.g. anything seq_file-related).

It's not so much available instances as available helpers; details of
semantics may seriously vary by the driver.

Note that once upon a time ->f_pos had been exposed to ->read() et.al.;
caused recurring bugs, until we switched to "sample ->f_pos before calling
->read(), pass the reference to local copy into the method, then put
what's the method left behind in there back into ->f_pos".

Something similar might be a good idea for ->llseek().  Locking is
an unpleasant problem, unfortunately.  lseek() is not a terribly hot
codepath, but read() and write() are.  For a while we used to do exclusion
on per-struct file basis for _all_ read/write/lseek; see 797964253d35
"file: reinstate f_pos locking optimization for regular files" for the
point where it eroded.

FWIW, I suspect that unconditionally taking ->f_pos_mutex for llseek(2)
would solve most of the problems - for one thing, with guaranteed
per-struct-file serialization of vfs_llseek() we could handle SEEK_CUR
right there, so that ->llseek() instances would never see it; for another,
we just might be able to pull the same 'pass a reference to local variable
and let it be handled there' trick for ->llseek().  That would require
an audit of locking in the instances, though...

Re: [PATCH 2/3] rust: file: add f_pos and set_f_pos

[Alice Ryhl]

On Fri, Sep 27, 2024 at 9:38 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Fri, Sep 27, 2024 at 08:56:50AM +0200, Alice Ryhl wrote:
>
> > Okay, interesting. I did not know about all of these llseek helpers.
> > I'm definitely happy to make the Rust API force users to do the right
> > thing if we can.
> >
> > It sounds like we basically have a few different seeking behaviors
> > that the driver can choose between, and we want to force the user to
> > use one of them?
>
> Depends...  Basically, SEEK_HOLE/SEEK_DATA is seriously fs-specific
> (unsurprisingly), and pretty much everything wants the usual relation
> between SEEK_SET and SEEK_CUR (<SEEK_CUR,n> is the same as <SEEK_SET,
> current position + n>).  SEEK_END availability varies - the simplest
> variant is <SEEK_END, n> == <SEEK_SET, size + n>, but there are
> cases that genuinely have nothing resembling end-relative seek
> (e.g. anything seq_file-related).
>
> It's not so much available instances as available helpers; details of
> semantics may seriously vary by the driver.
>
> Note that once upon a time ->f_pos had been exposed to ->read() et.al.;
> caused recurring bugs, until we switched to "sample ->f_pos before calling
> ->read(), pass the reference to local copy into the method, then put
> what's the method left behind in there back into ->f_pos".
>
> Something similar might be a good idea for ->llseek().  Locking is
> an unpleasant problem, unfortunately.  lseek() is not a terribly hot
> codepath, but read() and write() are.  For a while we used to do exclusion
> on per-struct file basis for _all_ read/write/lseek; see 797964253d35
> "file: reinstate f_pos locking optimization for regular files" for the
> point where it eroded.
>
> FWIW, I suspect that unconditionally taking ->f_pos_mutex for llseek(2)
> would solve most of the problems - for one thing, with guaranteed
> per-struct-file serialization of vfs_llseek() we could handle SEEK_CUR
> right there, so that ->llseek() instances would never see it; for another,
> we just might be able to pull the same 'pass a reference to local variable
> and let it be handled there' trick for ->llseek().  That would require
> an audit of locking in the instances, though...

Okay, thanks for the explanation. The file position stuff seems pretty
complicated.

One thing to think about is whether there are some behaviors used by
old drivers that new drivers should not use. We can design our Rust
APIs to prevent using it in those legacy ways.

For now I'm dropping this patch from the series at Greg's request.

Alice