linux-kernel - Re: [PATCH v10 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver

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: <DB8OT5ZZ4SRO.WP5PBFLML683@kernel.org>
Date: Thu, 10 Jul 2025 23:19:24 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: Uwe Kleine-König <ukleinek@...nel.org>
Cc: "Michal Wilczynski" <m.wilczynski@...sung.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Guo Ren" <guoren@...nel.org>, "Fu Wei"
 <wefu@...hat.com>, "Rob Herring" <robh@...nel.org>, "Krzysztof Kozlowski"
 <krzk+dt@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>, "Paul Walmsley"
 <paul.walmsley@...ive.com>, "Palmer Dabbelt" <palmer@...belt.com>, "Albert
 Ou" <aou@...s.berkeley.edu>, "Alexandre Ghiti" <alex@...ti.fr>, "Marek
 Szyprowski" <m.szyprowski@...sung.com>, "Benno Lossin" <lossin@...nel.org>,
 "Michael Turquette" <mturquette@...libre.com>, "Drew Fustini"
 <fustini@...nel.org>, <linux-kernel@...r.kernel.org>,
 <linux-pwm@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
 <linux-riscv@...ts.infradead.org>, <devicetree@...r.kernel.org>, "Krzysztof
 Kozlowski" <krzysztof.kozlowski@...aro.org>
Subject: Re: [PATCH v10 0/7] Rust Abstractions for PWM subsystem with TH1520
 PWM driver

On Thu Jul 10, 2025 at 10:57 PM CEST, Uwe Kleine-König wrote:
> On Thu, Jul 10, 2025 at 06:06:26PM +0200, Danilo Krummrich wrote:
>> On Thu Jul 10, 2025 at 5:25 PM CEST, Uwe Kleine-König wrote:
>> > Hello Michal,
>> >
>> > On Thu, Jul 10, 2025 at 03:48:08PM +0200, Michal Wilczynski wrote:
>> >> On 7/10/25 15:10, Uwe Kleine-König wrote:
>> >> > On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
>> >> >> On 7/7/25 11:48, Michal Wilczynski wrote:
>> >> >>> The series is structured as follows:
>> >> >>>  - Expose static function pwmchip_release.
>> >> > 
>> >> > Is this really necessary? I didn't try to understand the requirements
>> >> > yet, but I wonder about that. If you get the pwmchip from
>> >> > __pwmchip_add() the right thing to do to release it is to call
>> >> > pwmchip_remove(). Feels like a layer violation.
>> >> 
>> >> It's required to prevent a memory leak in a specific, critical failure
>> >> scenario. The sequence of events is as follows:
>> >> 
>> >>     pwm::Chip::new() succeeds, allocating both the C struct pwm_chip and
>> >>     the Rust drvdata.
>> >> 
>> >>     pwm::Registration::register() (which calls pwmchip_add()) fails for
>> >>     some reason.
>> >
>> 
>> (Just trying to help clear up the confusion.)
>
> Very appreciated!
>
>> > If you called pwmchip_alloc() but not yet pwmchip_add(), the right
>> > function to call for cleanup is pwmchip_put().
>> 
>> That is exactly what is happening when ARef<Chip> is dropped. If the reference
>> count drops to zero, pwmchip_release() is called, which frees the chip. However,
>> this would leave the driver's private data allocation behind, which is owned by
>> the Chip instance.
>
> I don't understand that. The chip and the driver private data both are
> located in the same allocation. How is this a problem of the driver
> private data only then? The kfree() in pwmchip_release() is good enough
> for both?!

Not in the current abstractions, there are two allocations, one for the Chip and
one for the driver's private data, or in other words the abstraction uses
pwmchip_set_drvdata() and pwmchip_get_drvdata().

Having a brief look at pwmchip_alloc(), it seems to me that PWM supports the
subclassing pattern with pwmchip_priv().

We should probably take advantage of that. Assuming we do that, the Rust
abstraction still needs a release() callback because we still need to call
drop_in_place() in order to get the destructor of the driver's private data
type called. We actually missed this in DRM and I fixed it up recently [1].

@Michal: With the subclassing pattern the Chip structure would look like this:

	#[repr(C)]
	#[pin_data]
	pub struct Chip<T> {
	   inner: Opaque<bindings::pwm_chip>,
	   #[pin]
	   data: T,
	}

And in the release() callback would look like this:

    extern "C" fn release(ptr: *mut bindings::pwm_chip) {
        // CAST: Casting `ptr` to `Chip<T>` is valid, since [...].
        let this = ptr.cast<Chip<T>>();

        // SAFETY:
        // - When `release` runs it is guaranteed that there is no further access to `this`.
        // - `this` is valid for dropping.
        unsafe { core::ptr::drop_in_place(this) };
    }

This is exactly what we're doing in DRM as well, I would have recommended this
to begin with, but I didn't recognize that PWM supports subclassing. :)

I recommend having a look at [2].

[1] https://lore.kernel.org/all/20250629153747.72536-1-dakr@kernel.org/
[2] https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-fixes/rust/kernel/drm/device.rs