diff mbox series

[v3] perf data convert: Fix segfault when converting to json on arm64

Message ID 20240117215101.77713-1-ilkka@os.amperecomputing.com
State New
Headers
Series [v3] perf data convert: Fix segfault when converting to json on arm64 |

Commit Message

Ilkka Koskinen Jan. 17, 2024, 9:51 p.m. UTC 
  Arm64 doesn't have Model in /proc/cpuinfo and, thus, cpu_desc doesn't get
assigned.

Running
	$ perf data convert --to-json perf.data.json

ends up calling output_json_string() with NULL pointer, which causes a
segmentation fault.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
v1:
    - https://lore.kernel.org/all/20240111232923.8138-1-ilkka@os.amperecomputing.com/
v2:
    - Changed the patch based on James's comments.
v3:
    - The architecture is checked from the actual data file to allow one to do
      conversion on another system. (thanks to James for the feedback)
---
tools/perf/util/data-convert-json.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Ilkka Koskinen Feb. 22, 2024, 9:42 p.m. UTC | #1 
cc: Evgeny Pistun since he submitted a patch pretty similar to my first 
version
(https://lore.kernel.org/all/20240125184411.30757-1-kotborealis@awooo.ru/)



Namhyung and James,

What's your thought on this? Is one of the patches (Evgeny's or mine) 
good enough or should we try some other approach?

Cheers, Ilkka


On Wed, 17 Jan 2024, Ilkka Koskinen wrote:
> Arm64 doesn't have Model in /proc/cpuinfo and, thus, cpu_desc doesn't get
> assigned.
>
> Running
> 	$ perf data convert --to-json perf.data.json
>
> ends up calling output_json_string() with NULL pointer, which causes a
> segmentation fault.
>
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> ---
> v1:
>    - https://lore.kernel.org/all/20240111232923.8138-1-ilkka@os.amperecomputing.com/
> v2:
>    - Changed the patch based on James's comments.
> v3:
>    - The architecture is checked from the actual data file to allow one to do
>      conversion on another system. (thanks to James for the feedback)
> ---
> tools/perf/util/data-convert-json.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/data-convert-json.c b/tools/perf/util/data-convert-json.c
> index 5bb3c2ba95ca..405c38371870 100644
> --- a/tools/perf/util/data-convert-json.c
> +++ b/tools/perf/util/data-convert-json.c
> @@ -284,7 +284,13 @@ static void output_headers(struct perf_session *session, struct convert_json *c)
> 	output_json_key_string(out, true, 2, "os-release", header->env.os_release);
> 	output_json_key_string(out, true, 2, "arch", header->env.arch);
>
> -	output_json_key_string(out, true, 2, "cpu-desc", header->env.cpu_desc);
> +	/*
> +	 * Arm64 doesn't have Model section in /proc/cpuinfo and, thus, cpu-desc
> +	 * is not set.
> +	 */
> +	if (strncmp(header->env.arch, "aarch64", 7))
> +		output_json_key_string(out, true, 2, "cpu-desc", header->env.cpu_desc);
> +
> 	output_json_key_string(out, true, 2, "cpuid", header->env.cpuid);
> 	output_json_key_format(out, true, 2, "nrcpus-online", "%u", header->env.nr_cpus_online);
> 	output_json_key_format(out, true, 2, "nrcpus-avail", "%u", header->env.nr_cpus_avail);
> -- 
> 2.43.0
>
>
Namhyung Kim Feb. 23, 2024, 7:12 a.m. UTC | #2 
Hello,

On Thu, Feb 22, 2024 at 1:42 PM Ilkka Koskinen
<ilkka@os.amperecomputing.com> wrote:
>
>
> cc: Evgeny Pistun since he submitted a patch pretty similar to my first
> version
> (https://lore.kernel.org/all/20240125184411.30757-1-kotborealis@awooo.ru/)
>
>
>
> Namhyung and James,
>
> What's your thought on this? Is one of the patches (Evgeny's or mine)
> good enough or should we try some other approach?

Sorry for the long delay.  Please see my comments below.

>
>
> On Wed, 17 Jan 2024, Ilkka Koskinen wrote:
> > Arm64 doesn't have Model in /proc/cpuinfo and, thus, cpu_desc doesn't get
> > assigned.
> >
> > Running
> >       $ perf data convert --to-json perf.data.json
> >
> > ends up calling output_json_string() with NULL pointer, which causes a
> > segmentation fault.
> >
> > Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> > ---
> > v1:
> >    - https://lore.kernel.org/all/20240111232923.8138-1-ilkka@os.amperecomputing.com/
> > v2:
> >    - Changed the patch based on James's comments.
> > v3:
> >    - The architecture is checked from the actual data file to allow one to do
> >      conversion on another system. (thanks to James for the feedback)
> > ---
> > tools/perf/util/data-convert-json.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/data-convert-json.c b/tools/perf/util/data-convert-json.c
> > index 5bb3c2ba95ca..405c38371870 100644
> > --- a/tools/perf/util/data-convert-json.c
> > +++ b/tools/perf/util/data-convert-json.c
> > @@ -284,7 +284,13 @@ static void output_headers(struct perf_session *session, struct convert_json *c)
> >       output_json_key_string(out, true, 2, "os-release", header->env.os_release);
> >       output_json_key_string(out, true, 2, "arch", header->env.arch);
> >
> > -     output_json_key_string(out, true, 2, "cpu-desc", header->env.cpu_desc);
> > +     /*
> > +      * Arm64 doesn't have Model section in /proc/cpuinfo and, thus, cpu-desc
> > +      * is not set.
> > +      */
> > +     if (strncmp(header->env.arch, "aarch64", 7))
> > +             output_json_key_string(out, true, 2, "cpu-desc", header->env.cpu_desc);

I prefer checking cpu_desc (if it's NULL) not the arch string.
Maybe other arch has the same problem or can introduce one later..

Thanks,
Namhyung

> > +
> >       output_json_key_string(out, true, 2, "cpuid", header->env.cpuid);
> >       output_json_key_format(out, true, 2, "nrcpus-online", "%u", header->env.nr_cpus_online);
> >       output_json_key_format(out, true, 2, "nrcpus-avail", "%u", header->env.nr_cpus_avail);
> > --
> > 2.43.0
> >
> >
Ilkka Koskinen Feb. 23, 2024, 8:25 p.m. UTC | #3 
On Fri, 23 Feb 2024, Namhyung Kim wrote:
> Hello,
>
> On Thu, Feb 22, 2024 at 1:42 PM Ilkka Koskinen
> <ilkka@os.amperecomputing.com> wrote:
>>
>>
>> cc: Evgeny Pistun since he submitted a patch pretty similar to my first
>> version
>> (https://lore.kernel.org/all/20240125184411.30757-1-kotborealis@awooo.ru/)
>>
>>
>>
>> Namhyung and James,
>>
>> What's your thought on this? Is one of the patches (Evgeny's or mine)
>> good enough or should we try some other approach?
>
> Sorry for the long delay.  Please see my comments below.
>
>>
>>
>> On Wed, 17 Jan 2024, Ilkka Koskinen wrote:
>>> Arm64 doesn't have Model in /proc/cpuinfo and, thus, cpu_desc doesn't get
>>> assigned.
>>>
>>> Running
>>>       $ perf data convert --to-json perf.data.json
>>>
>>> ends up calling output_json_string() with NULL pointer, which causes a
>>> segmentation fault.
>>>
>>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>>> ---
>>> v1:
>>>    - https://lore.kernel.org/all/20240111232923.8138-1-ilkka@os.amperecomputing.com/
>>> v2:
>>>    - Changed the patch based on James's comments.
>>> v3:
>>>    - The architecture is checked from the actual data file to allow one to do
>>>      conversion on another system. (thanks to James for the feedback)
>>> ---
>>> tools/perf/util/data-convert-json.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/util/data-convert-json.c b/tools/perf/util/data-convert-json.c
>>> index 5bb3c2ba95ca..405c38371870 100644
>>> --- a/tools/perf/util/data-convert-json.c
>>> +++ b/tools/perf/util/data-convert-json.c
>>> @@ -284,7 +284,13 @@ static void output_headers(struct perf_session *session, struct convert_json *c)
>>>       output_json_key_string(out, true, 2, "os-release", header->env.os_release);
>>>       output_json_key_string(out, true, 2, "arch", header->env.arch);
>>>
>>> -     output_json_key_string(out, true, 2, "cpu-desc", header->env.cpu_desc);
>>> +     /*
>>> +      * Arm64 doesn't have Model section in /proc/cpuinfo and, thus, cpu-desc
>>> +      * is not set.
>>> +      */
>>> +     if (strncmp(header->env.arch, "aarch64", 7))
>>> +             output_json_key_string(out, true, 2, "cpu-desc", header->env.cpu_desc);
>
> I prefer checking cpu_desc (if it's NULL) not the arch string.
> Maybe other arch has the same problem or can introduce one later..

Sounds reasonable. I'll submit a new version soon

Cheers, Ilkka



>
> Thanks,
> Namhyung
>
>>> +
>>>       output_json_key_string(out, true, 2, "cpuid", header->env.cpuid);
>>>       output_json_key_format(out, true, 2, "nrcpus-online", "%u", header->env.nr_cpus_online);
>>>       output_json_key_format(out, true, 2, "nrcpus-avail", "%u", header->env.nr_cpus_avail);
>>> --
>>> 2.43.0
>>>
>>>
>
diff mbox series

Patch

diff --git a/tools/perf/util/data-convert-json.c b/tools/perf/util/data-convert-json.c
index 5bb3c2ba95ca..405c38371870 100644
--- a/tools/perf/util/data-convert-json.c
+++ b/tools/perf/util/data-convert-json.c
@@ -284,7 +284,13 @@  static void output_headers(struct perf_session *session, struct convert_json *c)
 	output_json_key_string(out, true, 2, "os-release", header->env.os_release);
 	output_json_key_string(out, true, 2, "arch", header->env.arch);
 
-	output_json_key_string(out, true, 2, "cpu-desc", header->env.cpu_desc);
+	/*
+	 * Arm64 doesn't have Model section in /proc/cpuinfo and, thus, cpu-desc
+	 * is not set.
+	 */
+	if (strncmp(header->env.arch, "aarch64", 7))
+		output_json_key_string(out, true, 2, "cpu-desc", header->env.cpu_desc);
+
 	output_json_key_string(out, true, 2, "cpuid", header->env.cpuid);
 	output_json_key_format(out, true, 2, "nrcpus-online", "%u", header->env.nr_cpus_online);
 	output_json_key_format(out, true, 2, "nrcpus-avail", "%u", header->env.nr_cpus_avail);