Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors
From: Gary Guo
Date: Wed Mar 11 2026 - 08:58:41 EST
On Tue Mar 10, 2026 at 3:56 AM GMT, Eliot Courtney wrote:
> On Tue Mar 10, 2026 at 11:01 AM JST, Gary Guo wrote:
>>> + pub(in crate::gsp) fn advance_cpu_read_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
>>> + let rptr = cpu_read_ptr(qs).wrapping_add(count) % MSGQ_NUM_PAGES;
>>> +
>>> + // Ensure read pointer is properly ordered.
>>> + fence(Ordering::SeqCst);
>>> +
>>> + // PANIC: A `dma::CoherentAllocation` always contains at least one element.
>>> + || -> Result {
>>> + dma_write!(qs, [0]?.cpuq.rx.0.readPtr, rptr);
>>> + Ok(())
>>> + }()
>>> + .unwrap()
>>> + }
>>> +
>>> + pub(in crate::gsp) fn cpu_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
>>> + // PANIC: A `dma::CoherentAllocation` always contains at least one element.
>>> + || -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
>>> + }
>>> +
>>> + pub(in crate::gsp) fn advance_cpu_write_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
>>> + let wptr = cpu_write_ptr(qs).wrapping_add(count) & MSGQ_NUM_PAGES;
>>
>> Not really related to your change, but this `&` probably require a comment, as
>> it has different behaviour compared to `%` given the `MSGQ_NUM_PAGES` is not
>> power of two. I suppose this is actually intended so there's a way to
>> distinguish between empty and full ring buffer?
>>
>> Best,
>> Gary
>
> This is actually incorrect and I have fixed it here[1]. I think it
> should be merged in drm-rust-next now.
Right, I recollect reviewing your series which is why I am confused when I still
see the `&` present, thinking that this must be intentional rather than
accidental.
If this patch is intended to go through -fixes, then we should really land your
series via -fixes too, otherwise it is just causing unnecessary conflicts on
linux-next.
Best,
Gary
>
> [1]: https://lore.kernel.org/all/20260129-nova-core-cmdq1-v3-0-2ede85493a27@xxxxxxxxxx/