Re: [PATCH v4 2/2] gpu: nova-core: gsp: fix undefined behavior in command queue code

Gary Guo
2026-04-04T12:33:19Z

On Sat Apr 4, 2026 at 5:37 AM BST, Alexandre Courbot wrote:
> On Sat Apr 4, 2026 at 9:05 AM JST, Danilo Krummrich wrote:
>> On Sat Apr 4, 2026 at 1:47 AM CEST, John Hubbard wrote:
>>> On 4/1/26 7:29 AM, Alexandre Courbot wrote:
>>>> `driver_read_area` and `driver_write_area` are internal methods that
>>>> return slices containing the area of the command queue buffer that the
>>>> driver has exclusive read or write access, respectively.
>>>>
>>>> While their returned value is correct and safe to use, internally they
>>>> temporarily create a reference to the whole command-buffer slice,
>>>> including GSP-owned regions. These regions can change without notice,
>>>> and thus creating a slice to them, even if never accessed, is undefined
>>>> behavior.
>>>>
>>>> Fix this by rewriting these methods to use pointer projections in order
>>>> to create slices to valid regions only. It should eventually be replaced
>>>> by `IoView` and `IoSlice` once they land.
>>>>
>>>> Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
>>>> Reported-by: Danilo Krummrich <dakr@kernel.org>
>>>> Closes: https://lore.kernel.org/all/DH47AVPEKN06.3BERUSJIB4M1R@kernel.org/
>>>> Reviewed-by: Gary Guo <gary@garyguo.net>
>>>> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
>>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>>> ---
>>>> drivers/gpu/nova-core/gsp/cmdq.rs | 114 ++++++++++++++++++++++----------------
>>>> 1 file changed, 65 insertions(+), 49 deletions(-)
>>>
>>> This is causing a build_assert failure in the latest drm-rust-next, with
>>> rustc 1.85.0, and also with 1.78.0.
>>>
>>> rustc 1.93.0 does not show the problem.
>>
>> Odd, it did pass all the testing at my end. Anyways, it only proves once again
>> that this is pretty fragile.
>
> Same here, although I did not test with all the supported rustc versions
> (but pretty sure I did test 1.78). This seems to be heavily
> configuration dependent.
>
> Gary, do you know of a way to restrict the `project_pointer` macro to
> only take constant values as indices? That problem is pretty sneaky and
> likely to happen again, so we should probably harden that part a bit.

It doesn't necessary need to be const, for plain indexing (not slicing), if you
do modular arithmetic or bound-checking before accessing it's pretty reliably
optimized out. Slicing is more tricky, as there're more checking involved here.

For the macro, it doesn't really know whether you're slicing or indexing, so
this restriction is not trivial. We discussed this at length in last RfL
meeting, and we've come up with keyworded projection variants like this:

project!(p, [try foo]) // fallible
project!(p, [build foo]) // build_assert! variant, for indexing or constants (indexing/slicing)
project!(p, [panic foo]) // panicking variant

I'll be working on this next week.

Best,
Gary