[PATCH] drm/amdgpu: Fix the page fault issue in amdgpu_irq_fini

[Christian König]

Am 19.03.21 um 02:38 schrieb Deng, Emily:
> [AMD Official Use Only - Internal Distribution Only]
>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken at gmail.com>
>> Sent: Thursday, March 18, 2021 7:52 PM
>> To: Deng, Emily <Emily.Deng at amd.com>; amd-gfx at lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: Fix the page fault issue in amdgpu_irq_fini
>>
>> Am 18.03.21 um 12:48 schrieb Emily Deng:
>>> For some source, it will be shared by some client ID and source ID.
>>> To fix the page fault issue, set all those to null.
>>>
>>> Signed-off-by: Emily Deng <Emily.Deng at amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 16 +++++++++++++---
>>>    1 file changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>>> index af026109421a..623b1ac6231d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>>> @@ -359,7 +359,7 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>>>     */
>>>    void amdgpu_irq_fini(struct amdgpu_device *adev)
>>>    {
>>> -unsigned i, j;
>>> +unsigned i, j, m, n;
>>>
>>>    if (adev->irq.installed) {
>>>    drm_irq_uninstall(adev_to_drm(adev));
>>> @@ -380,12 +380,22 @@ void amdgpu_irq_fini(struct amdgpu_device
>> *adev)
>>>    if (!src)
>>>    continue;
>>>
>>> -kfree(src->enabled_types);
>>> +if (src->enabled_types)
>>> +kfree(src->enabled_types);
>> A NULL check before kfree() is unecessary and will be complained about by the
>> static checkers.
> Sorry, will remove this.
>>> +
>>>    src->enabled_types = NULL;
>>> +
>> Unrelated white space change.
> Sorry, will remove this also.
>>>    if (src->data) {
>>>    kfree(src->data);
>>>    kfree(src);
>>> -adev->irq.client[i].sources[j] = NULL;
>>> +}
>>> +
>>> +for (m = 0; m < AMDGPU_IRQ_CLIENTID_MAX; ++m) {
>>> +if (!adev->irq.client[m].sources)
>>> +continue;
>>> +for (n = 0; n < AMDGPU_MAX_IRQ_SRC_ID;
>> ++n)
>>> +if (adev->irq.client[m].sources[n] ==
>> src)
>>> +adev->irq.client[m].sources[n]
>> = NULL;
>>
>> Hui what? The memory you set to NULL here is freed on the line below.
>>
>> Accessing it after that would be illegal, so why do you want to set it to NULL?
> [Emily] It is in the loop "for (j = 0; j < AMDGPU_MAX_IRQ_SRC_ID; ++j) {", shouldn't have been freed in this loop. Only set " adev->irq.client[i].sources[j] = NULL;" is not enough,
> as it maybe have other client ID and src ID will share the same src. Also need to set those to NULL.

No, that can't happen.

It is illegal to use a dynamic allocated source with multiple client ID 
and src ID. Where do you see that?

We could also probably completely remove this feature since it is unused 
as far as I know.

Thanks,
Christian.

>> Christian.
>>
>>>    }
>>>    }
>>>    kfree(adev->irq.client[i].sources);