Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

[Danilo Krummrich <dakr@kernel.org>]

On Fri, May 30, 2025 at 07:35:56PM +0200, Christian Schrefl wrote:
> On 30.05.25 4:24 PM, Danilo Krummrich wrote:

<snip>

> > +#[pinned_drop]
> > +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<T> {
> > +    fn drop(self: Pin<&mut Self>) {
> > +        // SAFETY: We know that the device is registered by the type invariants.
> > +        unsafe { bindings::misc_deregister(self.inner.get()) };
> > +    }> +}

<snip>

> >  #[pinned_drop]
> >  impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
> >      fn drop(self: Pin<&mut Self>) {
> > -        // SAFETY: We know that the device is registered by the type invariants.
> > -        unsafe { bindings::misc_deregister(self.inner.get()) };
> > -
> >          // SAFETY: `self.data` is valid for dropping.
> >          unsafe { core::ptr::drop_in_place(self.data.get()) };
> 
> I think this can race for a use after free.
> The data gets freed in this `Drop` impl but the `miscdevice_deregister` call will only
> happen in the `DeviceRegistrationInner` `Drop` implementatation, since the fields 
> will only be dropped after the `drop` function has executed.

Yes and no. :-) The fun part is that the drop order actually depends on whether
we have a parent device and use Devres or whether we have no parent device.

In the first case the drop order is correct by chance, due to Devres revoking
things when the parent device in unbound, which happens before, since the faux
device is dropped first.

But in general you're right, this needs to be fixed.

> Either  inner: DeviceRegistrationInner<T> needs to be wrapped in a ManuallyDrop and
> dropped manually,
> or the Data needs to be wrapped in a type that will automatically drop it (this would
> be fine with `UnsafePinned`).

There's also option 3), which is moving the drop_in_place() for the registration
data into RawDeviceRegistration::drop(), right below misc_deregister(), until we
have `UnsafePinned`.

(This is fine, since RawDeviceRegistration already has (and requires) a pointer
to the registration data and hence is always embedded in a
MiscDeviceRegistration.)