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

[Danilo Krummrich <dakr@redhat.com>]

On Tue, Mar 26, 2024 at 07:03:47PM +0100, Greg KH wrote:
> 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

You omit the restriction "implementers must ensure that the device is never
renamed".

> unless you somehow prevent that from ever happening on a pointer that
> the rust code gets here.  How can you do that?

By setting the above requirement for the subsystem implementing the RawDevice
trait.

Do we really need to expect the device name to change at any (random) point of
time? Isn't the subsystem in control of that?

The alternative would be to always return a copy of the device name when
RawDevice::name() is called.

Besides all that, aren't we in trouble on the C side as well?

devm_request_threaded_irq(), for instance, just picks up the pointer returned by
dev_name(dev), which is stored in struct irqaction and e.g. subsequently
accessed by the irq_handler_entry trace event.

Another example would be ehci_platform_probe() where in a subsequent
usb_create_hcd() call kobj->name (or dev->init_name) is assigned to
hcd->self.bus_name. In contrast to the example above, I guess the USB subsystem
takes care to never rename the device. Isn't this assumption identical to the
requirement of trait RawDevice?

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

I think Rust isn't special here. Isn't the old name freed in kobject_rename()?
That'd be a problem for the current Rust code and it'd be a problem for the
examples I gave above, right?