Re: [PATCH 1/2] rust: hwmon: add hwmon abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] hwmon: use-after-free of hwdev->name due to unconstrained &CStr lifetime
- [High] hwmon: out-of-bounds read due to missing null-terminator in chip_config
- [High] hwmon: linker error for drivers omitting #[optional] chip_read/write
- [Medium] hwmon: missing hwmon_lock() API and incorrect locking documentation
- [Medium] hwmon: manual pointer wiring bypasses required pin-init API
- [Low] hwmon: abstraction limits drivers to a single temperature channel
- [Low] hwmon: missing #[inline] attribute on Registration drop method
--

commit a2d95b04547e27fa736c176ee2cb6a3c7255a3eb
Author: skxby <2374604267@qq.com>
Date:   Fri Oct 16 15:12:27 2026 +0800

rust: hwmon: add hwmon abstractions
    
Add Rust abstractions for the hwmon subsystem, supporting temp[1-*]
attributes and chip-level attributes. Registration owns its inner state
and uses a Driver trait for callbacks.

Link: https://lore.kernel.org/all/20260512155747.10136-1-donplat@barrensea.org/

> diff --git a/rust/kernel/hwmon.rs b/rust/kernel/hwmon.rs
> new file mode 100644
> index 0000000000000..d7e3fd683b862
> --- /dev/null
> +++ b/rust/kernel/hwmon.rs
[ ... ]
> +/// The hwmon driver trait.
> +///
> +/// # Concurrency and Driver Responsibilities
> +///
> +/// The hwmon C core provides **no internal locking** for attribute access. When user-space
                                     ^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
This isn't a bug, but is this statement accurate?

The documentation incorrectly claims that the 'hwmon C core provides no
internal locking for attribute access'. The C core uses a per-device mutex
(guard(mutex)(&hwdev->lock)) during sysfs attribute reads and writes.

Furthermore, the hwmon subsystem guidelines explicitly state that drivers
should use hwmon_lock() and hwmon_unlock() to implement locking required
for interrupt handling. This abstraction fails to expose or wrap these
functions, making it impossible for Rust drivers to comply with this
official rule.

> +/// reads or writes `/sys/class/hwmon/hwmon*/temp*_*`, the core directly invokes the
> +/// registered callbacks with a **shared `&self` reference** to `HwmonInner<T>`.
[ ... ]
> +    unsafe extern "C" fn read_callback(
> +        dev: *mut bindings::device,
> +        type_: u32,
> +        attr: u32,
> +        channel: c_int,
> +        val: *mut c_long,
> +    ) -> c_int {
> +        from_result(|| {
[ ... ]
> +            let result = match SensorType::try_from(type_)? {
> +                SensorType::Chip => T::chip_read(&inner.driver, ChipAttr::try_from(attr)?)?,
                                       ^^^^^^^^^^^
[Severity: High]
Does this code cause a compilation failure for drivers that do not implement
the optional chip methods?

The Driver trait marks chip_read and chip_write as #[optional]. The
#[vtable] macro replaces missing optional methods with a build_error!()
call. Here, the calls to T::chip_read and T::chip_write are unconditional
within the dynamically evaluated SensorType::Chip branch.

Because the compiler cannot prove this branch is dead at compile time, the
build_error!() call is retained in the binary, causing a fatal linker error
for any driver that chooses not to implement these methods.

Should the calls be guarded by if T::HAS_CHIP_READ and
if T::HAS_CHIP_WRITE?

> +                // C core guarantees `channel >= 0`.
> +                SensorType::Temp => {
> +                    T::read(&inner.driver, TempAttr::try_from(attr)?, channel as u32)?
> +                }
> +            };
[ ... ]
> +    fn new_placeholder(driver: T) -> Self {
> +        // A driver that exposes no chip attribute must not advertise the chip channel
> +        // either: leaving `chip_config` zeroed makes the channel carry no attributes, and
> +        // `is_visible` would not be consulted for it in the first place.
> +        let chip_config = if T::HAS_CHIP_IS_VISIBLE {
> +            [CHIP_ATTR_MASK]
             ^^^^^^^^^^^^^^^^
[Severity: High]
Can this out-of-bounds read adjacent memory when the C core reads the
configuration?

The C hwmon core strictly requires the hwmon_channel_info.config array to
be null-terminated (ending with a 0). Here, chip_config is defined as
[u32; 1] and initialized to [CHIP_ATTR_MASK]. It lacks a trailing 0
terminator.

When hwmon_device_register_with_info reads the channel configuration, it
will fail to stop at the end of chip_config and read past the array bounds
into adjacent memory, which can cause the C core to misinterpret memory as
attribute masks.

> +        } else {
> +            [0]
> +        };
> +
> +        Self {
> +            driver,
> +            ops: bindings::hwmon_ops {
> +                is_visible: Some(Adapter::<T>::is_visible_callback),
> +                visible: 0,
> +                read: Some(Adapter::<T>::read_callback),
> +                read_string: None,
> +                write: Some(Adapter::<T>::write_callback),
> +            },
> +            chip_config,
> +            temp_config: [TEMP_ATTR_MASK, 0],
                            ^^^^^^^^^^^^^^^^^
[Severity: Low]
Does this hardcoded array strictly limit the abstraction to a single
temperature channel?

The commit message asserts that the abstraction supports registering
temperature sensors with the full standard temp[1-*] attribute set.
However, this single configuration mask means the C core will only create
attributes for temp1_* and will never query channel > 0, preventing drivers
from registering multiple temperature sensors.

> +            chip_channel: bindings::hwmon_channel_info {
> +                type_: 0,
> +                config: core::ptr::null(),
> +            },
[ ... ]
> +    pub fn new(parent: &Device<Bound>, name: &CStr, data: T) -> Result<Self> {
> +        let mut boxed = KBox::new(HwmonInner::new_placeholder(data), GFP_KERNEL)?;
> +
> +        // Wire self-referential pointers before pinning. We obtain a mutable raw pointer
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Does this manual pointer wiring bypass the required pin-init API?

The Rust subsystem guidelines require that try_pin_init! or pin_init! be
used to initialize structs that require pinning. The abstraction bypasses
this safe pin-init framework by allocating a KBox, acquiring a raw pointer,
mutating fields unsafely to wire self-referential pointers, and manually
calling Pin::new_unchecked.

> +        // from the uniquely-owned `KBox` -- no intermediate `&mut` reference is created,
> +        // so no aliasing rules are violated.
> +        let inner_ptr: *mut HwmonInner<T> = &raw mut *boxed;
[ ... ]
> +        // SAFETY: `chip_info` and all nested pointers target memory within the same
> +        // allocation, which remains valid until `hwmon_device_unregister` in `Drop`.
> +        // `parent.as_raw()` and `name.as_char_ptr()` are valid. The name is copied by
> +        // the kernel (via `dev_set_name` -> `kvasprintf_const`), so no lifetime issue.
> +        let hwmon_dev = from_err_ptr(unsafe {
> +            bindings::hwmon_device_register_with_info(
> +                parent.as_raw(),
> +                name.as_char_ptr(),
                 ^^^^^^^^^^^^^^^^^^
[Severity: High]
Can this cause a use-after-free of hwdev->name?

The Registration::new function accepts a name: &CStr parameter without a
lifetime constraint tying it to the Registration object. The C core stores
this pointer directly in hwdev->name (which is returned by the sysfs
name_show attribute) without copying the string.

If a caller passes a temporary string (e.g., a short-lived CString), the
string will be freed after registration. When userspace later reads
/sys/class/hwmon/hwmon*/name, the C core will dereference the freed memory.

> +                drvdata,
> +                &raw const (*inner_ptr).chip_info,
> +                core::ptr::null_mut(),
> +            )
> +        })?;
> +
> +        Ok(Self { hwmon_dev, inner })
> +    }
> +}
> +
> +impl<T: Driver> Drop for Registration<T> {
> +    fn drop(&mut self) {
       ^^^^^^^
[Severity: Low]
This isn't a bug, but should this method have an #[inline] attribute?

According to the Rust subsystem guidelines, small functions or functions
forwarding to a C binding call in abstractions should be annotated with
#[inline]. This method forwards directly to bindings::hwmon_device_unregister
but lacks this required annotation.

> +        // SAFETY: `hwmon_dev` was returned by a successful registration. This call waits for
> +        // all in-flight callbacks before returning.
> +        unsafe { bindings::hwmon_device_unregister(self.hwmon_dev) };
> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_95E7F9B5CE294CA4027A0CD4C550B454050A@qq.com?part=1