linux-kernel - Re: [PATCH v2 05/10] gpu: nova-core: gsp: Add GSP command queue handling

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <mkecw5p2eb6bsl54ccpxrdezeatr4sxjtkvsteu4klx6u3ldka@p42jqjvoi275>
Date: Mon, 29 Sep 2025 11:06:27 +1000
From: Alistair Popple <apopple@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Lyude Paul <lyude@...hat.com>, rust-for-linux@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, dakr@...nel.org, 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 <lossin@...nel.org>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, 
	Trevor Gross <tmgross@...ch.edu>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	John Hubbard <jhubbard@...dia.com>, Joel Fernandes <joelagnelf@...dia.com>, 
	Timur Tabi <ttabi@...dia.com>, linux-kernel@...r.kernel.org, nouveau@...ts.freedesktop.org
Subject: Re: [PATCH v2 05/10] gpu: nova-core: gsp: Add GSP command queue
 handling

On 2025-09-26 at 12:20 +1000, Alexandre Courbot <acourbot@...dia.com> wrote...
> On Thu Sep 25, 2025 at 3:32 PM JST, Alistair Popple wrote:
> <snip>
> >> > +    #[expect(unused)]
> >> > +    pub(crate) fn receive_msg_from_gsp<M: GspMessageFromGsp, R>(
> >> > +        &mut self,
> >> > +        timeout: Delta,
> >> > +        init: impl FnOnce(&M, SBuffer<core::array::IntoIter<&[u8], 2>>) -> Result<R>,
> >> > +    ) -> Result<R> {
> >> > +        let (driver_area, msg_header, slice_1) = wait_on(timeout, || {
> >> > +            let driver_area = self.gsp_mem.driver_read_area();
> >> > +            // TODO: find an alternative to as_flattened()
> >> > +            #[allow(clippy::incompatible_msrv)]
> >> > +            let (msg_header_slice, slice_1) = driver_area
> >> > +                .0
> >> > +                .as_flattened()
> >> > +                .split_at(size_of::<GspMsgElement>());
> >> > +
> >> > +            // Can't fail because msg_slice will always be
> >> > +            // size_of::<GspMsgElement>() bytes long by the above split.
> >> > +            let msg_header = GspMsgElement::from_bytes(msg_header_slice).unwrap();
> >> 
> >> Any reason we're not just using unwrap_unchecked() here then?
> >
> > Because whilst my assertions about the code are currently correct if it ever
> > changes I figured it would be better to explicitly panic than end up with
> > undefined behaviour. Is there some other advantage to using unwrap_unchecked()?
> > I can't imagine there'd be much of a performance difference.
> 
> Here I think we should just use the `?` operator. The function already
> returns a `Result` so it would fit.

Actually note quite true - this is in a closure that must return `Option<_>`
so returning `Result` doesn't fit. However it still fits because I just noticed
`::from_bytes()` returns an `Option` so `?` will still work.

> I'd be willing to consider unwrapping is this can prevent an
> obviously-unfallible method from having to return a `Result` - but here
> this is not the case, and handling the error doesn't cost us more
> than the `unwrap`, so let's do that.

Agreed. I assumed from_bytes() returned `Result<_>` which would not have worked
rather than `Option<_>` which will though.

> <snip>
> >> > +impl GspRpcHeader {
> >> > +    pub(crate) fn new(cmd_size: u32, function: u32) -> Self {
> >> > +        Self {
> >> > +            // TODO: magic number
> >> > +            header_version: 0x03000000,
> >> > +            signature: bindings::NV_VGPU_MSG_SIGNATURE_VALID,
> >> > +            function,
> >> > +            // TODO: overflow check?
> >> > +            length: size_of::<Self>() as u32 + cmd_size,
> >> 
> >> (just curious, do you mean overflow as in arith overflow or overflow as in
> >> going past the boundaries of the header?)
> >
> > Actually this snuck in from some of Alex's suggested code improvements (I had
> > intended to credit him in the commit message! Will fix that) so maybe he can
> > answer what he had in mind? I assumed arith overflow but maybe he meant ring
> > buffer overflow or something.
> 
> I was thinking about arithmetic overflow, but maybe that was just
> overthinking. :) We're probably not going to send a 4 GB payload anytime
> soon...

Lets hope not :) I guess we might want `checked_add()` to panic if we've gone
insane though so have done that.