linux-kernel - Re: [PATCH v2 1/2] rust: add abstraction for struct device

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 10 Jun 2024 11:38:07 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Danilo Krummrich <dakr@...hat.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, mcgrof@...nel.org,
	russell.h.weight@...el.com, ojeda@...nel.org, alex.gaynor@...il.com,
	wedsonaf@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	benno.lossin@...ton.me, a.hindborg@...sung.com,
	aliceryhl@...gle.com, airlied@...il.com, fujita.tomonori@...il.com,
	pstanner@...hat.com, ajanulgu@...hat.com, lyude@...hat.com,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] rust: add abstraction for struct device

On Mon, Jun 10, 2024 at 08:02:27PM +0200, Danilo Krummrich wrote:
[...]
> +/// A reference-counted device.
> +///
> +/// This structure represents the Rust abstraction for a C `struct device`. This implementation
> +/// abstracts the usage of an already existing C `struct device` within Rust code that we get
> +/// passed from the C side.
> +///
> +/// An instance of this abstraction can be obtained temporarily or permanent.
> +///
> +/// A temporary one is bound to the lifetime of the C `struct device` pointer used for creation.
> +/// A permanent instance is always reference-counted and hence not restricted by any lifetime
> +/// boundaries.
> +///
> +/// For subsystems it is recommended to create a permanent instance to wrap into a subsystem
> +/// specifc device structure (e.g. `pci::Device`). This is useful for passing it to drivers in
> +/// `T::probe()`, such that a driver can store the `ARef<Device>` (equivalent to storing a
> +/// `struct device` pointer in a C driver) for arbitrary purposes, e.g. allocating DMA coherent
> +/// memory.
> +///
> +/// # Invariants
> +///
> +/// The pointer stored in `Self` is non-null and valid for the lifetime of the `ARef` instance. In
> +/// particular, the `ARef` instance owns an increment on the underlying object’s reference count.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::device>);
> +
[...]
> +
> +// SAFETY: `Device` only holds a pointer to a C `struct device`, which is safe to be used from any
> +// thread.
> +unsafe impl Send for Device {}
> +
> +// SAFETY: `Device` only holds a pointer to a C `struct device`, references to which are safe to be
> +// used from any thread.
> +unsafe impl Sync for Device {}

These comments need some rework, `Device` is not a pointer to `struct
device` anymore. For the `Sync` one, how about:

// SAFETY: `Device` can be shared among threads because all immutable
// methods are protected by the synchronization in `struct device`.
unsafe impl Sync for Device {}

and for `Send`, I actually don't think we can easily say the generic
`Device` is `Send`: you can create a `struct device` where `->release`
requires to be run on the same thread that creates the `device`, and
nothing is wrong about it, I think (e.g. making a thread be the sole
owner of some special devices). Unless, in the #Invariants of `Device`,
and the #safety of `from_ptr`, you mention that `Device` assume its
`->release` can be called on any thread.

Regards,
Boqun

> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index fbd91a48ff8b..dd1207f1a873 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -28,6 +28,7 @@
>  
>  pub mod alloc;
>  mod build_assert;
> +pub mod device;
>  pub mod error;
>  pub mod init;
>  pub mod ioctl;
> -- 
> 2.45.1
>