Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Mar 26, 2024 at 05:01:53PM +0100, Danilo Krummrich wrote:
> On Mon, Mar 25, 2024 at 07:17:46PM +0100, Greg KH wrote:
> > On Mon, Mar 25, 2024 at 06:49:07PM +0100, Danilo Krummrich wrote:
> > > +        let name = unsafe { bindings::dev_name(ptr) };
> > > +
> > > +        // SAFETY: The name of the device remains valid while it is alive (because the device is
> > > +        // never renamed, per the safety requirement of this trait). This is guaranteed to be the
> > > +        // case because the reference to `self` outlives the one of the returned `CStr` (enforced
> > > +        // by the compiler because of their lifetimes).
> > 
> > devices are renamed all the time, I don't understand how this can be
> > true here.
> 
> As the comment says, it's the safety requirement of this trait, which is
> documented in the DOC comment of trait RawDevice. Admittedly, it's not obvious,
> since the comment isn't part of this diff and was introduced in the previous
> commit "rust: device: Add a minimal RawDevice trait". Maybe we should move it to
> this commit then.
> 
> It says: "Additionally, implementers must ensure that the device is never
> renamed. Commit a5462516aa99 ("driver-core: document restrictions on
> device_rename()") has details on why `device_rename` should not be used."

Yes, I agree it should not be used, but that's different from you saying
"this is safe as the device will never be renamed" which is not true
unless you somehow prevent that from ever happening on a pointer that
the rust code gets here.  How can you do that?

And why the issue if the device is renamed (other than the documented
ones), why is the rust code here special in this way?

> > > +
> > > +impl Device {
> > > +    /// Creates a new device instance.
> > > +    ///
> > > +    /// # Safety
> > > +    ///
> > > +    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
> > 
> > device pointers are NULL all the time, you better be able to handle this
> > otherwise it's not going to go well :(
> 
> You mean as in check for NULL and return an error in case? I wouldn't see much
> value in that.
> 
> I think the only places where we call this are the ones where the C side already
> guarantees that the raw device pointer is valid, e.g. probe() or remove()
> callbacks.

If that's true, then that's fine, but say that, and perhaps test for it
if you are going to guarantee it.  Otherwise we have many places in the
kernel where device pointers are NULL and code needs to handle it in
general when dealing with device pointers for other types of usages.

> > > +    pub unsafe fn new(ptr: *mut bindings::device) -> Self {
> > > +        // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented.
> > > +        unsafe { bindings::get_device(ptr) };
> > > +        // INVARIANT: The safety requirements satisfy all but one invariant, which is that `self`
> > > +        // owns a reference. This is satisfied by the call to `get_device` above.
> > > +        Self { ptr }
> > > +    }
> > > +
> > > +    /// Creates a new device instance from an existing [`RawDevice`] instance.
> > > +    pub fn from_dev(dev: &dyn RawDevice) -> Self {
> > > +        // SAFETY: The requirements are satisfied by the existence of `RawDevice` and its safety
> > > +        // requirements.
> > > +        unsafe { Self::new(dev.raw_device()) }
> > > +    }
> > > +}
> > > +
> > > +// SAFETY: The device returned by `raw_device` is the one for which we hold a reference.
> > > +unsafe impl RawDevice for Device {
> > > +    fn raw_device(&self) -> *mut bindings::device {
> > > +        self.ptr
> > > +    }
> > > +}
> > > +
> > > +impl Drop for Device {
> > > +    fn drop(&mut self) {
> > > +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> > > +        // relinquish it now.
> > > +        unsafe { bindings::put_device(self.ptr) };
> > > +    }
> > > +}
> > > +
> > > +impl Clone for Device {
> > > +    fn clone(&self) -> Self {
> > > +        Self::from_dev(self)
> > 
> > Does this increment the reference count?
> 
> Yes, it does. from_dev() calls new() and new() calls get_device() again.

Ok, but why would you ever allow a device structure to be cloned?
That's not something that should ever happen, once it is created, it is
the ONLY instance of that structure around, and it can't be changed, as
you passed it off to be handled by the driver core, and the driver core
is the one that handles the lifetime of the object, NOT the bus or the
driver that happens to bind to it.

I've been worried about how the interaction between the driver core and
rust code is going to handle structure lifetime rules.  You need to be
very careful here with what you do.  Rust code can create these
structures, but once they are passed to the driver core, you lose the
ability to control them and have to allow the driver core to manage
them, as that's its job, not the rust code.

thanks,

greg k-h