tools/power turbostat: Fix segfault and restart loop issues

[PATCH 1/3] tools/power turbostat: avoid segfault referencing fd_instr_count_percpu

Posted by David Arcari 1 week, 6 days ago

The problem is that fd_instr_count_percpu is allocated based on
the value of has_aperf. If has_aperf=0 then fd_instr_count_percpu
remains NULL. However, get_instr_count_fd() is called from
turbostat_init() based on the value of has_aperf_access.

On some VM systems has_aperf can be 0, while has_aperf_access can be
1.  In order to resolve the issue simply check for to see if
fd_instr_count_percpu is NULL and return -1 if it is.  Accordingly,
the has_aperf_access check can be removed from turbostat_init.

Signed-off-by: David Arcari <darcari@redhat.com>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 tools/power/x86/turbostat/turbostat.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index f2512d78bcbd..584b0f7f9067 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -2463,6 +2463,9 @@ static long open_perf_counter(int cpu, unsigned int type, unsigned int config, i
 
 int get_instr_count_fd(int cpu)
 {
+	if (!fd_instr_count_percpu)
+		return -1;
+
 	if (fd_instr_count_percpu[cpu])
 		return fd_instr_count_percpu[cpu];
 
@@ -10027,7 +10030,7 @@ void turbostat_init()
 	for_all_cpus(get_cpu_type, ODD_COUNTERS);
 	for_all_cpus(get_cpu_type, EVEN_COUNTERS);
 
-	if (BIC_IS_ENABLED(BIC_IPC) && has_aperf_access && get_instr_count_fd(base_cpu) != -1)
+	if (BIC_IS_ENABLED(BIC_IPC) && get_instr_count_fd(base_cpu) != -1)
 		BIC_PRESENT(BIC_IPC);
 
 	/*
-- 
2.51.0

Re: [PATCH 1/3] tools/power turbostat: avoid segfault referencing fd_instr_count_percpu

Posted by Len Brown 6 days, 7 hours ago

not your fault, but looking at this code, it seems that
get_instr_count_fd(base_cpu)
assumes that 0 is an invalid FD.  Fine, but based on that you'd think
we'd use zero for invalid
and non-zero for valid as return for the function call...

On Tue, Nov 18, 2025 at 10:58 AM David Arcari <darcari@redhat.com> wrote:
>
> The problem is that fd_instr_count_percpu is allocated based on
> the value of has_aperf. If has_aperf=0 then fd_instr_count_percpu
> remains NULL. However, get_instr_count_fd() is called from
> turbostat_init() based on the value of has_aperf_access.
>
> On some VM systems has_aperf can be 0, while has_aperf_access can be
> 1.  In order to resolve the issue simply check for to see if
> fd_instr_count_percpu is NULL and return -1 if it is.  Accordingly,
> the has_aperf_access check can be removed from turbostat_init.
>
> Signed-off-by: David Arcari <darcari@redhat.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-kernel@vger.kernel.org
> ---
>  tools/power/x86/turbostat/turbostat.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index f2512d78bcbd..584b0f7f9067 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -2463,6 +2463,9 @@ static long open_perf_counter(int cpu, unsigned int type, unsigned int config, i
>
>  int get_instr_count_fd(int cpu)
>  {
> +       if (!fd_instr_count_percpu)
> +               return -1;
> +
>         if (fd_instr_count_percpu[cpu])
>                 return fd_instr_count_percpu[cpu];
>
> @@ -10027,7 +10030,7 @@ void turbostat_init()
>         for_all_cpus(get_cpu_type, ODD_COUNTERS);
>         for_all_cpus(get_cpu_type, EVEN_COUNTERS);
>
> -       if (BIC_IS_ENABLED(BIC_IPC) && has_aperf_access && get_instr_count_fd(base_cpu) != -1)
> +       if (BIC_IS_ENABLED(BIC_IPC) && get_instr_count_fd(base_cpu) != -1)
>                 BIC_PRESENT(BIC_IPC);
>
>         /*
> --
> 2.51.0
>
>


-- 
Len Brown, Intel Open Source Technology Center

Re: [PATCH 1/3] tools/power turbostat: avoid segfault referencing fd_instr_count_percpu

Posted by David Arcari 12 hours ago

So get_instr_count_fd() calls open_perf_counter() which in turn calls 
perf_event_open() which returns the value from syscall().  From the 
documentation this seems to return -1 in the case of a failure.

Looking at get_instr_count_fd() I see:

int get_instr_count_fd(int cpu)
{
	if (fd_instr_count_percpu[cpu])
		return fd_instr_count_percpu[cpu];

	fd_instr_count_percpu[cpu] = open_perf_counter(cpu, PERF_TYPE_HARDWARE, 
PERF_COUNT_HW_INSTRUCTIONS, -1, 0);

	return fd_instr_count_percpu[cpu];
}


So open_perf_counter() is only called when fd_instr_count_percpu[cpu] is 
0.  In that case the return value is stored in 
fd_instr_count_percpu[cpu].  So in the case of an error this value would 
be -1; otherwise, it should be a valid file descriptor.  In fact, I 
don't think the function should ever return 0.

As far as I can tell fd_instr_count_percpu[] is initialized to zero so 
that  get_instr_count_fd() can discern whether or not 
open_perf_counter() needs to be called.

Am I missing something?

I do see that free_fd_instr_count_percpu() has a bug as I think the code 
should be:

		if (fd_instr_count_percpu[i] > 0)

instead of:

		if (fd_instr_count_percpu[i] != 0)


Thanks,
-DA

On 11/25/25 2:11 PM, Len Brown wrote:
> not your fault, but looking at this code, it seems that
> get_instr_count_fd(base_cpu)
> assumes that 0 is an invalid FD.  Fine, but based on that you'd think
> we'd use zero for invalid
> and non-zero for valid as return for the function call...
> 
> On Tue, Nov 18, 2025 at 10:58 AM David Arcari <darcari@redhat.com> wrote:
>>
>> The problem is that fd_instr_count_percpu is allocated based on
>> the value of has_aperf. If has_aperf=0 then fd_instr_count_percpu
>> remains NULL. However, get_instr_count_fd() is called from
>> turbostat_init() based on the value of has_aperf_access.
>>
>> On some VM systems has_aperf can be 0, while has_aperf_access can be
>> 1.  In order to resolve the issue simply check for to see if
>> fd_instr_count_percpu is NULL and return -1 if it is.  Accordingly,
>> the has_aperf_access check can be removed from turbostat_init.
>>
>> Signed-off-by: David Arcari <darcari@redhat.com>
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   tools/power/x86/turbostat/turbostat.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
>> index f2512d78bcbd..584b0f7f9067 100644
>> --- a/tools/power/x86/turbostat/turbostat.c
>> +++ b/tools/power/x86/turbostat/turbostat.c
>> @@ -2463,6 +2463,9 @@ static long open_perf_counter(int cpu, unsigned int type, unsigned int config, i
>>
>>   int get_instr_count_fd(int cpu)
>>   {
>> +       if (!fd_instr_count_percpu)
>> +               return -1;
>> +
>>          if (fd_instr_count_percpu[cpu])
>>                  return fd_instr_count_percpu[cpu];
>>
>> @@ -10027,7 +10030,7 @@ void turbostat_init()
>>          for_all_cpus(get_cpu_type, ODD_COUNTERS);
>>          for_all_cpus(get_cpu_type, EVEN_COUNTERS);
>>
>> -       if (BIC_IS_ENABLED(BIC_IPC) && has_aperf_access && get_instr_count_fd(base_cpu) != -1)
>> +       if (BIC_IS_ENABLED(BIC_IPC) && get_instr_count_fd(base_cpu) != -1)
>>                  BIC_PRESENT(BIC_IPC);
>>
>>          /*
>> --
>> 2.51.0
>>
>>
> 
>