linux-kernel - Re: [RFC PATCH V3 1/8] rust: Add initial bindings for OPP framework

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

Message-ID: <ZoVvn0QCSR8y4HQJ@Boquns-Mac-mini.home>
Date: Wed, 3 Jul 2024 08:34:55 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
	Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...sung.com>,
	Alice Ryhl <aliceryhl@...gle.com>, linux-pm@...r.kernel.org,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Stephen Boyd <sboyd@...nel.org>, Nishanth Menon <nm@...com>,
	rust-for-linux@...r.kernel.org,
	Manos Pitsidianakis <manos.pitsidianakis@...aro.org>,
	Erik Schilling <erik.schilling@...aro.org>,
	Alex Bennée <alex.bennee@...aro.org>,
	Joakim Bech <joakim.bech@...aro.org>, Rob Herring <robh@...nel.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH V3 1/8] rust: Add initial bindings for OPP framework

Hi Viresh,

On Wed, Jul 03, 2024 at 12:44:26PM +0530, Viresh Kumar 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.
> 
> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@...aro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
[...]
> +
> +/// Operating performance point (OPP).
> +///
> +/// # Invariants
> +///
> +/// The pointer stored in `Self` is non-null and valid for the lifetime of the ARef instance. In
> +/// particular, the ARef instance owns an increment on underlying object´s reference count.

Since you use `ARef` pattern now, you may want to rewrite this
"invariants".

> +#[repr(transparent)]
> +pub struct OPP(Opaque<bindings::dev_pm_opp>);
> +
> +// SAFETY: `OPP` only holds a pointer to a C OPP, which is safe to be used from any thread.
> +unsafe impl Send for OPP {}
> +
> +// SAFETY: `OPP` only holds a pointer to a C OPP, references to which are safe to be used from any
> +// thread.
> +unsafe impl Sync for OPP {}
> +

Same for the above safety comments, as they are still based on the old
implementation.

> +// SAFETY: The type invariants guarantee that [`OPP`] is always refcounted.
> +unsafe impl AlwaysRefCounted for OPP {
> +    fn inc_ref(&self) {
> +        // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> +        unsafe { bindings::dev_pm_opp_get(self.0.get()) };
> +    }
> +
> +    unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
> +        unsafe { bindings::dev_pm_opp_put(obj.cast().as_ptr()) }
> +    }
> +}
> +
> +impl OPP {
[...]
> +
> +impl Drop for OPP {

I don't think you need the `drop` implementation here, since it should
be already handled by `impl AlwaysRefCounted`, could you try to a doc
test for this? Something like:

	let opp: ARef<OPP> = <from a raw dev_pm_opp ponter whose refcount is 1>
	drop(opp);

IIUC, this will result double-free with the current implementation.

Overall, `OPP` is now representing to the actual device instead of the
pointer to the device, so the `drop` function won't need to handle the
refcounting.

Regards,
Boqun

> +    fn drop(&mut self) {
> +        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
> +        unsafe { bindings::dev_pm_opp_put(self.as_raw()) }
> +    }
> +}
> -- 
> 2.31.1.272.g89b43f80a514
>