Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] gsp: cmdq: drain() wedges queue on malformed RPC message - [High] gpu: nova-core: ring buffer desynchronization on malformed GSP messages - [Low] cmdq: stale comment regarding read pointer advancement on early return --
commit a033b6d3e9c45eced0baaf0fa5456db1072ad255 Author: Alexandre Courbot <[email protected]> Subject: gpu: nova-core: gsp: cmdq: split the transport part of the receive path This patch separates the transport and RPC layers in the GSP command queue receive path by introducing a MessageElement trait. It replaces receive_msg with a transport-agnostic consume_element which parses messages via the trait and advances the read pointer. > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs > b/drivers/gpu/nova-core/gsp/cmdq.rs > index a059ce74ba2da..65e1a0b6bb444 100644 > --- a/drivers/gpu/nova-core/gsp/cmdq.rs > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs > @@ -233,45 +233,45 @@ > +impl<M> MessageElement for RpcMessageElement<M> > +where > + M: MessageFromGsp, > + Error: From<M::InitError>, > +{ > + fn read(dev: &device::Device, message: GspMessage<'_>) -> Result<Self> { > + let rpc_message = RpcMessage::parse(dev, message)?; > + let function = rpc_message.header.function(); > + > + // An early return here would leave the read pointer on this message. > + let result = if matches!(function, Ok(f) if f == M::FUNCTION) { [Severity: Low] Is this comment still accurate under the new architecture? Under the new abstraction, CmdqInner::consume_element() executes M::read(), captures the result, and unconditionally advances the read pointer before returning the result: CmdqInner::consume_element() { ... let result = M::read(self.dev, message); self.gsp_mem.advance_cpu_read_ptr(elem_count); result } An early return inside RpcMessageElement::read() no longer leaves the read pointer on the message, making this comment misleading. [ ... ] > @@ -933,53 +981,19 @@ fn wait_for_msg(&self, timeout: Delta) -> > Result<GspMessage<'_>> { > [ ... ] > - fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M> > - where > - // This allows all error types, including `Infallible`, to be used > for `M::InitError`. > - Error: From<M::InitError>, > - { > + fn consume_element<M: MessageElement>(&mut self, timeout: Delta) -> > Result<M> { > let message = self.wait_for_msg(timeout)?; > let elem_count = > u32::try_from(message.header.msg_length().div_ceil(GSP_PAGE_SIZE))?; > - let rpc_message = RpcMessage::parse(self.dev, message)?; > - let function = rpc_message.header.function(); > - > - // An early return here would leave the read pointer on this message. > - let result = if matches!(function, Ok(f) if f == M::FUNCTION) { > - let (cmd, contents_1) = > - > M::Message::from_bytes_prefix(rpc_message.contents.0).ok_or(EIO)?; > [ ... ] > + let result = M::read(self.dev, message); > > // Advance the read pointer past this message. > + self.gsp_mem.advance_cpu_read_ptr(elem_count); > > result > } [Severity: High] Does this code introduce a regression where malformed GSP messages can desynchronize the ring buffer? The read pointer is unconditionally advanced by an unvalidated elem_count calculated from message.header.msg_length(). If the GSP sends a message with an rpc_length exceeding the queue's capacity, M::read() (via RpcMessage::parse()) correctly returns an error because the message is physically too large. However, because advance_cpu_read_ptr() is executed regardless of the error, the read pointer is advanced by the oversized, corrupted elem_count. Can this cause the read pointer to wrap around arbitrarily, permanently desynchronizing the ring buffer and breaking all further GSP communication? The previous code correctly bubbled up the error from RpcMessage::parse() before advancing the pointer. [ ... ] > @@ -1044,14 +1044,14 @@ fn drain(&mut self) -> Result { > while !self.gsp_mem.driver_read_area().0.is_empty() { > // A message is available, so this returns without waiting. > let msg = self.wait_for_msg(Delta::ZERO)?; > > let pages = > - > u32::try_from(msg.header.length().div_ceil(GSP_PAGE_SIZE)).map_err(|_| { > + > u32::try_from(msg.header.msg_length().div_ceil(GSP_PAGE_SIZE)).map_err(|_| { > dev_err!(&self.dev, "GSP drain: message length > overflow\n"); > EIO > })?; > - let function = msg.header.function(); > - let seq = msg.header.sequence(); > + > + RpcMessage::parse(self.dev, msg)?.log(self.dev); > > self.gsp_mem.advance_cpu_read_ptr(pages); > - self.log_event(function, seq); > } [Severity: High] Can this code permanently wedge the queue if RpcMessage::parse() fails? The commit message states that messages with unexpected sizes are consumed instead of remaining on the queue. However, if RpcMessage::parse() returns an error for a malformed message, the ? operator causes an early return from drain(). This skips self.gsp_mem.advance_cpu_read_ptr(pages), leaving the malformed message on the queue. Any subsequent calls to drain() would repeatedly fail on the same message, permanently wedging the queue. Verification at the series end commit (37073815d78fabf6293f38e4abb8f78d887d7841) confirms this issue is not fixed by subsequent patches. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
