linux-kernel - Re: [PATCH v3 3/9] rust: pwm: Add driver operations trait and registration support

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: <aFF-fNOOEzoiomFu@pollux>
Date: Tue, 17 Jun 2025 16:41:00 +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>,
	Drew Fustini <drew@...7.com>, 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>,
	Stephen Boyd <sboyd@...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,
	linux-clk@...r.kernel.org
Subject: Re: [PATCH v3 3/9] rust: pwm: Add driver operations trait and
 registration support

On Tue, Jun 17, 2025 at 04:07:26PM +0200, Michal Wilczynski wrote:
> +/// Manages the registration of a PWM chip, ensuring `pwmchip_remove` is called on drop.
> +pub struct Registration {
> +    chip: ManuallyDrop<ARef<Chip>>,

Why is this ManuallyDrop when you call ManuallyDrop::drop(&mut self.chip) as
the last thing in Registration::drop()?

I think you don't need ManuallyDrop here.

> +}
> +
> +impl Registration {
> +    /// Registers a PWM chip (obtained via `Chip::new`) with the PWM subsystem.
> +    ///
> +    /// Takes an [`ARef<Chip>`]. On `Drop` of the returned `Registration` object,
> +    /// `pwmchip_remove` is called for the chip.
> +    pub fn new(chip: ARef<Chip>, ops_vtable: &'static PwmOpsVTable) -> Result<Self> {

For the reason mentioned in [1] this should either return Result<Devres<Self>>
or just Result, if you use Devres::new_foreign_owned() (see also [2]).

In case of the latter, the Registration instance is automatically dropped once
the parent device is unbound.

If you go with the first, you can drop the Devres<Registration> (and hence the
inner Registration) at any arbitrary point of time, but Devres will still
gurarantee that the inner Registration is dropped once the parent device is
unbound, i.e. it can't out-live driver unbind.

This guarantees that the &Device<Bound> instance you provide in the callbacks
below is guaranteed to be valid.

[1] https://lore.kernel.org/lkml/aFF7qqlexxh540FW@pollux/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/drm/driver.rs#n134

> +        // Get the raw C pointer from ARef<Chip>.
> +        let c_chip_ptr = chip.as_raw().cast::<bindings::pwm_chip>();
> +
> +        // SAFETY: `c_chip_ptr` is valid (guaranteed by ARef existing).
> +        // `ops_vtable.as_raw()` provides a valid `*const bindings::pwm_ops`.
> +        // `bindings::__pwmchip_add` preconditions (valid pointers, ops set on chip) are met.
> +        unsafe {
> +            (*c_chip_ptr).ops = ops_vtable.as_raw();
> +            to_result(bindings::__pwmchip_add(c_chip_ptr, core::ptr::null_mut()))?;
> +        }

Please split this up into separate unsafe blocks.

> +        Ok(Registration {
> +            chip: ManuallyDrop::new(chip),
> +        })
> +    }
> +}
> +
> +impl Drop for Registration {
> +    fn drop(&mut self) {
> +        let chip = &**self.chip;
> +        let chip_raw: *mut bindings::pwm_chip = chip.as_raw();
> +
> +        // SAFETY: `chip_raw` points to a chip that was successfully registered via `Self::new`.
> +        // `bindings::pwmchip_remove` is the correct C function to unregister it.
> +        unsafe {
> +            bindings::pwmchip_remove(chip_raw);
> +            ManuallyDrop::drop(&mut self.chip); // Drops the ARef<Chip>
> +        }

Same here, but I don't think ManuallyDrop is needed anyways.

> +    }
> +}