[bug report] drm/amdgpu/mes: use ring for kernel queue submission

* [bug report] drm/amdgpu/mes: use ring for kernel queue submission
@ 2022-05-09  9:07 Dan Carpenter
  0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2022-05-09  9:07 UTC (permalink / raw)
  To: Jack.Xiao; +Cc: amd-gfx

Hello Jack Xiao,

The patch d0c423b64765: "drm/amdgpu/mes: use ring for kernel queue
submission" from Mar 27, 2020, leads to the following Smatch static
checker warning:

drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c:924 amdgpu_mes_add_ring() error: format string overflow. buf_size: 16 length: 39
drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c:927 amdgpu_mes_add_ring() error: format string overflow. buf_size: 16 length: 43
drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c:930 amdgpu_mes_add_ring() error: format string overflow. buf_size: 16 length: 40

drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
    848 int amdgpu_mes_add_ring(struct amdgpu_device *adev, int gang_id,
    849                         int queue_type, int idx,
    850                         struct amdgpu_mes_ctx_data *ctx_data,
    851                         struct amdgpu_ring **out)
    852 {
    853         struct amdgpu_ring *ring;
    854         struct amdgpu_mes_gang *gang;
    855         struct amdgpu_mes_queue_properties qprops = {0};
    856         int r, queue_id, pasid;
    857 
    858         /*
    859          * Avoid taking any other locks under MES lock to avoid circular
    860          * lock dependencies.
    861          */
    862         amdgpu_mes_lock(&adev->mes);
    863         gang = idr_find(&adev->mes.gang_id_idr, gang_id);
    864         if (!gang) {
    865                 DRM_ERROR("gang id %d doesn't exist\n", gang_id);
    866                 amdgpu_mes_unlock(&adev->mes);
    867                 return -EINVAL;
    868         }
    869         pasid = gang->process->pasid;
    870 
    871         ring = kzalloc(sizeof(struct amdgpu_ring), GFP_KERNEL);
    872         if (!ring) {
    873                 amdgpu_mes_unlock(&adev->mes);
    874                 return -ENOMEM;
    875         }
    876 
    877         ring->ring_obj = NULL;
    878         ring->use_doorbell = true;
    879         ring->is_mes_queue = true;
    880         ring->mes_ctx = ctx_data;
    881         ring->idx = idx;
    882         ring->no_scheduler = true;
    883 
    884         if (queue_type == AMDGPU_RING_TYPE_COMPUTE) {
    885                 int offset = offsetof(struct amdgpu_mes_ctx_meta_data,
    886                                       compute[ring->idx].mec_hpd);
    887                 ring->eop_gpu_addr =
    888                         amdgpu_mes_ctx_get_offs_gpu_addr(ring, offset);
    889         }
    890 
    891         switch (queue_type) {
    892         case AMDGPU_RING_TYPE_GFX:
    893                 ring->funcs = adev->gfx.gfx_ring[0].funcs;
    894                 break;
    895         case AMDGPU_RING_TYPE_COMPUTE:
    896                 ring->funcs = adev->gfx.compute_ring[0].funcs;
    897                 break;
    898         case AMDGPU_RING_TYPE_SDMA:
    899                 ring->funcs = adev->sdma.instance[0].ring.funcs;
    900                 break;
    901         default:
    902                 BUG();
    903         }
    904 
    905         r = amdgpu_ring_init(adev, ring, 1024, NULL, 0,
    906                              AMDGPU_RING_PRIO_DEFAULT, NULL);
    907         if (r)
    908                 goto clean_up_memory;
    909 
    910         amdgpu_mes_ring_to_queue_props(adev, ring, &qprops);
    911 
    912         dma_fence_wait(gang->process->vm->last_update, false);
    913         dma_fence_wait(ctx_data->meta_data_va->last_pt_update, false);
    914         amdgpu_mes_unlock(&adev->mes);
    915 
    916         r = amdgpu_mes_add_hw_queue(adev, gang_id, &qprops, &queue_id);
    917         if (r)
    918                 goto clean_up_ring;
    919 
    920         ring->hw_queue_id = queue_id;
    921         ring->doorbell_index = qprops.doorbell_off;
    922 
    923         if (queue_type == AMDGPU_RING_TYPE_GFX)
--> 924                 sprintf(ring->name, "gfx_%d.%d.%d", pasid, gang_id, queue_id);

Using sprintf() is always ill-advised.  Better to use snprintf().

"gfx_.." 6 characters.
passid is capped at USHRT_MAX so 5 characters
gang_id is capped at INT_MAX so 10 characters
queue_id is up to 10 characters as well.
1 char for the NUL terminator

Smatch is saying that it can be 39 characters but depending on the
implementation of idr_alloc() this could reach up to 32 characters.
Still that's well past the 16 characters avaliable.

    925         else if (queue_type == AMDGPU_RING_TYPE_COMPUTE)
    926                 sprintf(ring->name, "compute_%d.%d.%d", pasid, gang_id,
    927                         queue_id);

Same

    928         else if (queue_type == AMDGPU_RING_TYPE_SDMA)
    929                 sprintf(ring->name, "sdma_%d.%d.%d", pasid, gang_id,
    930                         queue_id);

Same

    931         else
    932                 BUG();
    933 
    934         *out = ring;
    935         return 0;
    936 
    937 clean_up_ring:
    938         amdgpu_ring_fini(ring);
    939 clean_up_memory:
    940         kfree(ring);
    941         amdgpu_mes_unlock(&adev->mes);
    942         return r;
    943 }

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 3+ messages in thread