Received-SPF: pass (google.com: domain of linux-kernel+bounces-205909-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 07 Jun 2024 14:18:54 +0300
From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
To: Alice Ryhl <aliceryhl@google.com>, Viresh Kumar <viresh.kumar@linaro.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>, Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, Wedson Almeida Filho <wedsonaf@gmail.com>, Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, Bj=?UTF-8?B?w7Y=?= rn Roy Baron <bjorn3_gh@protonmail.com>, Benno Lossin <benno.lossin@proton.me>, Andreas Hindborg <a.hindborg@samsung.com>, linux-pm@vger.kernel.org, Vincent Guittot <vincent.guittot@linaro.org>, Stephen Boyd <sboyd@kernel.org>, Nishanth Menon <nm@ti.com>, rust-for-linux@vger.kernel.org, Manos Pitsidianakis <manos.pitsidianakis@linaro.org>, Erik Schilling <erik.schilling@linaro.org>, Alex Benn=?UTF-8?B?w6k=?= e <alex.bennee@linaro.org>, Joakim Bech <joakim.bech@linaro.org>, Rob Herring <robh@kernel.org>, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH V2 1/8] rust: Add initial bindings for OPP framework
User-Agent: meli 0.8.5
References: <cover.1717750631.git.viresh.kumar@linaro.org> <e74e3a14e6da3f920cee90d32a023ba4805328a0.1717750631.git.viresh.kumar@linaro.org> <CAH5fLgjChZCtTUnHVHJat-sXFyLVE+MgDXrNDiUD0LNsUndpBQ@mail.gmail.com>
In-Reply-To: <CAH5fLgjChZCtTUnHVHJat-sXFyLVE+MgDXrNDiUD0LNsUndpBQ@mail.gmail.com>
Message-ID: <epjoz.s6l4l8qrt331@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset=utf-8; format=flowed

On Fri, 07 Jun 2024 13:51, Alice Ryhl <aliceryhl@google.com> wrote:
>On Fri, Jun 7, 2024 at 11:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>
>> This commit adds initial Rust bindings for the Operating performance
>> points (OPP) core. This adds bindings for `struct dev_pm_opp` and
>> `struct dev_pm_opp_data` to begin with.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
>> +//! Operating performance points.
>> +//!
>> +//! This module provides bindings for interacting with the OPP subsystem.
>> +//!
>> +//! C header: [`include/linux/pm_opp.h`](../../../../../../include/linux/pm_opp.h)
>
>Please use srctree links instead.
>
>C header: [`include/linux/pm_opp.h`](srctree/include/linux/pm_opp.h)
>
>> +impl OPP {
>> +    /// Creates a reference to a [`OPP`] from a valid pointer.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
>> +    /// returned [`OPP`] reference.
>> +    pub unsafe fn from_ptr_owned(ptr: *mut bindings::dev_pm_opp) -> Result<ARef<Self>> {
>> +        let ptr = ptr::NonNull::new(ptr).ok_or(ENODEV)?;
>> +
>> +        // SAFETY: The safety requirements guarantee the validity of the pointer.
>> +        //
>> +        // INVARIANT: The refcount is already incremented by the C API that returned the pointer,
>> +        // and we pass ownership of the refcount to the new `ARef<OPP>`.
>> +        Ok(unsafe { ARef::from_raw(ptr.cast()) })
>> +    }
>> +
>> +    /// Creates a reference to a [`OPP`] from a valid pointer.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
>> +    /// returned [`OPP`] reference.
>> +    pub unsafe fn from_ptr(ptr: *mut bindings::dev_pm_opp) -> Result<ARef<Self>> {
>> +        let opp = unsafe { Self::from_ptr_owned(ptr) }?;
>> +
>> +        // Take an extra reference to the OPP since the caller didn't take it.
>> +        opp.inc_ref();
>> +
>> +        Ok(opp)
>> +    }
>
>I would recommend a slightly different approach here. You can provide
>a method called `from_raw_opp` that takes a *mut bindings::dev_pm_opp
>and returns a &Self. The ARef type provides a method that converts
>&Self to ARef<Self> by taking a refcount. This way, users would also
>be able to call OPP methods without giving Rust any refcounts. You can

Wouldn't this allow for use-after-free? What if the refcount drops to 0 
before the method is called?

>As for `from_ptr_owned`, I would probably rename it to
>`from_raw_opp_owned` or similar. It's often nice to use a more
>descriptive name than just "ptr".
>I think most existing examples call this `as_raw` and mark it 
>`#[inline]`.

I think `ptr` is more idiomatic to Rust users, not that your suggestion 
is wrong. from_ptr_owned also implies the function signature.


>
>> +    /// Adds an OPP dynamically.
>> +    pub fn add(dev: ARef<Device>, mut data: Data) -> Result<()> {
>> +        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
>> +        // requirements.
>> +        to_result(unsafe { bindings::dev_pm_opp_add_dynamic(dev.as_raw(), &mut data.0) })
>> +    }
>> +
>> +    /// Removes a dynamically added OPP.
>> +    pub fn remove(dev: ARef<Device>, freq: u64) {
>> +        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
>> +        // requirements.
>> +        unsafe { bindings::dev_pm_opp_remove(dev.as_raw(), freq) };
>> +    }
>
>Is it intentional that these methods take ownership of a refcount to
>the device that it then drops after calling the C function?

use-after-free again? Though I'm suggesting this without actually 
examining if it can happen.