| 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
| ||
|
Message-ID: <Zx-Tgm9CJO-Myrgv@pollux>
Date: Mon, 28 Oct 2024 14:37:06 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Abdiel Janulgue <abdiel.janulgue@...il.com>,
rust-for-linux@...r.kernel.org, aliceryhl@...gle.com,
dakr@...hat.com, linux-kernel@...r.kernel.org, airlied@...hat.com,
miguel.ojeda.sandonis@...il.com
Subject: Re: [PATCH v2 5/5] rust: firmware: implement `Ownable` for Firmware
On Sun, Oct 27, 2024 at 03:20:03PM -0700, Boqun Feng wrote:
> On Wed, Oct 23, 2024 at 11:35:15AM +0200, Danilo Krummrich wrote:
> > On Wed, Oct 23, 2024 at 01:44:49AM +0300, Abdiel Janulgue wrote:
> > > For consistency, wrap the firmware as an `Owned` smart pointer in the
> > > constructor.
> > >
> > > Cc: Danilo Krummrich <dakr@...hat.com>
> > > Suggested-by: Boqun Feng <boqun.feng@...il.com>
> > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
> > > ---
> > > rust/kernel/firmware.rs | 31 ++++++++++++++++++-------------
> > > 1 file changed, 18 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > > index dee5b4b18aec..6da834b37455 100644
> > > --- a/rust/kernel/firmware.rs
> > > +++ b/rust/kernel/firmware.rs
> > > @@ -4,8 +4,8 @@
> > > //!
> > > //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h)
> > >
> > > -use crate::{bindings, device::Device, error::Error, error::Result, str::CStr};
> > > -use core::ptr::NonNull;
> > > +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr,
> > > + types::{Opaque, Owned, Ownable}};
> > >
> > > /// # Invariants
> > > ///
> > > @@ -52,10 +52,11 @@ fn request_nowarn() -> Self {
> > > /// # Ok(())
> > > /// # }
> > > /// ```
> > > -pub struct Firmware(NonNull<bindings::firmware>);
> > > + #[repr(transparent)]
> > > +pub struct Firmware(Opaque<bindings::firmware>);
> > >
> > > impl Firmware {
> > > - fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> > > + fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Owned<Self>> {
> >
> > I think it's fine to implement this for consistency, but I'm not sure I like
> > that drivers have to refer to it as `Owned<Firmware>`.
> >
>
> May I ask why not? ;-)
I think it's because with this an instance of `Firmware` is never a valid thing
to have, which is a bit weird at a first glance.
But I fully agree with the existance of the `Owned` type and the rationale
below.
>
> Ideally, we should not wrap a pointer to particular type, instead we
> should wrap the type and then combine it with a meaningful pointer type,
> e.g. Box<>, ARef<>, Owned<> ... in this way, we de-couple how the
> lifetime of object is maintained (described by the pointer type) and
> what operations are available on the object (described by the wrapper
> type).
>
> If later on, a firmware object creation is doable in pure Rust code for
> some condition, we can then have a function that returns a
> `KBox<Firmware>` (assume using kmalloc for the object), and it will just
> work (tm).
>
> Regards,
> Boqun
>
> > Anyway, if we keep it this way the patch also needs the following change.
> >
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > index 6da834b37455..1db854eb2422 100644
> > --- a/rust/kernel/firmware.rs
> > +++ b/rust/kernel/firmware.rs
> > @@ -115,8 +115,8 @@ unsafe fn ptr_drop(ptr: *mut Self) {
> >
> > // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, which is safe to be used from
> > // any thread.
> > -unsafe impl Send for Firmware {}
> > +unsafe impl Send for Owned<Firmware> {}
> >
> > // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, references to which are safe to
> > // be used from any thread.
> > -unsafe impl Sync for Firmware {}
> > +unsafe impl Sync for Owned<Firmware> {}
> >
> > > let mut fw: *mut bindings::firmware = core::ptr::null_mut();
> > > let pfw: *mut *mut bindings::firmware = &mut fw;
> > >
> > > @@ -65,25 +66,26 @@ fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> > > if ret != 0 {
> > > return Err(Error::from_errno(ret));
> > > }
> > > -
> > > + // CAST: Self` is a `repr(transparent)` wrapper around `bindings::firmware`.
> > > + let ptr = fw.cast::<Self>();
> > > // SAFETY: `func` not bailing out with a non-zero error code, guarantees that `fw` is a
> > > // valid pointer to `bindings::firmware`.
> > > - Ok(Firmware(unsafe { NonNull::new_unchecked(fw) }))
> > > + Ok(unsafe { Owned::to_owned(ptr) })
> > > }
> > >
> > > /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
> > > - pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> > > + pub fn request(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> > > Self::request_internal(name, dev, FwFunc::request())
> > > }
> > >
> > > /// Send a request for an optional firmware module. See also
> > > /// `bindings::firmware_request_nowarn`.
> > > - pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
> > > + pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> > > Self::request_internal(name, dev, FwFunc::request_nowarn())
> > > }
> > >
> > > fn as_raw(&self) -> *mut bindings::firmware {
> > > - self.0.as_ptr()
> > > + self.0.get()
> > > }
> > >
> > > /// Returns the size of the requested firmware in bytes.
> > > @@ -101,10 +103,13 @@ pub fn data(&self) -> &[u8] {
> > > }
> > > }
> > >
> > > -impl Drop for Firmware {
> > > - fn drop(&mut self) {
> > > - // SAFETY: `self.as_raw()` is valid by the type invariant.
> > > - unsafe { bindings::release_firmware(self.as_raw()) };
> > > +unsafe impl Ownable for Firmware {
> > > + unsafe fn ptr_drop(ptr: *mut Self) {
> > > + // SAFETY:
> > > + // - By the type invariants, we have ownership of the ptr and can free it.
> > > + // - Per function safety, this is called in Owned::drop(), so `ptr` is a
> > > + // unique pointer to object, it's safe to release the firmware.
> > > + unsafe { bindings::release_firmware(ptr.cast()) };
> > > }
> > > }
> > >
> > > --
> > > 2.43.0
> > >
> >
>
Powered by blists - more mailing lists