diff mbox series

[i-g-t] intel_gpu_top: Fix cleanup on old kernels / unsupported GPU

Message ID 20230119155428.3260937-1-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [i-g-t] intel_gpu_top: Fix cleanup on old kernels / unsupported GPU | expand

Commit Message

Tvrtko Ursulin Jan. 19, 2023, 3:54 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Avoid trying to dereference null engines on exit when there are either
none which are supported, or kernel does not have i915 PMU support.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tools/intel_gpu_top.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Nirmoy Das Jan. 20, 2023, 2:29 p.m. UTC | #1 
Hi Tvrtko,

On 1/19/2023 4:54 PM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Avoid trying to dereference null engines on exit when there are either
> none which are supported, or kernel does not have i915 PMU support.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   tools/intel_gpu_top.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> index 7aa233570463..517dc2995d26 100644
> --- a/tools/intel_gpu_top.c
> +++ b/tools/intel_gpu_top.c
> @@ -448,6 +448,9 @@ static void free_engines(struct engines *engines)
>   	};
>   	unsigned int i;
>   
> +	if (!engines)
> +		return;
> +


This check should happen before the  struct pmu_counter free_list 
generation.


Regards,

Nirmoy

>   	for (pmu = &free_list[0]; *pmu; pmu++) {
>   		if ((*pmu)->present)
>   			free((char *)(*pmu)->units);
> @@ -2568,7 +2571,7 @@ int main(int argc, char **argv)
>   			"Failed to detect engines! (%s)\n(Kernel 4.16 or newer is required for i915 PMU support.)\n",
>   			strerror(errno));
>   		ret = EXIT_FAILURE;
> -		goto err;
> +		goto err_engines;
>   	}
>   
>   	ret = pmu_init(engines);
> @@ -2585,7 +2588,7 @@ int main(int argc, char **argv)
>   "More information can be found at 'Perf events and tool security' document:\n"
>   "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
>   		ret = EXIT_FAILURE;
> -		goto err;
> +		goto err_pmu;
>   	}
>   
>   	ret = EXIT_SUCCESS;
> @@ -2699,8 +2702,9 @@ int main(int argc, char **argv)
>   		free_clients(clients);
>   
>   	free(codename);
> -err:
> +err_pmu:
>   	free_engines(engines);
> +err_engines:
>   	free(pmu_device);
>   exit:
>   	igt_devices_free();
Tvrtko Ursulin Jan. 23, 2023, 10:24 a.m. UTC | #2 
On 20/01/2023 14:29, Das, Nirmoy wrote:
> Hi Tvrtko,
> 
> On 1/19/2023 4:54 PM, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Avoid trying to dereference null engines on exit when there are either
>> none which are supported, or kernel does not have i915 PMU support.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   tools/intel_gpu_top.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>> index 7aa233570463..517dc2995d26 100644
>> --- a/tools/intel_gpu_top.c
>> +++ b/tools/intel_gpu_top.c
>> @@ -448,6 +448,9 @@ static void free_engines(struct engines *engines)
>>       };
>>       unsigned int i;
>> +    if (!engines)
>> +        return;
>> +
> 
> 
> This check should happen before the  struct pmu_counter free_list 
> generation.

I could, but it doesn't have to, not sure what reasons for should you 
have in mind?

Regards,

Tvrtko

> 
> Regards,
> 
> Nirmoy
> 
>>       for (pmu = &free_list[0]; *pmu; pmu++) {
>>           if ((*pmu)->present)
>>               free((char *)(*pmu)->units);
>> @@ -2568,7 +2571,7 @@ int main(int argc, char **argv)
>>               "Failed to detect engines! (%s)\n(Kernel 4.16 or newer 
>> is required for i915 PMU support.)\n",
>>               strerror(errno));
>>           ret = EXIT_FAILURE;
>> -        goto err;
>> +        goto err_engines;
>>       }
>>       ret = pmu_init(engines);
>> @@ -2585,7 +2588,7 @@ int main(int argc, char **argv)
>>   "More information can be found at 'Perf events and tool security' 
>> document:\n"
>>   
>> "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
>>           ret = EXIT_FAILURE;
>> -        goto err;
>> +        goto err_pmu;
>>       }
>>       ret = EXIT_SUCCESS;
>> @@ -2699,8 +2702,9 @@ int main(int argc, char **argv)
>>           free_clients(clients);
>>       free(codename);
>> -err:
>> +err_pmu:
>>       free_engines(engines);
>> +err_engines:
>>       free(pmu_device);
>>   exit:
>>       igt_devices_free();
Nirmoy Das Jan. 23, 2023, 11:09 a.m. UTC | #3 
Hi Tvrtko,

On 1/23/2023 11:24 AM, Tvrtko Ursulin wrote:
>
> On 20/01/2023 14:29, Das, Nirmoy wrote:
>> Hi Tvrtko,
>>
>> On 1/19/2023 4:54 PM, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Avoid trying to dereference null engines on exit when there are either
>>> none which are supported, or kernel does not have i915 PMU support.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>   tools/intel_gpu_top.c | 10 +++++++---
>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>>> index 7aa233570463..517dc2995d26 100644
>>> --- a/tools/intel_gpu_top.c
>>> +++ b/tools/intel_gpu_top.c
>>> @@ -448,6 +448,9 @@ static void free_engines(struct engines *engines)
>>>       };
>>>       unsigned int i;
>>> +    if (!engines)
>>> +        return;
>>> +
>>
>>
>> This check should happen before the  struct pmu_counter free_list 
>> generation.
>
> I could, but it doesn't have to, not sure what reasons for should you 
> have in mind?

In my tree, I see:

"

static void free_engines(struct engines *engines)

{
         struct pmu_counter **pmu, *free_list[] = {
                 &engines->r_gpu,
                 &engines->r_pkg,
                 &engines->imc_reads,
                 &engines->imc_writes,
                 NULL
         };

         unsigned int i;

"

Maybe I am missing something here, wouldn't those dereferences of 
"engines" segfault if the engines  is NULL.


Regards,

Nirmoy

>
> Regards,
>
> Tvrtko
>
>>
>> Regards,
>>
>> Nirmoy
>>
>>>       for (pmu = &free_list[0]; *pmu; pmu++) {
>>>           if ((*pmu)->present)
>>>               free((char *)(*pmu)->units);
>>> @@ -2568,7 +2571,7 @@ int main(int argc, char **argv)
>>>               "Failed to detect engines! (%s)\n(Kernel 4.16 or newer 
>>> is required for i915 PMU support.)\n",
>>>               strerror(errno));
>>>           ret = EXIT_FAILURE;
>>> -        goto err;
>>> +        goto err_engines;
>>>       }
>>>       ret = pmu_init(engines);
>>> @@ -2585,7 +2588,7 @@ int main(int argc, char **argv)
>>>   "More information can be found at 'Perf events and tool security' 
>>> document:\n"
>>> "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
>>>           ret = EXIT_FAILURE;
>>> -        goto err;
>>> +        goto err_pmu;
>>>       }
>>>       ret = EXIT_SUCCESS;
>>> @@ -2699,8 +2702,9 @@ int main(int argc, char **argv)
>>>           free_clients(clients);
>>>       free(codename);
>>> -err:
>>> +err_pmu:
>>>       free_engines(engines);
>>> +err_engines:
>>>       free(pmu_device);
>>>   exit:
>>>       igt_devices_free();
Nirmoy Das Jan. 23, 2023, 11:13 a.m. UTC | #4 
On 1/23/2023 12:09 PM, Das, Nirmoy wrote:
> Hi Tvrtko,
>
> On 1/23/2023 11:24 AM, Tvrtko Ursulin wrote:
>>
>> On 20/01/2023 14:29, Das, Nirmoy wrote:
>>> Hi Tvrtko,
>>>
>>> On 1/19/2023 4:54 PM, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Avoid trying to dereference null engines on exit when there are either
>>>> none which are supported, or kernel does not have i915 PMU support.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> ---
>>>>   tools/intel_gpu_top.c | 10 +++++++---
>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>>>> index 7aa233570463..517dc2995d26 100644
>>>> --- a/tools/intel_gpu_top.c
>>>> +++ b/tools/intel_gpu_top.c
>>>> @@ -448,6 +448,9 @@ static void free_engines(struct engines *engines)
>>>>       };
>>>>       unsigned int i;
>>>> +    if (!engines)
>>>> +        return;
>>>> +
>>>
>>>
>>> This check should happen before the  struct pmu_counter free_list 
>>> generation.
>>
>> I could, but it doesn't have to, not sure what reasons for should you 
>> have in mind?
>
> In my tree, I see:
>
> "
>
> static void free_engines(struct engines *engines)
>
> {
>         struct pmu_counter **pmu, *free_list[] = {
>                 &engines->r_gpu,
>                 &engines->r_pkg,
>                 &engines->imc_reads,
>                 &engines->imc_writes,
>                 NULL
>         };
>
>         unsigned int i;
>
> "
>
> Maybe I am missing something here, wouldn't those dereferences of 
> "engines" segfault if the engines  is NULL.


Ah never mind it shouldn't matter.

>
>
> Regards,
>
> Nirmoy
>
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Regards,
>>>
>>> Nirmoy
>>>
>>>>       for (pmu = &free_list[0]; *pmu; pmu++) {
>>>>           if ((*pmu)->present)
>>>>               free((char *)(*pmu)->units);
>>>> @@ -2568,7 +2571,7 @@ int main(int argc, char **argv)
>>>>               "Failed to detect engines! (%s)\n(Kernel 4.16 or 
>>>> newer is required for i915 PMU support.)\n",
>>>>               strerror(errno));
>>>>           ret = EXIT_FAILURE;
>>>> -        goto err;
>>>> +        goto err_engines;
>>>>       }
>>>>       ret = pmu_init(engines);
>>>> @@ -2585,7 +2588,7 @@ int main(int argc, char **argv)
>>>>   "More information can be found at 'Perf events and tool security' 
>>>> document:\n"
>>>> "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n"); 
>>>>
>>>>           ret = EXIT_FAILURE;
>>>> -        goto err;
>>>> +        goto err_pmu;
>>>>       }
>>>>       ret = EXIT_SUCCESS;
>>>> @@ -2699,8 +2702,9 @@ int main(int argc, char **argv)
>>>>           free_clients(clients);
>>>>       free(codename);
>>>> -err:
>>>> +err_pmu:
>>>>       free_engines(engines);
>>>> +err_engines:
>>>>       free(pmu_device);
>>>>   exit:
>>>>       igt_devices_free();
Nirmoy Das Jan. 23, 2023, 11:15 a.m. UTC | #5 
On 1/19/2023 4:54 PM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Avoid trying to dereference null engines on exit when there are either
> none which are supported, or kernel does not have i915 PMU support.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Acked-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   tools/intel_gpu_top.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> index 7aa233570463..517dc2995d26 100644
> --- a/tools/intel_gpu_top.c
> +++ b/tools/intel_gpu_top.c
> @@ -448,6 +448,9 @@ static void free_engines(struct engines *engines)
>   	};
>   	unsigned int i;
>   
> +	if (!engines)
> +		return;
> +
>   	for (pmu = &free_list[0]; *pmu; pmu++) {
>   		if ((*pmu)->present)
>   			free((char *)(*pmu)->units);
> @@ -2568,7 +2571,7 @@ int main(int argc, char **argv)
>   			"Failed to detect engines! (%s)\n(Kernel 4.16 or newer is required for i915 PMU support.)\n",
>   			strerror(errno));
>   		ret = EXIT_FAILURE;
> -		goto err;
> +		goto err_engines;
>   	}
>   
>   	ret = pmu_init(engines);
> @@ -2585,7 +2588,7 @@ int main(int argc, char **argv)
>   "More information can be found at 'Perf events and tool security' document:\n"
>   "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
>   		ret = EXIT_FAILURE;
> -		goto err;
> +		goto err_pmu;
>   	}
>   
>   	ret = EXIT_SUCCESS;
> @@ -2699,8 +2702,9 @@ int main(int argc, char **argv)
>   		free_clients(clients);
>   
>   	free(codename);
> -err:
> +err_pmu:
>   	free_engines(engines);
> +err_engines:
>   	free(pmu_device);
>   exit:
>   	igt_devices_free();
diff mbox series

