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: <Y/jmfR47ZdepTyf7@kroah.com>
Date:   Fri, 24 Feb 2023 17:31:57 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Asahi Lina <lina@...hilina.net>
Cc:     Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...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 Sat, Feb 25, 2023 at 12:51:38AM +0900, Asahi Lina wrote:
> On 25/02/2023 00.34, Greg Kroah-Hartman wrote:
> > On Sat, Feb 25, 2023 at 12:10:47AM +0900, Asahi Lina wrote:
> >>>> +impl Device {
> >>>> +    /// Creates a new device instance.
> >>>> +    ///
> >>>> +    /// # Safety
> >>>> +    ///
> >>>> +    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
> >>>> +    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) };
> >>>
> >>> You don't check the return value of get_device()?  What if it failed
> >>> (hint, it can)?
> >>
> >> Really? I looked at the implementation and I don't see how it can fail,
> >> as long as the argument is non-NULL and valid... (which is a
> >> precondition of this unsafe function). Did I miss something?
> > 
> > This function has changed a bit over time, but yes, it could fail if
> > someone else just dropped the last reference right before you tried to
> > grab a new one (look at the implementation of kobject_get()).
> > 
> > Yes, if you "know" you have a non-zero reference count first, it should
> > never fail, but how do you know this?  We have the ability to check the
> > return value of the function, shouldn't that happen?
> 
> Well, we know this because it is part of the invariant. If we don't
> uphold invariants, things fall apart... that's why we create safe
> abstractions that don't let you break those invariants after all!
> 
> In this particular case though, I don't see what we can usefully do
> here. `device_get()` is going to be part of Clone impls and things like
> that which are non-fallible. At most we can assert!() and rust-panic
> (which is a BUG as far as I know) if it fails... would that be preferable?

That's a good question, I don't know.  In thinking about it some more,
I think we are ok with this as-is for now.

BUT, I want to see any code that is actually using a struct device, and
I really want to review the heck out of the platform device api that you
must have somewhere, as that might show some other issues around this
type of thing that might be lurking.

thanks,

greg k-h