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

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: <DDVTVQ54W7FM.1XS6MIH4ALW8U@kernel.org>
Date: Thu, 30 Oct 2025 18:21:06 +0100
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Jason Gunthorpe" <jgg@...dia.com>
Cc: "Zhi Wang" <zhiw@...dia.com>, <rust-for-linux@...r.kernel.org>,
 <bhelgaas@...gle.com>, <kwilczynski@...nel.org>, <ojeda@...nel.org>,
 <alex.gaynor@...il.com>, <boqun.feng@...il.com>, <gary@...yguo.net>,
 <bjorn3_gh@...tonmail.com>, <lossin@...nel.org>, <a.hindborg@...nel.org>,
 <aliceryhl@...gle.com>, <tmgross@...ch.edu>,
 <linux-kernel@...r.kernel.org>, <cjia@...dia.com>, <smitra@...dia.com>,
 <ankita@...dia.com>, <aniketa@...dia.com>, <kwankhede@...dia.com>,
 <targupta@...dia.com>, <zhiwang@...nel.org>, <alwilliamson@...dia.com>,
 <acourbot@...dia.com>, <joelagnelf@...dia.com>, <jhubbard@...dia.com>
Subject: Re: [RFC 1/2] rust: introduce abstractions for fwctl

On Thu Oct 30, 2025 at 5:22 PM CET, Jason Gunthorpe wrote:
> On Thu, Oct 30, 2025 at 04:03:12PM +0000, Zhi Wang wrote:
>> +impl<T: FwCtlOps> Registration<T> {
>> +    /// Allocate and register a new fwctl device under the given parent device.
>> +    pub fn new(parent: &device::Device) -> Result<Self> {
>> +        let ops = &FwCtlVTable::<T>::VTABLE as *const _ as *mut _;
>> +
>> +        // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
>> +        // and initializes its embedded `struct device`.
>> +        let dev = unsafe {
>> +            bindings::_fwctl_alloc_device(
>> +                parent.as_raw(),
>> +                ops,
>> +                core::mem::size_of::<bindings::fwctl_device>(),
>> +            )
>> +        };
>> +
>> +        let dev = NonNull::new(dev).ok_or(ENOMEM)?;
>> +
>> +        // SAFETY: `fwctl_register()` expects a valid device from `_fwctl_alloc_device()`.
>> +        let ret = unsafe { bindings::fwctl_register(dev.as_ptr()) };
>
> This is a Bound device, not just any device.

Indeed, the safety comment should mention this. And if it would it could not
justify it with the current code, since the function takes a &Device instead of
the required &Device<Bound> argument.

>> +        if ret != 0 {
>> +            // SAFETY: If registration fails, release the allocated fwctl_device().
>> +            unsafe {
>> +                bindings::put_device(core::ptr::addr_of_mut!((*dev.as_ptr()).dev));
>
> ?? Don't open code fwctl_put() - it should be called directly?
>
>> +            }
>> +            return Err(Error::from_errno(ret));
>> +        }
>> +
>> +        Ok(Self {
>> +            fwctl_dev: dev,
>> +            _marker: PhantomData,
>> +        })
>> +    }
>> +
>> +    fn as_raw(&self) -> *mut bindings::fwctl_device {
>> +        self.fwctl_dev.as_ptr()
>> +    }
>> +}
>> +
>> +impl<T: FwCtlOps> Drop for Registration<T> {
>> +    fn drop(&mut self) {
>> +        // SAFETY: `fwctl_unregister()` expects a valid device from `_fwctl_alloc_device()`.
>
> Incomplete safety statement, the device passed to fwctl_alloc_device must
> still be bound prior to calling fwctl_unregister
>
>> +        unsafe {
>> +            bindings::fwctl_unregister(self.as_raw());
>> +            bindings::put_device(core::ptr::addr_of_mut!((*self.as_raw()).dev));
>
> There for Drop can only do fwctl_put() since otherwise there is no way
> to guarantee a Bound device.
>
> unregister has to happen before remove() completes, Danilo had some
> approach to this I think he told me?

Yeah, such Registration structures of (class) devices should be wrapped into a
Devres container (i.e. Devres<fwctl::Registration>)to be able to provide this
guarantee. See also my other reply to this patch [1].

While not a class device, the auxiliary bus with its auxiliary::Registration
[2], is a good example.

Alternatively (or additionally), it can also be implemented in a way that the
driver does not get control over a Registration object at all, but once created
it is not accessible anymore and automatically dropped on parent device unbind.
This approach is used by cpufreq [3].

It always depends on whether a driver might want to drop the Registration
manually before device unbind.

[1] https://lore.kernel.org/lkml/DDVT5YA564C6.3HN9WCMQX49PC@kernel.org/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/rust/kernel/auxiliary.rs?id=b0b7301b004301afe920b3d08caa6171dd3f4011#n304
[3] https://rust.docs.kernel.org/kernel/cpufreq/struct.Registration.html#method.new_foreign_owned