amdgpf: BUG: NULL pointer dereference and memory leak

amdgpf: BUG: NULL pointer dereference and memory leak

[亿一]

Hi  alll,
         While analyzing the source code, I notice that function
amdgpu_cs_process_fence_dep() may exist NULL pointer dereference and
memory leak in the following code fragments:


fence = amdgpu_ctx_get_fence(ctx, entity,
    deps[i].handle);

if (chunk->chunk_id == AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES) {
        struct drm_sched_fence *s_fence = to_drm_sched_fence(fence);
        struct dma_fence *old = fence;

        fence = dma_fence_get(&s_fence->scheduled);
        dma_fence_put(old);
}

if (IS_ERR(fence)) {
         r = PTR_ERR(fence);
         amdgpu_ctx_put(ctx);
         return r;
  } else if (fence) {
          r = amdgpu_sync_fence(p->adev, &p->job->sync, fence,
                                                 true);
          dma_fence_put(fence);
           amdgpu_ctx_put(ctx);
           if (r)
           return r;
           }

function amdgpu_ctx_get_fence may return NULL pointer,  which will
cause NULL pointer dereference. What's more,  IS_ERR() would not
return true when pointer is NULL,  which will cause the ctx reference
leaked.
But I don't know how to fix it, so report it to you all.

Best Regards.
Lin Yi.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amdgpf: BUG: NULL pointer dereference and memory leak

[Koenig, Christian]

Am 30.07.19 um 10:47 schrieb 亿一:
> Hi  alll,
>           While analyzing the source code, I notice that function
> amdgpu_cs_process_fence_dep() may exist NULL pointer dereference and
> memory leak in the following code fragments:
>
>
> fence = amdgpu_ctx_get_fence(ctx, entity,
>      deps[i].handle);
>
> if (chunk->chunk_id == AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES) {
>          struct drm_sched_fence *s_fence = to_drm_sched_fence(fence);
>          struct dma_fence *old = fence;
>
>          fence = dma_fence_get(&s_fence->scheduled);
>          dma_fence_put(old);
> }
>
> if (IS_ERR(fence)) {
>           r = PTR_ERR(fence);
>           amdgpu_ctx_put(ctx);
>           return r;
>    } else if (fence) {
>            r = amdgpu_sync_fence(p->adev, &p->job->sync, fence,
>                                                   true);
>            dma_fence_put(fence);
>             amdgpu_ctx_put(ctx);
>             if (r)
>             return r;
>             }
>
> function amdgpu_ctx_get_fence may return NULL pointer,  which will
> cause NULL pointer dereference. What's more,  IS_ERR() would not
> return true when pointer is NULL,  which will cause the ctx reference
> leaked.

That handling is actually correct.

The problem is the "if (chunk->chunk_id == 
AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES)" stuff above.

That comes to early and needs to be moved below checking the fence for 
errors. Going to send a fix for this to the mailing list in a minute.

Thanks for the notice,
Christian.

> But I don't know how to fix it, so report it to you all.
>
> Best Regards.
> Lin Yi.

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amdgpf: BUG: NULL pointer dereference and memory leak

[zhoucm1]

On 2019年07月30日 17:04, Koenig, Christian wrote:
> Am 30.07.19 um 10:47 schrieb 亿一:
>> Hi  alll,
>>            While analyzing the source code, I notice that function
>> amdgpu_cs_process_fence_dep() may exist NULL pointer dereference and
>> memory leak in the following code fragments:
>>
>>
>> fence = amdgpu_ctx_get_fence(ctx, entity,
>>       deps[i].handle);
>>
>> if (chunk->chunk_id == AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES) {
>>           struct drm_sched_fence *s_fence = to_drm_sched_fence(fence);
>>           struct dma_fence *old = fence;
>>
>>           fence = dma_fence_get(&s_fence->scheduled);
>>           dma_fence_put(old);
>> }
>>
>> if (IS_ERR(fence)) {
>>            r = PTR_ERR(fence);
>>            amdgpu_ctx_put(ctx);
>>            return r;
>>     } else if (fence) {
>>             r = amdgpu_sync_fence(p->adev, &p->job->sync, fence,
>>                                                    true);
>>             dma_fence_put(fence);
>>              amdgpu_ctx_put(ctx);
>>              if (r)
>>              return r;
>>              }
>>
>> function amdgpu_ctx_get_fence may return NULL pointer,  which will
>> cause NULL pointer dereference. What's more,  IS_ERR() would not
>> return true when pointer is NULL,  which will cause the ctx reference
>> leaked.
> That handling is actually correct.
>
> The problem is the "if (chunk->chunk_id ==
> AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES)" stuff above.
>
> That comes to early and needs to be moved below checking the fence for
> errors. Going to send a fix for this to the mailing list in a minute.
Lin Yi is right I think, we leaked ctx reference when fence is NULL.

-David
>
> Thanks for the notice,
> Christian.
>
>> But I don't know how to fix it, so report it to you all.
>>
>> Best Regards.
>> Lin Yi.

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amdgpf: BUG: NULL pointer dereference and memory leak

[Koenig, Christian]

Am 30.07.19 um 11:14 schrieb zhoucm1:
>
>
> On 2019年07月30日 17:04, Koenig, Christian wrote:
>> Am 30.07.19 um 10:47 schrieb 亿一:
>>> Hi  alll,
>>>            While analyzing the source code, I notice that function
>>> amdgpu_cs_process_fence_dep() may exist NULL pointer dereference and
>>> memory leak in the following code fragments:
>>>
>>>
>>> fence = amdgpu_ctx_get_fence(ctx, entity,
>>>       deps[i].handle);
>>>
>>> if (chunk->chunk_id == AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES) {
>>>           struct drm_sched_fence *s_fence = to_drm_sched_fence(fence);
>>>           struct dma_fence *old = fence;
>>>
>>>           fence = dma_fence_get(&s_fence->scheduled);
>>>           dma_fence_put(old);
>>> }
>>>
>>> if (IS_ERR(fence)) {
>>>            r = PTR_ERR(fence);
>>>            amdgpu_ctx_put(ctx);
>>>            return r;
>>>     } else if (fence) {
>>>             r = amdgpu_sync_fence(p->adev, &p->job->sync, fence,
>>>                                                    true);
>>>             dma_fence_put(fence);
>>>              amdgpu_ctx_put(ctx);
>>>              if (r)
>>>              return r;
>>>              }
>>>
>>> function amdgpu_ctx_get_fence may return NULL pointer,  which will
>>> cause NULL pointer dereference. What's more,  IS_ERR() would not
>>> return true when pointer is NULL,  which will cause the ctx reference
>>> leaked.
>> That handling is actually correct.
>>
>> The problem is the "if (chunk->chunk_id ==
>> AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES)" stuff above.
>>
>> That comes to early and needs to be moved below checking the fence for
>> errors. Going to send a fix for this to the mailing list in a minute.
> Lin Yi is right I think, we leaked ctx reference when fence is NULL.

Indeed, but what I meant was the a NULL fence here is not an error.

Just send out a patch to fix that stuff up, please review.

Christian.

>
> -David
>>
>> Thanks for the notice,
>> Christian.
>>
>>> But I don't know how to fix it, so report it to you all.
>>>
>>> Best Regards.
>>> Lin Yi.
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx