Re: [RFC 1/2] rust: introduce abstractions for fwctl

["Danilo Krummrich" <dakr@kernel.org>]

On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> +/// Static vtable mapping Rust trait methods to C callbacks.
> +pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
> +
> +impl<T: FwCtlOps> FwCtlVTable<T> {
> +    /// Static instance of `fwctl_ops` used by the C core to call into Rust.
> +    pub const VTABLE: bindings::fwctl_ops = bindings::fwctl_ops {
> +        device_type: T::DEVICE_TYPE,
> +        uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),

The fwctl code uses this size to allocate memory for both, struct fwctl_uctx and
the driver's private data at the end of the allocation.

This means that it is not enough to just consider the size of T::UCtx, you also
have to consider its required alignment, and, if required, allocate more memory.

> +        open_uctx: Some(Self::open_uctx_callback),
> +        close_uctx: Some(Self::close_uctx_callback),
> +        info: Some(Self::info_callback),
> +        fw_rpc: Some(Self::fw_rpc_callback),
> +    };
> +
> +    /// Called when a new user context is opened by userspace.
> +    unsafe extern "C" fn open_uctx_callback(uctx: *mut bindings::fwctl_uctx) -> ffi::c_int {
> +        // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
> +        let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };

Considering the above, this is incorrect for two reasons.

  (1) FwCtlUCtx::uctx might not be aligned correctly.

  (2) FwCtlUCtx::uctx is not initialized, hence creating a reference might be
      undefined behavior.

I think the correct way to fix (2) is to only provide an abstraction of struct
fwctl_uctx as argument to T::open_uctx() and let T::open_uctx() return an
initializer for FwCtlUCtx::uctx, i.e. impl PinInit<T::UCtx, Error>.

All other callbacks should be correct as they are once the alignment is
considered.

> +        match T::open_uctx(ctx) {
> +            Ok(()) => 0,
> +            Err(e) => e.to_errno(),
> +        }
> +    }