Message-ID: <aDMDrT7WIDc-yD0p@pollux.localdomain>
Date: Sun, 25 May 2025 13:49:01 +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>,
	Benno Lossin <benno.lossin@...ton.me>,
	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>,
	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 RFC 1/6] rust: Add basic PWM abstractions

Hi Michal,

On Sat, May 24, 2025 at 11:14:55PM +0200, Michal Wilczynski wrote:
> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..357fda46faa99c4462149658951ec53bf9cc2d1e
> --- /dev/null
> +++ b/rust/kernel/pwm.rs
> @@ -0,0 +1,376 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@...sung.com>
> +
> +//! PWM (Pulse Width Modulator) abstractions.
> +//!
> +//! This module provides safe Rust abstractions for working with the Linux
> +//! kernel's PWM subsystem, leveraging types generated by `bindgen`
> +//! from `<linux/pwm.h>` and `drivers/pwm/core.c`.
> +
> +use crate::{
> +    bindings,
> +    device::Device as CoreDevice,

I recommend referring the the base device as just device::Device, like we do it
everywhere else where we have this name conflict.

I'm not a huge fan of such aliases, since it's confusing when looking at patches
that do not have the alias as context later on.

> +    error::*,
> +    prelude::*,
> +    str::CStr,
> +    types::{ForeignOwnable, Opaque},
> +};
> +use core::marker::PhantomData;
> +
> +/// PWM polarity. Mirrors `enum pwm_polarity`.
> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
> +pub enum Polarity {
> +    /// Normal polarity (duty cycle defines the high period of the signal)
> +    Normal,
> +    /// Inversed polarity (duty cycle defines the low period of the signal)
> +    Inversed,
> +}
> +
> +impl From<bindings::pwm_polarity> for Polarity {
> +    fn from(polarity: bindings::pwm_polarity) -> Self {
> +        match polarity {
> +            bindings::pwm_polarity_PWM_POLARITY_NORMAL => Polarity::Normal,
> +            bindings::pwm_polarity_PWM_POLARITY_INVERSED => Polarity::Inversed,
> +            _ => {
> +                pr_warn!(
> +                    "Unknown pwm_polarity value {}, defaulting to Normal\n",
> +                    polarity
> +                );
> +                Polarity::Normal

Either Polarity::Normal is the correct thing to return in such a case, but then
we do not need the pr_warn!(), or it is not, but then this should be a TryFrom
impl instead.

> +            }
> +        }
> +    }
> +}
> +
> +impl From<Polarity> for bindings::pwm_polarity {
> +    fn from(polarity: Polarity) -> Self {
> +        match polarity {
> +            Polarity::Normal => bindings::pwm_polarity_PWM_POLARITY_NORMAL,
> +            Polarity::Inversed => bindings::pwm_polarity_PWM_POLARITY_INVERSED,
> +        }
> +    }
> +}
> +
> +/// Wrapper for board-dependent PWM arguments (`struct pwm_args`).
> +#[repr(transparent)]
> +pub struct Args(Opaque<bindings::pwm_args>);
> +
> +impl Args {
> +    /// Creates an `Args` wrapper from the C struct reference.
> +    fn from_c_ref(c_args: &bindings::pwm_args) -> Self {

I'd make this a pointer type instead, rather than having conversion to a
reference at the caller's side.

> +        // SAFETY: Pointer is valid, construct Opaque wrapper. We copy the data.
> +        Args(Opaque::new(*c_args))
> +    }
> +
> +    /// Returns the period of the PWM signal in nanoseconds.
> +    pub fn period(&self) -> u64 {
> +        // SAFETY: Reading from the valid pointer obtained by `get()`.

Here and below, you should explain why this pointer is guaranteed to be valid
instead.

> +        unsafe { (*self.0.get()).period }
> +    }
> +
> +    /// Returns the polarity of the PWM signal.
> +    pub fn polarity(&self) -> Polarity {
> +        // SAFETY: Reading from the valid pointer obtained by `get()`.
> +        Polarity::from(unsafe { (*self.0.get()).polarity })
> +    }
> +}
> +
> +/// Wrapper for PWM state (`struct pwm_state`).
> +#[repr(transparent)]
> +pub struct State(Opaque<bindings::pwm_state>);
> +
> +impl State {
> +    /// Creates a new zeroed `State`.
> +    pub fn new() -> Self {
> +        State(Opaque::new(bindings::pwm_state::default()))
> +    }
> +
> +    /// Creates a `State` wrapper around a copy of a C `pwm_state`.
> +    pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
> +        State(Opaque::new(c_state))
> +    }

You probably don't need Opaque here, given that you have instances of
bindings::pwm_state already.

> +
> +    /// Creates a `State` wrapper around a reference to a C `pwm_state`.
> +    fn from_c_ref(c_state: &bindings::pwm_state) -> &Self {
> +        // SAFETY: Pointer is valid, lifetime tied to input ref. Cast pointer type.
> +        unsafe { &*(c_state as *const bindings::pwm_state as *const Self) }
> +    }
> +
> +    /// Gets the period of the PWM signal in nanoseconds.
> +    pub fn period(&self) -> u64 {
> +        unsafe { (*self.0.get()).period }

This and all the methods below lack SAFETY comments, did you compile with
CLIPPY=1? You should get lots of warnings reminding you to add them.

<snip>

> +
> +/// Wrapper for a PWM device/channel (`struct pwm_device`).
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::pwm_device>);
> +
> +impl Device {
> +    pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_device) -> &'a mut Self {

We usually call this kind of function as_ref(), it'd be nice to stick to that
for consistency.

> +        unsafe { &mut *ptr.cast::<Self>() }

A mutable reference -- why?

<snip>

> +/// Wrapper for a PWM chip/controller (`struct pwm_chip`).
> +#[repr(transparent)]
> +pub struct Chip(Opaque<bindings::pwm_chip>);
> +
> +impl Chip {
> +    /// Creates a `Chip` reference from a raw pointer. (Safety notes apply)
> +    pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_chip) -> &'a mut Self {

Same here, for the name...

> +        unsafe { &mut *ptr.cast::<Self>() }

and the mutability.

> +    }
> +
> +    /// Returns a raw pointer to the underlying `pwm_chip`.
> +    pub(crate) fn as_ptr(&self) -> *mut bindings::pwm_chip {
> +        self.0.get()
> +    }
> +
> +    /// Gets the number of PWM channels (hardware PWMs) on this chip.
> +    pub fn npwm(&self) -> u32 {
> +        unsafe { (*self.as_ptr()).npwm }
> +    }
> +
> +    /// Returns `true` if the chip supports atomic operations for configuration.
> +    pub fn is_atomic(&self) -> bool {
> +        unsafe { (*self.as_ptr()).atomic }
> +    }
> +
> +    /// Returns a reference to the embedded `struct device` abstraction (`CoreDevice`).
> +    pub fn device(&self) -> &CoreDevice {
> +        // SAFETY: `dev` field exists and points to the embedded device.
> +        let dev_ptr = unsafe { &(*self.as_ptr()).dev as *const _ as *mut bindings::device };
> +        unsafe { &*(dev_ptr as *mut CoreDevice) }

Missing SAFETY comment, also please use cast() and cast_{const,mut}() instead.

> +    }
> +
> +    /// Returns a reference to the parent device (`struct device`) of this PWM chip's device.
> +    pub fn parent_device(&self) -> Option<&CoreDevice> {
> +        // SAFETY: Accessing fields via assumed-valid pointer and bindgen layout.
> +        let parent_ptr = unsafe { bindings::pwmchip_parent(self.as_ptr()) };
> +        if parent_ptr.is_null() {
> +            None
> +        } else {
> +            // SAFETY: Pointer is non-null, assume valid device managed by kernel.
> +            Some(unsafe { &*(parent_ptr as *mut CoreDevice) })
> +        }

This can just be

	self.device().parent() // [1]

which lands through the DRM tree in the upcoming merge window.

[1] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/device.rs?ref_type=heads#L72

> +    /// Gets the *typed* driver-specific data associated with this chip's embedded device.
> +    pub fn get_drvdata<T: 'static>(&self) -> Option<&T> {

I will soon send a patch series that adds drvdata accessors to the generic
Device abstraction.

Anyways, no need for you to wait for this.

> +        let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_ptr()) };

Obviously, the C side calls this pwmchip_get_drvdata() and dev_get_drvdata(),
however, I suggest not to use get_drvdata() as a method name, since 'get' is
used in the context of reference counting (get/put) instead, and hence may be
confusing. Let's just name this drvdata().

> +        if ptr.is_null() {
> +            None
> +        } else {
> +            unsafe { Some(&*(ptr as *const T)) }
> +        }
> +    }
> +
> +    /// Sets the *typed* driver-specific data associated with this chip's embedded device.
> +    pub fn set_drvdata<T: 'static + ForeignOwnable>(&mut self, data: T) {
> +        unsafe { bindings::pwmchip_set_drvdata(self.as_ptr(), data.into_foreign() as _) }
> +    }
> +}
> +
> +/// Allocates a PWM chip structure using device resource management. Mirrors `devm_pwmchip_alloc`.
> +pub fn devm_chip_alloc<'a>(

