| 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
| ||
|
Date: Mon, 17 Jun 2024 16:00:22 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Danilo Krummrich <dakr@...hat.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, mcgrof@...nel.org,
russ.weight@...ux.dev, 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 v3 2/2] rust: add firmware abstractions
On Tue, Jun 18, 2024 at 12:50:45AM +0200, Danilo Krummrich wrote:
[...]
> > /// A smart pointer owns the underlying data.
> > pub struct Owned<T: Ownable> {
> > ptr: NonNull<T>,
> > }
> >
> > impl<T: Ownable> Owned<T> {
> > /// # Safety
> > /// `ptr` needs to be a valid pointer, and it should be the
> > /// unique owner to the object, in other words, no one can touch
> > /// or free the underlying data.
> > pub unsafe to_owned(ptr: *mut T) -> Self {
> > // SAFETY: Per function safety requirement.
> > Self { ptr: unsafe { NonNull::new_unchecked(ptr) } }
> > }
> >
> > /// other safe constructors are available if a initializer (impl
> > /// Init) is provided
> > }
> >
> > /// A Ownable type is a type that can be put into `Owned<T>`, and
> > /// when `Owned<T>` drops, `ptr_drop` will be called.
> > pub unsafe trait Ownable {
> > /// # Safety
> > /// This could only be called in the `Owned::drop` function.
> > unsafe fn ptr_drop(ptr: *mut Self);
> > }
> >
> > impl<T: Ownable> Drop for Owned<T> {
> > fn drop(&mut self) {
> > /// SAFETY: In Owned<T>::drop.
> > unsafe {
> > <T as Ownable>::ptr_drop(self.as_mut_ptr());
> > }
> > }
> > }
> >
> > we can implement Deref and DerefMut easily on `Owned<T>`. And then we
> > could define Firmware as
> >
> > #[repr(transparent)]
> > pub struct Firmware(Opaque<bindings::firmware>);
> >
> > and
> >
> > unsafe impl Ownable for Firmware {
> > unsafe fn ptr_drop(ptr: *mut Self) {
> > // SAFETY: 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()); }
> > }
> > }
> >
> > and the request_*() will return a `Result<Owned<Self>>`.
> >
> > Alice mentioned the need of this in page as well:
> >
> > https://lore.kernel.org/rust-for-linux/CAH5fLgjrt0Ohj1qBv=GrqZumBTMQ1jbsKakChmxmG2JYDJEM8w@mail.gmail.com
>
> I think in the `Page` case this is useful to create `Page` references from
> previously allocated memory.
>
> In the case of `Firmware`, I agree it makes sense to use it once we have it,
> but other than for consistency, is there any advantage?
>
To help people build future abstraction easier (and make review easier
as well). But I'm also waiting for a third use case, yes, I usually
wait for 3 cases to begin thinking about generalization ;-)
> >
> > Just bring it up while we are (maybe not? ;-)) at it. Also I would like
> > to hear whether this would work for Firmware in the longer-term ;-) But
> > yes, I'm not that worried about merging it as it is if others are all
> > OK.
>
> I think there's not too much to add here in the future, once we got an allocator
> API (I should get back to that soon), I want to add a method that copies the
> data to a new buffer allocated with a given allocator. And maybe we want to
> support a few other request_firmware_* functions in the future, but none of that
> should require the above abstraction.
>
Thank you!
> >
> > > +impl Firmware {
> > > + fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> > > + let mut fw: *mut bindings::firmware = core::ptr::null_mut();
> > > + let pfw: *mut *mut bindings::firmware = &mut fw;
> > > +
> > > + // SAFETY: `pfw` is a valid pointer to a NULL initialized `bindings::firmware` pointer.
> > > + // `name` and `dev` are valid as by their type invariants.
> > > + let ret = unsafe { func(pfw as _, name.as_char_ptr(), dev.as_raw()) };
> > > + if ret != 0 {
> > > + return Err(Error::from_errno(ret));
> > > + }
> > > +
> > > + // 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) }))
> > > + }
> > > +
> > > + /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
> > > + pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> > > + Self::request_internal(name, dev, bindings::request_firmware)
> > > + }
> > > +
> > > + /// Send a request for an optional firmware module. See also
> > > + /// `bindings::firmware_request_nowarn`.
> > > + pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
> > > + Self::request_internal(name, dev, bindings::firmware_request_nowarn)
> > > + }
> > > +
> > > + fn as_raw(&self) -> *mut bindings::firmware {
> > > + self.0.as_ptr()
> > > + }
> > > +
> > > + /// Returns the size of the requested firmware in bytes.
> > > + pub fn size(&self) -> usize {
> > > + // SAFETY: Safe by the type invariant.
> > > + unsafe { (*self.as_raw()).size }
> > > + }
> > > +
> > > + /// Returns the requested firmware as `&[u8]`.
> > > + pub fn data(&self) -> &[u8] {
> > > + // SAFETY: Safe by the type invariant. Additionally, `bindings::firmware` guarantees, if
> >
> > Does this "Safe by the type invariant" also covers the following safe
> > requirement of `from_raw_parts`?
> >
> > The memory referenced by the returned slice must not be mutated for the duration of lifetime 'a, except inside an UnsafeCell.
> >
> > in that `&[u8]` has the same lifetime as `&self`, and as long as
> > `&self` exists, no function can touch the inner `data`? If so, I
> > probably want to call this out.
>
> Yes, nothing should ever modify the firmware buffer after it has been requested
> successfully. I can add this to the type invariant.
>
Oh, you have an even easier (stronger) type invariant. Yes, please add
it and use it here. Thanks!
Regards,
Boqun
> >
> > Regards,
> > Boqun
> >
> > > + // successfully requested, that `bindings::firmware::data` has a size of
> > > + // `bindings::firmware::size` bytes.
> > > + unsafe { core::slice::from_raw_parts((*self.as_raw()).data, self.size()) }
> > > + }
> > > +}
> > > +
> > > +impl Drop for Firmware {
> > > + fn drop(&mut self) {
> > > + // SAFETY: Safe by the type invariant.
> > > + unsafe { bindings::release_firmware(self.as_raw()) };
> > > + }
> > > +}
> > > +
> > > +// 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 {}
> > > +
> > > +// 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 {}
> > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > > index dd1207f1a873..7707cb013ce9 100644
> > > --- a/rust/kernel/lib.rs
> > > +++ b/rust/kernel/lib.rs
> > > @@ -30,6 +30,8 @@
> > > mod build_assert;
> > > pub mod device;
> > > pub mod error;
> > > +#[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
> > > +pub mod firmware;
> > > pub mod init;
> > > pub mod ioctl;
> > > #[cfg(CONFIG_KUNIT)]
> > > --
> > > 2.45.1
> > >
> >
>
Powered by blists - more mailing lists