[v3 5/6] drm/amdgpu: fix invalid memory access in amdgpu_fence_driver_sw_fini()

[Christian König]

Am 08.01.25 um 17:30 schrieb Chen, Xiaogang:
>
> On 1/8/2025 3:16 AM, Christian König wrote:
>> Am 08.01.25 um 09:56 schrieb Jiang Liu:
>>> Function detects initialization status by checking sched->ops,
>>
>> Where is that done? Inside the scheduler or inside amdgpu?
> Inside amdgpu set ring->sched.ops to null if ring's scheduler init 
> fail since we use ring->sched.ops to decide uninit it by drm_sched_fini.

That is probably something we should stop doing instead.

amdgpu_device_init_schedulers() needs some proper error handling and a 
matching amdgpu_device_fini_schedulers() function.

That this is still in the fence code and looking at the scheduler ops is 
probably just a leftover from very long ago.

Regards,
Christian.

>>
>> Regards,
>> Christian.
>>
>>>   so set
>>> sched->ops to non-NULL just before return in function
>>> amdgpu_fence_driver_sw_fini() and amdgpu_device_init_schedulers()
>>> to avoid possible invalid memory access on error recover path.
>>>
>>> Signed-off-by: Jiang Liu <gerry at linux.alibaba.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 4 +++-
>>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 510074a9074e..741807a1fd2e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -2857,6 +2857,7 @@ static int 
>>> amdgpu_device_init_schedulers(struct amdgpu_device *adev)
>>>           if (r) {
>>>               DRM_ERROR("Failed to create scheduler on ring %s.\n",
>>>                     ring->name);
>>> +            ring->sched.ops = NULL;
>>>               return r;
>>>           }
>>>           r = amdgpu_uvd_entity_init(adev, ring);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> index 2f24a6aa13bf..b5e87b515139 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> @@ -656,8 +656,10 @@ void amdgpu_fence_driver_sw_fini(struct 
>>> amdgpu_device *adev)
>>>            * The natural check would be sched.ready, which is
>>>            * set as drm_sched_init() finishes...
>>>            */
>>> -        if (ring->sched.ops)
>>> +        if (ring->sched.ops) {
>>>               drm_sched_fini(&ring->sched);
>>> +            ring->sched.ops = NULL;
>>> +        }
>>>             for (j = 0; j <= ring->fence_drv.num_fences_mask; ++j)
>>>               dma_fence_put(ring->fence_drv.fences[j]);
>>