Received-SPF: pass (google.com: domain of linux-kernel+bounces-205921-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <cover.1717750631.git.viresh.kumar@linaro.org> <e74e3a14e6da3f920cee90d32a023ba4805328a0.1717750631.git.viresh.kumar@linaro.org>
 <CAH5fLgjChZCtTUnHVHJat-sXFyLVE+MgDXrNDiUD0LNsUndpBQ@mail.gmail.com> <epjoz.s6l4l8qrt331@linaro.org>
In-Reply-To: <epjoz.s6l4l8qrt331@linaro.org>
From: Alice Ryhl <aliceryhl@google.com>
Date: Fri, 7 Jun 2024 13:30:33 +0200
Message-ID: <CAH5fLgiBtUkCfTt198mEkZD=-OqC7cdNY-hgBWPBE310ti0V+w@mail.gmail.com>
Subject: Re: [RFC PATCH V2 1/8] rust: Add initial bindings for OPP framework
To: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>, "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>, 
	=?UTF-8?Q?Bj=C3=B6_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, 
	Erik Schilling <erik.schilling@linaro.org>, =?UTF-8?Q?Alex_Benn=C3=A9_e?= <alex.bennee@linaro.org>, 
	Joakim Bech <joakim.bech@linaro.org>, Rob Herring <robh@kernel.org>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jun 7, 2024 at 1:24=E2=80=AFPM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> On Fri, 07 Jun 2024 13:51, Alice Ryhl <aliceryhl@google.com> wrote:
> >On Fri, Jun 7, 2024 at 11:12=E2=80=AFAM Viresh Kumar <viresh.kumar@linar=
o.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 subsys=
tem.
> >> +//!
> >> +//! C header: [`include/linux/pm_opp.h`](../../../../../../include/li=
nux/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) -> R=
esult<ARef<Self>> {
> >> +        let ptr =3D 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 AP=
I 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 =3D 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?

The method would be unsafe and require the caller to avoid that.

/// Creates a reference to a [`File`] from a valid pointer.
///
/// # Safety
///
/// The caller must ensure that `ptr` points at a valid file and that
/// the file's refcount is positive for the duration of 'a.
pub unsafe fn from_raw_file<'a>(ptr: *const bindings::file) -> &'a File {
    // SAFETY: The caller guarantees that the pointer is not dangling
    // and stays valid for the duration of 'a. The cast is okay because
    // `File` is `repr(transparent)`.
    unsafe { &*ptr.cast() }
}

> >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_ra=
w(), &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.

Rust references are required to not be dangling, so if you take a
&Device, then the caller promises that they will keep the device alive
for the duration of the call.

Alice