[PATCH] drm/scheduler: add NULL pointer check for run queue

[PATCH] drm/scheduler: add NULL pointer check for run queue

[Junwei Zhang]

To check rq pointer before adding entity into it.
That avoids NULL pointer access in some case.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
index 16bf446..5e5268d 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
@@ -91,6 +91,10 @@ static void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
 {
 	if (!list_empty(&entity->list))
 		return;
+	if (!rq) {
+		DRM_ERROR("rq is NULL!\n");
+		return;
+	}
 	spin_lock(&rq->lock);
 	list_add_tail(&entity->list, &rq->entities);
 	spin_unlock(&rq->lock);
-- 
1.9.1

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

Re: [PATCH] drm/scheduler: add NULL pointer check for run queue

[Christian König]

Am 16.07.2018 um 04:59 schrieb Junwei Zhang:
> To check rq pointer before adding entity into it.
> That avoids NULL pointer access in some case.
>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
> ---
>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> index 16bf446..5e5268d 100644
> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> @@ -91,6 +91,10 @@ static void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
>   {
>   	if (!list_empty(&entity->list))
>   		return;
> +	if (!rq) {
> +		DRM_ERROR("rq is NULL!\n");
> +		return;
> +	}

Better put that into drm_sched_entity_push_job(), e.g. something like:

/* first job wakes up scheduler */
if (first) {
     /* Add the entity to the run queue */
     spin_lock(&entity->rq_lock);
     if (!entity->rq) {
         DRM_ERROR("Trying to push to killed entity!\n");
....

Regards,
Christian.

>   	spin_lock(&rq->lock);
>   	list_add_tail(&entity->list, &rq->entities);
>   	spin_unlock(&rq->lock);

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

Re: [PATCH] drm/scheduler: add NULL pointer check for run queue

[Zhang, Jerry (Junwei)]

On 07/16/2018 04:31 PM, Christian König wrote:
> Am 16.07.2018 um 04:59 schrieb Junwei Zhang:
>> To check rq pointer before adding entity into it.
>> That avoids NULL pointer access in some case.
>>
>> Suggested-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>> ---
>>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> index 16bf446..5e5268d 100644
>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> @@ -91,6 +91,10 @@ static void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
>>   {
>>       if (!list_empty(&entity->list))
>>           return;
>> +    if (!rq) {
>> +        DRM_ERROR("rq is NULL!\n");
>> +        return;
>> +    }
>
> Better put that into drm_sched_entity_push_job(), e.g. something like:

Considered that as well.
Just be afraid of someone else could call it in another place without rq checking in the future.

Regards,
Jerry

>
> /* first job wakes up scheduler */
> if (first) {
>      /* Add the entity to the run queue */
>      spin_lock(&entity->rq_lock);
>      if (!entity->rq) {
>          DRM_ERROR("Trying to push to killed entity!\n");
> ....
>
> Regards,
> Christian.
>
>>       spin_lock(&rq->lock);
>>       list_add_tail(&entity->list, &rq->entities);
>>       spin_unlock(&rq->lock);
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/scheduler: add NULL pointer check for run queue

[Christian König]

Am 16.07.2018 um 11:01 schrieb Zhang, Jerry (Junwei):
> On 07/16/2018 04:31 PM, Christian König wrote:
>> Am 16.07.2018 um 04:59 schrieb Junwei Zhang:
>>> To check rq pointer before adding entity into it.
>>> That avoids NULL pointer access in some case.
>>>
>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>>> ---
>>>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
>>> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>> index 16bf446..5e5268d 100644
>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>> @@ -91,6 +91,10 @@ static void drm_sched_rq_add_entity(struct 
>>> drm_sched_rq *rq,
>>>   {
>>>       if (!list_empty(&entity->list))
>>>           return;
>>> +    if (!rq) {
>>> +        DRM_ERROR("rq is NULL!\n");
>>> +        return;
>>> +    }
>>
>> Better put that into drm_sched_entity_push_job(), e.g. something like:
>
> Considered that as well.
> Just be afraid of someone else could call it in another place without 
> rq checking in the future.

Well that's exactly the reason why I wanted to have the check in 
drm_sched_rq_add_entity().

Calling drm_sched_rq_add_entity() will a NULL rq is illegal and that 
should be avoided in the caller instead of more or less silently dropped 
in the function.

Regards,
Christian.

>
> Regards,
> Jerry
>
>>
>> /* first job wakes up scheduler */
>> if (first) {
>>      /* Add the entity to the run queue */
>>      spin_lock(&entity->rq_lock);
>>      if (!entity->rq) {
>>          DRM_ERROR("Trying to push to killed entity!\n");
>> ....
>>
>> Regards,
>> Christian.
>>
>>>       spin_lock(&rq->lock);
>>>       list_add_tail(&entity->list, &rq->entities);
>>>       spin_unlock(&rq->lock);
>>

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

Re: [PATCH] drm/scheduler: add NULL pointer check for run queue

[Zhang, Jerry (Junwei)]

On 07/16/2018 05:11 PM, Christian König wrote:
> Am 16.07.2018 um 11:01 schrieb Zhang, Jerry (Junwei):
>> On 07/16/2018 04:31 PM, Christian König wrote:
>>> Am 16.07.2018 um 04:59 schrieb Junwei Zhang:
>>>> To check rq pointer before adding entity into it.
>>>> That avoids NULL pointer access in some case.
>>>>
>>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>> index 16bf446..5e5268d 100644
>>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>> @@ -91,6 +91,10 @@ static void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
>>>>   {
>>>>       if (!list_empty(&entity->list))
>>>>           return;
>>>> +    if (!rq) {
>>>> +        DRM_ERROR("rq is NULL!\n");
>>>> +        return;
>>>> +    }
>>>
>>> Better put that into drm_sched_entity_push_job(), e.g. something like:
>>
>> Considered that as well.
>> Just be afraid of someone else could call it in another place without rq checking in the future.
>
> Well that's exactly the reason why I wanted to have the check in drm_sched_rq_add_entity().
>
> Calling drm_sched_rq_add_entity() will a NULL rq is illegal and that should be avoided in the caller instead of more or less silently dropped in the function.

Fine, to expose the error in place explicitly.

Regards,
Jerry

>
> Regards,
> Christian.
>
>>
>> Regards,
>> Jerry
>>
>>>
>>> /* first job wakes up scheduler */
>>> if (first) {
>>>      /* Add the entity to the run queue */
>>>      spin_lock(&entity->rq_lock);
>>>      if (!entity->rq) {
>>>          DRM_ERROR("Trying to push to killed entity!\n");
>>> ....
>>>
>>> Regards,
>>> Christian.
>>>
>>>>       spin_lock(&rq->lock);
>>>>       list_add_tail(&entity->list, &rq->entities);
>>>>       spin_unlock(&rq->lock);
>>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx