linux-kernel - Re: [PATCH v2] rust: device: change the from

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] [day] [month] [year] [list]

Message-ID: <CAM_RzfY1-5LYi+G251FrtHRYOwMh52Z8y-a8o2WYRHnjc0tE-w@mail.gmail.com>
Date: Tue, 1 Oct 2024 14:18:45 -0300
From: Guilherme Giácomo Simões <trintaeoitogc@...il.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: rafael@...nel.org, ojeda@...nel.org, alex.gaynor@...il.com, 
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, 
	benno.lossin@...ton.me, aliceryhl@...gle.com, mcgrof@...nel.org, 
	russ.weight@...ux.dev, dakr@...hat.com, a.hindborg@...nel.org, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rust: device: change the from_raw() function

Greg KH <gregkh@...uxfoundation.org> writes:
>
> On Tue, Oct 01, 2024 at 01:43:38PM -0300, Guilherme Giacomo Simoes wrote:
> > The function Device::from_raw() increments a refcount by a call to
> > bindings::get_device(ptr). This can be confused because usually
> > from_raw() functions don't increment a refcount.
> > Hence, rename Device::from_raw() to avoid confuion with other "from_raw"
> > semantics.
> >
> > The new name of function should be "get_device" to be consistent with
> > the function get_device() already exist in .c files.
> >
> > This function body also changed, because the `into()` will convert the
> > `&'a Device` into `ARef<Device>` and also call `inc_ref` from the
> > `AlwaysRefCounted` trait implemented for Device.
> >
> > Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@...il.com>
> > ---
> >  rust/kernel/device.rs   | 15 +++------------
> >  rust/kernel/firmware.rs |  2 +-
> >  2 files changed, 4 insertions(+), 13 deletions(-)
> >
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 851018eef885..c8199ee079ef 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -51,18 +51,9 @@ impl Device {
> >      ///
> >      /// It must also be ensured that `bindings::device::release` can be called from any thread.
> >      /// While not officially documented, this should be the case for any `struct device`.
> > -    pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> {
> > -        // SAFETY: By the safety requirements, ptr is valid.
> > -        // Initially increase the reference count by one to compensate for the final decrement once
> > -        // this newly created `ARef<Device>` instance is dropped.
> > -        unsafe { bindings::get_device(ptr) };
> > -
> > -        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::device`.
> > -        let ptr = ptr.cast::<Self>();
> > -
> > -        // SAFETY: `ptr` is valid by the safety requirements of this function. By the above call to
> > -        // `bindings::get_device` we also own a reference to the underlying `struct device`.
> > -        unsafe { ARef::from_raw(ptr::NonNull::new_unchecked(ptr)) }
> > +    pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> {
> > +        // SAFETY: By the safety requirements ptr is valid
> > +        unsafe { Self::as_ref(ptr) }.into()
> >      }
> >
> >      /// Obtain the raw `struct device *`.
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > index dee5b4b18aec..13a374a5cdb7 100644
> > --- a/rust/kernel/firmware.rs
> > +++ b/rust/kernel/firmware.rs
> > @@ -44,7 +44,7 @@ fn request_nowarn() -> Self {
> >  ///
> >  /// # fn no_run() -> Result<(), Error> {
> >  /// # // SAFETY: *NOT* safe, just for the example to get an `ARef<Device>` instance
> > -/// # let dev = unsafe { Device::from_raw(core::ptr::null_mut()) };
> > +/// # let dev = unsafe { Device::get_device(core::ptr::null_mut()) };
> >  ///
> >  /// let fw = Firmware::request(c_str!("path/to/firmware.bin"), &dev)?;
> >  /// let blob = fw.data();
> > --
> >
> > differences from v1:
> > - this version don't have 0/1 patch anymore.
> > - this version contains a refactor get_device() function
> >
> > The motivation from this change was will discussion in:
> > https://rust-for-linux.zulipchat.com/#narrow/stream/291566-Library/topic/Inconsistency.20of.20.60from_raw.60.2E
> >
> > I would like to thanks for Greg <gregkh@...uxfoundation.org>, Danilo
> > <dakr@...nel.org> and Alice <aliceryhl@...gle.com> for help me with this
> > patch.
>
> This information goes below the _first_ --- line, not down here at the
> bottom of the patch.  The documentation should explain all of this, you
> did read that, right?  :)
>
> v3 please?
>
> thanks,
>
> greg k-h

I only edited manually my last patch... I maked mistakes, sorry