linux-kernel - Re: [PATCH v9 2/6] rust: pwm: Add complete abstraction layer

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: <aGpqqGMTU3a3O8cn@pollux>
Date: Sun, 6 Jul 2025 14:23:04 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Michal Wilczynski <m.wilczynski@...sung.com>
Cc: Uwe Kleine-König <ukleinek@...nel.org>,
	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
Subject: Re: [PATCH v9 2/6] rust: pwm: Add complete abstraction layer

On Sun, Jul 06, 2025 at 01:45:13PM +0200, Michal Wilczynski wrote:
> +/// Trait defining the operations for a PWM driver.
> +pub trait PwmOps: 'static + Sized {
> +    /// The type of the owned driver data (e.g., `Pin<KBox<...>>`).
> +    type DrvData: 'static + ForeignOwnable;
> +    /// The driver-specific hardware representation of a waveform.
> +    ///
> +    /// This type must be [`Copy`], [`Default`], and fit within `PWM_WFHWSIZE`.
> +    type WfHw: Copy + Default;
> +
> +    /// Optional hook for when a PWM device is requested.
> +    fn request(
> +        _chip: &Chip<Self::DrvData>,
> +        _pwm: &Device,
> +        _parent_dev: &device::Device<Bound>,
> +    ) -> Result {
> +        Ok(())
> +    }
> +
> +    /// Optional hook for when a PWM device is freed.
> +    fn free(_chip: &Chip<Self::DrvData>, _pwm: &Device, _parent_dev: &device::Device<Bound>) {}

NIT: I can't think of a case providing this callback in Rust is useful. Do you
have a clear use-case in mind? Otherwise, I'd not provide this callback until
you have one. Should be trivial to add later on.

> +impl<T: PwmOps> Adapter<T> {

<snip>

> +    /// # Safety
> +    ///
> +    /// `dev` must be a valid pointer to a `bindings::device` embedded within a
> +    /// `bindings::pwm_chip`. This function is called by the device core when the
> +    /// last reference to the device is dropped.
> +    unsafe extern "C" fn release_callback(dev: *mut bindings::device) {
> +        // SAFETY: The function's contract guarantees that `dev` points to a `device`
> +        // field embedded within a valid `pwm_chip`. `container_of!` can therefore
> +        // safely calculate the address of the containing struct.
> +        let c_chip_ptr = unsafe { container_of!(dev, bindings::pwm_chip, dev) };
> +
> +        // SAFETY: `c_chip_ptr` is a valid pointer to a `pwm_chip` as established
> +        // above. Calling this FFI function is safe.
> +        let drvdata_ptr = unsafe { bindings::pwmchip_get_drvdata(c_chip_ptr) };
> +
> +        if !drvdata_ptr.is_null() {

Is this check needed? I think one can't create a pwm::Chip instance without
providing a T, so this pointer can't be NULL I think.

> +            // SAFETY: `drvdata_ptr` was stored by `Chip::new` from an owned `T::DrvData`
> +            // and is guaranteed to be valid if non-null. `from_foreign` can safely
> +            // reclaim ownership to allow Rust to drop and free the data.
> +            let _owned_drvdata = unsafe { T::DrvData::from_foreign(drvdata_ptr.cast()) };
> +        }
> +    }

If you overwrite this callback (as you do below) you're leaking the memory
allocated by pwmchip_alloc().

The simple way to solve this would be to call pwmchip_release() from here.

<snip>

> +impl<T: 'static + ForeignOwnable> Chip<T> {
> +    /// Allocates and wraps a PWM chip using `bindings::pwmchip_alloc`.
> +    ///
> +    /// Returns an [`ARef<Chip>`] managing the chip's lifetime via refcounting
> +    /// on its embedded `struct device`.
> +    pub fn new<O: PwmOps<DrvData = T>>(
> +        parent_dev: &device::Device,
> +        npwm: u32,
> +        sizeof_priv: usize,
> +        drvdata: T,
> +    ) -> Result<ARef<Self>> {
> +        // SAFETY: `parent_device_for_dev_field.as_raw()` is valid.
> +        // `bindings::pwmchip_alloc` returns a valid `*mut bindings::pwm_chip` (refcount 1)
> +        // or an ERR_PTR.
> +        let c_chip_ptr_raw =
> +            unsafe { bindings::pwmchip_alloc(parent_dev.as_raw(), npwm, sizeof_priv) };
> +
> +        let c_chip_ptr: *mut bindings::pwm_chip = error::from_err_ptr(c_chip_ptr_raw)?;
> +
> +        // Set the custom release function on the embedded device. This is the crucial step
> +        // to ensure `drvdata` is freed when the chip's refcount reaches zero, regardless
> +        // of whether `Registration::register` was called.
> +        // SAFETY: `c_chip_ptr` points to a valid chip.
> +        unsafe { (*c_chip_ptr).dev.release = Some(Adapter::<O>::release_callback); }

This overwrites [1].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/core.c?h=v6.16-rc4#n1601