Patch

diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 7aa233570463..517dc2995d26 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -448,6 +448,9 @@  static void free_engines(struct engines *engines)
 	};
 	unsigned int i;
 
+	if (!engines)
+		return;
+
 	for (pmu = &free_list[0]; *pmu; pmu++) {
 		if ((*pmu)->present)
 			free((char *)(*pmu)->units);
@@ -2568,7 +2571,7 @@  int main(int argc, char **argv)
 			"Failed to detect engines! (%s)\n(Kernel 4.16 or newer is required for i915 PMU support.)\n",
 			strerror(errno));
 		ret = EXIT_FAILURE;
-		goto err;
+		goto err_engines;
 	}
 
 	ret = pmu_init(engines);
@@ -2585,7 +2588,7 @@  int main(int argc, char **argv)
 "More information can be found at 'Perf events and tool security' document:\n"
 "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
 		ret = EXIT_FAILURE;
-		goto err;
+		goto err_pmu;
 	}
 
 	ret = EXIT_SUCCESS;
@@ -2699,8 +2702,9 @@  int main(int argc, char **argv)
 		free_clients(clients);
 
 	free(codename);
-err:
+err_pmu:
 	free_engines(engines);
+err_engines:
 	free(pmu_device);
 exit:
 	igt_devices_free();