linux-kernel - Re: [PATCH 1/3] rust: char_dev: add character device abstraction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2024101253-golf-detonator-592c@gregkh>
Date: Sat, 12 Oct 2024 09:36:52 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Josef Zoller <josef@...terzollerpiano.com>
Cc: Arnd Bergmann <arnd@...db.de>, 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>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 1/3] rust: char_dev: add character device abstraction

On Fri, Oct 11, 2024 at 08:55:42PM +0200, Josef Zoller wrote:
> +unsigned int rust_helper_MAJOR(dev_t dev)
> +{
> +	return MAJOR(dev);
> +}

I don't think you use this function in the code anywhere.

> +unsigned int rust_helper_MINOR(dev_t dev)
> +{
> +	return MINOR(dev);
> +}

Is this really needed?  No driver should care about their minor number,
except to possibly set it, not read it.

> +dev_t rust_helper_MKDEV(unsigned int major, unsigned int minor)
> +{
> +	return MKDEV(major, minor);
> +}

If you are only doing dynamic creation, as your initial text said, I
don't think you need this either as the kernel should create it for you.


> diff --git a/rust/kernel/char_dev.rs b/rust/kernel/char_dev.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..b81c0d55ab60f18dc82a99991318a5ae0a26e560
> --- /dev/null
> +++ b/rust/kernel/char_dev.rs
> @@ -0,0 +1,976 @@
> +// SPDX-License-Identifier: GPL-2.0

Minor nit, you forgot a copyright line :)

> +
> +//! Character device support.
> +//!
> +//! C headers: [`include/linux/cdev.h`](srctree/include/linux/cdev.h) and
> +//! [`include/linux/fs.h`](srctree/include/linux/fs.h)
> +
> +use crate::{
> +    bindings, container_of,
> +    error::{to_result, VTABLE_DEFAULT_ERROR},
> +    fs::{File, LocalFile},
> +    ioctl::IoctlCommand,
> +    prelude::*,
> +    types::{ForeignOwnable, Opaque},
> +    uaccess::{UserPtr, UserSlice, UserSliceReader, UserSliceWriter},
> +};
> +use core::{ffi, mem, ops::Deref};
> +
> +/// Character device ID.
> +///
> +/// This is a wrapper around the kernel's `dev_t` type and identifies a
> +/// character device by its major and minor numbers.
> +#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
> +#[repr(transparent)]
> +pub struct CharDeviceID(bindings::dev_t); // u32
> +
> +impl CharDeviceID {
> +    /// Creates a new device ID from the given major and minor numbers.
> +    ///
> +    /// This corresponds to the kernel's `MKDEV` macro.
> +    pub fn new(major: u32, minor: u32) -> Self {
> +        // SAFETY: The kernel's `MKDEV` macro is safe to call with any values.
> +        Self(unsafe { bindings::MKDEV(major, minor) })
> +    }
> +
> +    /// Returns the major number of the device ID.
> +    ///
> +    /// This corresponds to the kernel's `MAJOR` macro.
> +    pub fn major(&self) -> u32 {
> +        // SAFETY: The kernel's `MAJOR` macro is safe to call with any value.
> +        unsafe { bindings::MAJOR(self.0) }
> +    }
> +
> +    /// Returns the minor number of the device ID.
> +    ///
> +    /// This corresponds to the kernel's `MINOR` macro.
> +    pub fn minor(&self) -> u32 {
> +        // SAFETY: The kernel's `MINOR` macro is safe to call with any value.
> +        unsafe { bindings::MINOR(self.0) }
> +    }
> +}
> +
> +/// Seek mode for the `llseek` method.
> +///
> +/// This enum corresponds to the `SEEK_*` constants in the kernel.
> +#[repr(u32)]
> +pub enum Whence {
> +    /// Set the file position to `offset`.
> +    Set = bindings::SEEK_SET,
> +    /// Set the file position to the current position plus `offset`.
> +    Cur = bindings::SEEK_CUR,
> +    /// Set the file position to the end of the file plus `offset`.
> +    End = bindings::SEEK_END,
> +    /// Set the file position to the next location in the file greater than or
> +    /// equal to `offset` containing data.
> +    Data = bindings::SEEK_DATA,
> +    /// Set the file position to the next hole in the file greater than or
> +    /// equal to `offset`.
> +    Hole = bindings::SEEK_HOLE,
> +}
> +
> +// Make sure at compile time that the `Whence` enum can be safely converted
> +// from integers up to `SEEK_MAX`.
> +const _: () = assert!(Whence::Hole as u32 == bindings::SEEK_MAX);
> +
> +/// Trait implemented by a registered character device.
> +///
> +/// A registered character device just handles the `open` operation on the
> +/// device file and returns an open device type (which implements the
> +/// [`OpenCharDevice`] trait) that handles the actual I/O operations on the
> +/// device file. Optionally, the `release` operation can be implemented to
> +/// handle the final close of the device file, but simple cleanup can also be
> +/// done in the `Drop` implementation of the open device type.

release is traditionally where you handle cleaning up what was allocated
for this "open", and then drop can handle any "global" state for the
device associated with this specific instance.  So "simple cleanup"
might not be possible in both places, as they are different parts of the
lifecycle of a device.

thanks,

greg k-h