Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Double unlock of `adev->mes` mutex in error path. - [High] Unchecked NULL fence pointers passed to `dma_fence_wait()`. -- --- Patch [1]: [PATCH 6.1] drm/amdgpu: Use scnprintf() in amdgpu_mes_add_ring() --- commit a3f6095725ba7df26135d6b5a8b649631f54d76e Author: Evgenii Burenchev <[email protected]> drm/amdgpu: Use scnprintf() in amdgpu_mes_add_ring() Replace 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 0c546245793b9..41e699f4b5d2e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c > @@ -1053,13 +1053,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) [Severity: High] This is a pre-existing issue, but I noticed a potential NULL pointer dereference just before this code in amdgpu_mes_add_ring(): dma_fence_wait(gang->process->vm->last_update, false); dma_fence_wait(ctx_data->meta_data_va->last_pt_update, false); If the fences are unallocated, these pointers might be NULL. The dma_fence_wait() function does not check for NULL inputs before dereferencing. Could this result in a kernel oops during normal process/gang queue creation? [Severity: High] This is a pre-existing issue, but I also noticed a potential double unlock of the adev->mes mutex. Before the new scnprintf() code, we unlock it here: 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: clean_up_memory: kfree(ring); amdgpu_mes_unlock(&adev->mes); return r; Will this second call to amdgpu_mes_unlock() cause a double unlock panic or state corruption since the mutex was already dropped? > - 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(); > -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
