1. Patchew
  2. linux
  3. [PATCH v2 0/4] gpu: nova-core: gsp: fix command queue ring buffer bugs
  4. View patch

[PATCH v2 4/4] gpu: nova-core: gsp: fix improper indexing in driver_read_area

Eliot Courtney posted 4 patches 2 weeks ago
There is a newer version of this series
[PATCH v2 4/4] gpu: nova-core: gsp: fix improper indexing in driver_read_area
Posted by Eliot Courtney 2 weeks ago 
The current code indexes into `after_rx` using `tx` which is an index
for the whole buffer, not the split buffer `after_rx`.

Also add more rigorous no-panic proofs.

Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/gsp/cmdq.rs | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index aa8758fc7723..c26396fda29c 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
 use core::{
-    cmp,
     mem,
     sync::atomic::{
         fence,
@@ -267,10 +266,20 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
         // PANIC: per the invariant of `cpu_read_ptr`, `rx` is `< MSGQ_NUM_PAGES`.
         let (before_rx, after_rx) = gsp_mem.gspq.msgq.data.split_at(rx);
 
-        match tx.cmp(&rx) {
-            cmp::Ordering::Equal => (&after_rx[0..0], &after_rx[0..0]),
-            cmp::Ordering::Greater => (&after_rx[..tx], &before_rx[0..0]),
-            cmp::Ordering::Less => (after_rx, &before_rx[..tx]),
+        // The area starting at `rx` and ending at `tx - 1` modulo MSGQ_NUM_PAGES, inclusive,
+        // belongs to the driver for reading.
+        if rx <= tx {
+            // The area is contiguous.
+            // PANIC:
+            // - The index `tx - rx` is non-negative because `rx <= tx` in this branch.
+            // - The index does not exceed `after_rx.len()` (which is `MSGQ_NUM_PAGES - rx`)
+            //   because `tx < MSGQ_NUM_PAGES` by the `gsp_write_ptr` invariant.
+            (&after_rx[..(tx - rx)], &after_rx[0..0])
+        } else {
+            // The area is discontiguous.
+            // PANIC: `tx` does not exceed `before_rx.len()` (which equals `rx`) because
+            //   `tx < rx` in this branch.
+            (after_rx, &before_rx[..tx])
         }
     }
 

-- 
2.52.0
Re: [PATCH v2 4/4] gpu: nova-core: gsp: fix improper indexing in driver_read_area
Posted by Alexandre Courbot 1 week, 2 days ago 
On Fri Jan 23, 2026 at 9:12 PM JST, Eliot Courtney wrote:
> The current code indexes into `after_rx` using `tx` which is an index
> for the whole buffer, not the split buffer `after_rx`.

Another dangerous one! Well found.

>
> Also add more rigorous no-panic proofs.
>
> Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/cmdq.rs | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index aa8758fc7723..c26396fda29c 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -1,7 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
>  use core::{
> -    cmp,
>      mem,
>      sync::atomic::{
>          fence,
> @@ -267,10 +266,20 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>          // PANIC: per the invariant of `cpu_read_ptr`, `rx` is `< MSGQ_NUM_PAGES`.
>          let (before_rx, after_rx) = gsp_mem.gspq.msgq.data.split_at(rx);
>  
> -        match tx.cmp(&rx) {
> -            cmp::Ordering::Equal => (&after_rx[0..0], &after_rx[0..0]),
> -            cmp::Ordering::Greater => (&after_rx[..tx], &before_rx[0..0]),
> -            cmp::Ordering::Less => (after_rx, &before_rx[..tx]),
> +        // The area starting at `rx` and ending at `tx - 1` modulo MSGQ_NUM_PAGES, inclusive,
> +        // belongs to the driver for reading.
> +        if rx <= tx {
> +            // The area is contiguous.
> +            // PANIC:
> +            // - The index `tx - rx` is non-negative because `rx <= tx` in this branch.
> +            // - The index does not exceed `after_rx.len()` (which is `MSGQ_NUM_PAGES - rx`)
> +            //   because `tx < MSGQ_NUM_PAGES` by the `gsp_write_ptr` invariant.
> +            (&after_rx[..(tx - rx)], &after_rx[0..0])

I guess we can also use `&[]` here (maybe convert the whole file in the
same patch).
Re: [PATCH v2 4/4] gpu: nova-core: gsp: fix improper indexing in driver_read_area
Posted by Gary Guo 1 week, 4 days ago 
On Fri Jan 23, 2026 at 12:12 PM GMT, Eliot Courtney wrote:
> The current code indexes into `after_rx` using `tx` which is an index
> for the whole buffer, not the split buffer `after_rx`.
>
> Also add more rigorous no-panic proofs.
>
> Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/cmdq.rs | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index aa8758fc7723..c26396fda29c 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -1,7 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
>  use core::{
> -    cmp,
>      mem,
>      sync::atomic::{
>          fence,
> @@ -267,10 +266,20 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>          // PANIC: per the invariant of `cpu_read_ptr`, `rx` is `< MSGQ_NUM_PAGES`.
>          let (before_rx, after_rx) = gsp_mem.gspq.msgq.data.split_at(rx);

This code doesn't need splitting as it doesn't have the uniqueness issue that
mutable references have. While you're at it, probably it's chance to simplify
the code.

>  
> -        match tx.cmp(&rx) {
> -            cmp::Ordering::Equal => (&after_rx[0..0], &after_rx[0..0]),
> -            cmp::Ordering::Greater => (&after_rx[..tx], &before_rx[0..0]),
> -            cmp::Ordering::Less => (after_rx, &before_rx[..tx]),
> +        // The area starting at `rx` and ending at `tx - 1` modulo MSGQ_NUM_PAGES, inclusive,
> +        // belongs to the driver for reading.
> +        if rx <= tx {
> +            // The area is contiguous.
> +            // PANIC:
> +            // - The index `tx - rx` is non-negative because `rx <= tx` in this branch.
> +            // - The index does not exceed `after_rx.len()` (which is `MSGQ_NUM_PAGES - rx`)
> +            //   because `tx < MSGQ_NUM_PAGES` by the `gsp_write_ptr` invariant.
> +            (&after_rx[..(tx - rx)], &after_rx[0..0])

This can be just `(&data[rx..tx], &[])` without the split.

> +        } else {
> +            // The area is discontiguous.
> +            // PANIC: `tx` does not exceed `before_rx.len()` (which equals `rx`) because
> +            //   `tx < rx` in this branch.
> +            (after_rx, &before_rx[..tx])

This can be just `(&data[rx..], &data[..tx])` without the split.

Best,
Gary


>          }
>      }
>
Re: [PATCH v2 4/4] gpu: nova-core: gsp: fix improper indexing in driver_read_area
Posted by Alexandre Courbot 1 week, 2 days ago 
On Tue Jan 27, 2026 at 3:30 AM JST, Gary Guo wrote:
> On Fri Jan 23, 2026 at 12:12 PM GMT, Eliot Courtney wrote:
>> The current code indexes into `after_rx` using `tx` which is an index
>> for the whole buffer, not the split buffer `after_rx`.
>>
>> Also add more rigorous no-panic proofs.
>>
>> Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/gsp/cmdq.rs | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>> index aa8758fc7723..c26396fda29c 100644
>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>> @@ -1,7 +1,6 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  
>>  use core::{
>> -    cmp,
>>      mem,
>>      sync::atomic::{
>>          fence,
>> @@ -267,10 +266,20 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>>          // PANIC: per the invariant of `cpu_read_ptr`, `rx` is `< MSGQ_NUM_PAGES`.
>>          let (before_rx, after_rx) = gsp_mem.gspq.msgq.data.split_at(rx);
>
> This code doesn't need splitting as it doesn't have the uniqueness issue that
> mutable references have. While you're at it, probably it's chance to simplify
> the code.
>
>>  
>> -        match tx.cmp(&rx) {
>> -            cmp::Ordering::Equal => (&after_rx[0..0], &after_rx[0..0]),
>> -            cmp::Ordering::Greater => (&after_rx[..tx], &before_rx[0..0]),
>> -            cmp::Ordering::Less => (after_rx, &before_rx[..tx]),
>> +        // The area starting at `rx` and ending at `tx - 1` modulo MSGQ_NUM_PAGES, inclusive,
>> +        // belongs to the driver for reading.
>> +        if rx <= tx {
>> +            // The area is contiguous.
>> +            // PANIC:
>> +            // - The index `tx - rx` is non-negative because `rx <= tx` in this branch.
>> +            // - The index does not exceed `after_rx.len()` (which is `MSGQ_NUM_PAGES - rx`)
>> +            //   because `tx < MSGQ_NUM_PAGES` by the `gsp_write_ptr` invariant.
>> +            (&after_rx[..(tx - rx)], &after_rx[0..0])
>
> This can be just `(&data[rx..tx], &[])` without the split.
>
>> +        } else {
>> +            // The area is discontiguous.
>> +            // PANIC: `tx` does not exceed `before_rx.len()` (which equals `rx`) because
>> +            //   `tx < rx` in this branch.
>> +            (after_rx, &before_rx[..tx])
>
> This can be just `(&data[rx..], &data[..tx])` without the split.

Indeed, this is arguably easier to understand.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.