Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] This problem wasn't introduced by this patch, but there is a double unlock of `adev->mes` in the error handling path of `amdgpu_mes_add_ring()`. -- --- Patch [1]: [PATCH 6.6] drm/amdgpu: Use scnprintf() in amdgpu_mes_add_ring() --- commit 44ecf76b72a68dfa86323d985780462c9ca485b7 Author: Evgenii Burenchev <[email protected]> drm/amdgpu: Use scnprintf() in amdgpu_mes_add_ring() This commit replaces sprintf() with scnprintf() to prevent a potential buffer overflow when writing to ring->name. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c > index b6015157763af..d2128bcf9844f 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c [ ... ] > @@ -1023,13 +1023,14 @@ int amdgpu_mes_add_ring(struct amdgpu_device *adev, > int gang_id, > ring->doorbell_index = qprops.doorbell_off; > > if (queue_type == AMDGPU_RING_TYPE_GFX) > - sprintf(ring->name, "gfx_%d.%d.%d", pasid, gang_id, queue_id); > + scnprintf(ring->name, sizeof(ring->name), "gfx_%d.%d.%d", > + pasid, gang_id, queue_id); > else if (queue_type == AMDGPU_RING_TYPE_COMPUTE) > - sprintf(ring->name, "compute_%d.%d.%d", pasid, gang_id, > - queue_id); > + scnprintf(ring->name, sizeof(ring->name), "compute_%d.%d.%d", > + pasid, gang_id, queue_id); > else if (queue_type == AMDGPU_RING_TYPE_SDMA) > - sprintf(ring->name, "sdma_%d.%d.%d", pasid, gang_id, > - queue_id); > + scnprintf(ring->name, sizeof(ring->name), "sdma_%d.%d.%d", > + pasid, gang_id, queue_id); > else > BUG(); > [Severity: High] This isn't a bug introduced by this patch, but there appears to be a double unlock of adev->mes in the error handling path of amdgpu_mes_add_ring(). Earlier in amdgpu_mes_add_ring(), the lock is explicitly released before calling amdgpu_mes_add_hw_queue(): amdgpu_mes_unlock(&adev->mes); r = amdgpu_mes_add_hw_queue(adev, gang_id, &qprops, &queue_id); if (r) goto clean_up_ring; If amdgpu_mes_add_hw_queue() fails, it jumps to clean_up_ring, which falls through to clean_up_memory where it unconditionally unlocks again: clean_up_ring: amdgpu_ring_fini(ring); clean_up_memory: kfree(ring); amdgpu_mes_unlock(&adev->mes); return r; Could this result in a double unlock when the hardware queue allocation fails? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
