linux-kernel - Re: [PATCH v2] rust: platform: add Io support

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: <CAH5fLgiCgoUQLgZcd-w_PGjtFHYdFxsBFezoaFeyZXnBxY3O=A@mail.gmail.com>
Date: Fri, 15 Nov 2024 15:40:56 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: 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>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Trevor Gross <tmgross@...ch.edu>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Boris Brezillon <boris.brezillon@...labora.com>, 
	Rob Herring <robh@...nel.org>, Daniel Stone <daniels@...labora.com>
Subject: Re: [PATCH v2] rust: platform: add Io support

On Wed, Nov 13, 2024 at 8:43 PM Daniel Almeida
<daniel.almeida@...labora.com> wrote:
>
> Hi all, hopefully I did not butcher anything using b4 this time.
>
> See one question from me below.
>
> > On 13 Nov 2024, at 16:30, Daniel Almeida <daniel.almeida@...labora.com> wrote:
> > +    pub(crate) fn resource(&self, resource: u32) -> Result<*mut bindings::resource> {
> > +        // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
> > +        let resource = unsafe {
> > +            bindings::platform_get_resource(self.as_raw(), bindings::IORESOURCE_MEM, resource)
> > +        };
> > +        if !resource.is_null() {
> > +            Ok(resource)
> > +        } else {
> > +            Err(EINVAL)
> > +        }
> > +    }
> > }
>
> Should this return an immutable reference instead of a pointer?
>
> Once the pointer is known not to be null, I think it should. So far, this
> is the only place in the kernel crate calling `platform_get_resource`, so
> there would be no risk of breaking the reference rules and it would make
> the API more ergonomic to use.

You should never create references to bare C structs. If you want a
reference, you'll need to create a wrapper Rust type that uses Opaque
so that Rust's assumptions about references are not applied to the C
struct.

Since you're just passing it on to a C api that expects a raw pointer,
I would say keep the raw pointer. I wouldn't go for the wrapper struct
unless you expect this to be part of the API used by drivers.

Alice