linux-kernel - Re: [PATCH 5/5] rust: device: Add a stub abstraction for devices

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: <ZAnB/DozWsir1cIE@kroah.com>
Date:   Thu, 9 Mar 2023 12:24:44 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Wedson Almeida Filho <wedsonaf@...il.com>
Cc:     Asahi Lina <lina@...hilina.net>, Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Joerg Roedel <joro@...tes.org>,
        Hector Martin <marcan@...can.st>,
        Sven Peter <sven@...npeter.dev>, Arnd Bergmann <arnd@...db.de>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Neal Gompa <neal@...pa.dev>, rust-for-linux@...r.kernel.org,
        linux-kernel@...r.kernel.org, asahi@...ts.linux.dev
Subject: Re: [PATCH 5/5] rust: device: Add a stub abstraction for devices

On Sun, Mar 05, 2023 at 03:39:25AM -0300, Wedson Almeida Filho wrote:
> > > +    /// Returns the name of the device.
> > > +    fn name(&self) -> &CStr {
> > > +        let ptr = self.raw_device();
> > > +
> > > +        // SAFETY: `ptr` is valid because `self` keeps it alive.
> > > +        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).
> > > +        unsafe { CStr::from_char_ptr(name) }
> >
> > Why can the device never be renamed?  Devices are renamed all the time,
> > sometimes when you least expect it (i.e. by userspace).  So how is this
> > considered "safe"? and actually correct?
> >
> > Again, maybe seeing a real user of this might make more sense, but
> > as-is, this feels wrong and not needed at all.
> 
> This requirement is to allow callers to use the string without having
> to make a copy of it.
> 
> If subsystems/buses are not following what the C documentation says,
> as you point out in another thread, we have a several options: (a)
> remove access to names altogether, (b) leave things as they are, then
> those subsystems wouldn't be able to honour the safety requirements of
> this trait therefore they wouldn't implement it, (c) make a copy of
> the string, etc.

How about we fix the documentation in the .c code and also drop this as
you really don't need it now.

Want to send a patch for the driver core code fix?

> > > +        // 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 {
> >
> > I am a rust newbie, but I don't understand this "RawDevice" here at all.
> 
> Different buses will have their own Rust "Device" type, for example,
> pci::Device, amba::Device, platform::Device that wrap their C
> counterparts pci_dev, amba_device, platform_device.
> 
> "RawDevice" is a trait for functionality that is common to all
> devices. It exposes the "struct device" of each bus/subsystem so that
> functions that work on any "struct device", for example, `clk_get`,
> `pr_info`. will automatically work on all subsystems.

Why is this being called "Raw" then?  Why not just "Device" to follow
along with the naming scheme that the kernel already uses?

> For example, as part writing Rust abstractions for a platform devices,
> we have a platform::Device type, which is wrapper around `struct
> platform_device`. It has a bunch of associated functions that do
> things that are specific to the platform bus. But then they also
> implement the `RawDevice` trait (by implementing `raw_device` that
> returns &pdev->dev), which allows drivers to call `clk_get` and the
> printing functions directly.
> 
> Let's say `pdev` is a platform device; if we wanted to call `clk_get`
> in C, we'd do something like:
> 
> clk = clk_get(&pdev->dev, NULL);
> 
> In Rust, we'd do:
> 
> clk = pdev.clk_get(None);
> 
> (Note that we didn't have to know that pdev had a field whose type is
> a `struct device` that we could use to call clk_get on; `RawDevice`
> encoded this information.)
> 
> Does the intent of the abstraction make sense to you now?

A bit more, yes.  But I want to see some real users before agreeing that
it is sane :)

thanks,

greg k-h