This should be a function of pwm::Chip rather than standalone.

> +    parent: &'a CoreDevice,

Since you're using devres, this must be a bound device, i.e. the parameter must
be &'a device::Device<device::Bound>, see also [2], which lands in the upcoming
merge window through the driver-core tree.

But I think this shouldn't use devm_pwmchip_alloc() anyways, see below.

[2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/rust/kernel/device.rs?h=driver-core-next#n237

> +    npwm: u32,
> +    sizeof_priv: usize,
> +) -> Result<&'a mut Chip> {

Besides the above, this solution seems a bit half-baked, since it means that you
can only ever access the pwm::Chip as long as you have the &Device<Bound>,
which, given that you call this function typically from probe(), not beyond the
scope of probe().

This is because you return a reference which you can't save in the driver's
private data.

Instead you should use pwmchip_alloc() instead and make this return a real
instance of pwm::Chip and implement AlwaysRefCounted [3] for pwm::Chip.

You can then store the pwm::Chip instance in the Driver's private data, which
will only live until the driver is unbound, so it gives the same guarantees.

[3] https://rust.docs.kernel.org/kernel/types/trait.AlwaysRefCounted.html

> +    // SAFETY: `devm_pwmchip_alloc` called with valid args. Returns valid ptr or ERR_PTR.
> +    let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
> +    let chip_ptr = unsafe { bindings::devm_pwmchip_alloc(parent_ptr, npwm, sizeof_priv) };
> +    if unsafe { bindings::IS_ERR(chip_ptr as *const core::ffi::c_void) } {
> +        let err = unsafe { bindings::PTR_ERR(chip_ptr as *const core::ffi::c_void) };
> +        pr_err!("devm_pwmchip_alloc failed: {}\n", err);

You have the parent device, please use dev_err!(). But I don't think this needs
to error print at all.

> +        Err(Error::from_errno(err as i32))
> +    } else {
> +        // SAFETY: `chip_ptr` valid, lifetime managed by `devm` tied to `parent`.
> +        Ok(unsafe { &mut *(chip_ptr as *mut Chip) })
> +    }
> +}
> +
> +/// Registers a PWM chip with the PWM subsystem. Mirrors `__pwmchip_add`.
> +pub fn chip_add(chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
> +    // SAFETY: Pointers are valid. `__pwmchip_add` requires ops to be set.
> +    unsafe {
> +        let chip_ptr = chip.as_ptr();
> +        // Assign the ops pointer directly to the C struct field
> +        (*chip_ptr).ops = ops.as_ptr();
> +        to_result(bindings::__pwmchip_add(
> +            chip_ptr,
> +            core::ptr::null_mut()
> +        ))
> +    }
> +}

How do you ensure to unregister the chip, once it was registered through this
function? I think this can cause UAF bugs. Instead you should wrap this in a
'Registration' structure, like we do everywhere else, see for example [4].

The structure should look like this:

	pub struct Registration(ARef<Chip>);

Registration::new() should register the chip and Registration::drop() should
unregister the chip.

[4] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/drm/driver.rs?ref_type=heads#L121

> +/// Registers a PWM chip using device resource management. Mirrors `__devm_pwmchip_add`.
> +pub fn devm_chip_add(parent: &CoreDevice, chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
> +    // SAFETY: Pointers are valid. `__devm_pwmchip_add` requires ops to be set.
> +    unsafe {
> +        let chip_ptr = chip.as_ptr();
> +        // Assign the ops pointer directly to the C struct field
> +        (*chip_ptr).ops = ops.as_ptr();
> +        let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
> +        to_result(bindings::__devm_pwmchip_add(
> +            parent_ptr,
> +            chip_ptr,
> +            core::ptr::null_mut()
> +        ))
> +    }
> +}

This won't work anymore when creating a real pwm::Chip instance, since you can't
guarantee that the pwm::Chip still exists when devres will clean this up.

If you want devres to clean this up, you should make Registration::new() return
a Result<Devres<Registration>> instance.

This way the Registration keeps a reference to the pwm::Chip (giving the
guarantee of no potential UAF), and the Devres container ensures to drop the
Registration when the parent device is unbound internally.

If you want the exact same semantics as your current devm_chip_add(), but
without potential UAF, there is also Devres::new_foreign_owned() [5]. This way
Registration::new() will only return a Result.

[5] https://rust.docs.kernel.org/kernel/devres/struct.Devres.html#method.new_foreign_owned

Powered by blists - more mailing lists