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: <4e0be534-e3d7-4c74-b8f1-51bd869b18e4@nvidia.com>
Date: Thu, 30 Oct 2025 17:19:02 +0000
From: Zhi Wang <zhiw@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: "rust-for-linux@...r.kernel.org" <rust-for-linux@...r.kernel.org>,
	"dakr@...nel.org" <dakr@...nel.org>, "bhelgaas@...gle.com"
	<bhelgaas@...gle.com>, "kwilczynski@...nel.org" <kwilczynski@...nel.org>,
	"ojeda@...nel.org" <ojeda@...nel.org>, "alex.gaynor@...il.com"
	<alex.gaynor@...il.com>, "boqun.feng@...il.com" <boqun.feng@...il.com>,
	"gary@...yguo.net" <gary@...yguo.net>, "bjorn3_gh@...tonmail.com"
	<bjorn3_gh@...tonmail.com>, "lossin@...nel.org" <lossin@...nel.org>,
	"a.hindborg@...nel.org" <a.hindborg@...nel.org>, "aliceryhl@...gle.com"
	<aliceryhl@...gle.com>, "tmgross@...ch.edu" <tmgross@...ch.edu>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Neo Jia
	<cjia@...dia.com>, Surath Mitra <smitra@...dia.com>, Ankit Agrawal
	<ankita@...dia.com>, Aniket Agashe <aniketa@...dia.com>, Kirti Wankhede
	<kwankhede@...dia.com>, "Tarun Gupta (SW-GPU)" <targupta@...dia.com>,
	"zhiwang@...nel.org" <zhiwang@...nel.org>, Alex Williamson
	<alwilliamson@...dia.com>, Alexandre Courbot <acourbot@...dia.com>, Joel
 Fernandes <joelagnelf@...dia.com>, John Hubbard <jhubbard@...dia.com>
Subject: Re: [RFC 1/2] rust: introduce abstractions for fwctl

On 30.10.2025 18.22, 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.
> 

Will do this in the next re-spin. It needs extra abstraction around fwctl_device.

>> +        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?

fwctl_put() is a inline function and it seem opened by the compiler when bindings are
generated. 

$ grep -R fwctl_put *
$ pwd
/home/inno/drm-rust/rust/bindings
$ grep -R put_device *
bindings_generated.rs:    pub fn put_device(dev: *mut device);

Hehe. I am open to options.

Z.
> 
>> +            }
>> +            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?
> 
> Jason