[PATCH 1/2] perf: Fix failure to set cpumask when only one cpu

[PATCH 1/2] perf: Fix failure to set cpumask when only one cpu

[zhe.he]

From: He Zhe <zhe.he@windriver.com>

The buffer containing string used to set cpumask is overwritten by end of
string later in cpu_map__snprint_mask due to not enough memory space, when
there is only one cpu. And thus causes the following failure.

$ perf ftrace ls
failed to reset ftrace

This patch fixes the calculation of cpumask string size.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 tools/perf/builtin-ftrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 66d5a66..0193128 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -173,7 +173,7 @@ static int set_tracing_cpumask(struct cpu_map *cpumap)
 	int last_cpu;
 
 	last_cpu = cpu_map__cpu(cpumap, cpumap->nr - 1);
-	mask_size = (last_cpu + 3) / 4 + 1;
+	mask_size = last_cpu / 4 + 2; /* one more byte for EOS */
 	mask_size += last_cpu / 32; /* ',' is needed for every 32th cpus */
 
 	cpumask = malloc(mask_size);
-- 
2.7.4

[PATCH 2/2] perf: Fix writing to illegal memory in handling cpumap mask

[zhe.he]

From: He Zhe <zhe.he@windriver.com>

cpu_map__snprint_mask would write to illegal memory pointed by zalloc(0)
when there is only one cpu.

This patch fixes the calculation and adds sanity check against the input
parameters.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 tools/perf/util/cpumap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 3acfbe3..39cce66 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -751,7 +751,10 @@ size_t cpu_map__snprint_mask(struct cpu_map *map, char *buf, size_t size)
 	unsigned char *bitmap;
 	int last_cpu = cpu_map__cpu(map, map->nr - 1);
 
-	bitmap = zalloc((last_cpu + 7) / 8);
+	if (buf == NULL)
+		return 0;
+
+	bitmap = zalloc(last_cpu / 8 + 1);
 	if (bitmap == NULL) {
 		buf[0] = '\0';
 		return 0;
-- 
2.7.4

Re: [PATCH 1/2] perf: Fix failure to set cpumask when only one cpu

[Jiri Olsa]

On Fri, Aug 02, 2019 at 04:29:51PM +0800, zhe.he@windriver.com wrote:
> From: He Zhe <zhe.he@windriver.com>
> 
> The buffer containing string used to set cpumask is overwritten by end of
> string later in cpu_map__snprint_mask due to not enough memory space, when
> there is only one cpu. And thus causes the following failure.
> 
> $ perf ftrace ls
> failed to reset ftrace
> 
> This patch fixes the calculation of cpumask string size.
> 
> Signed-off-by: He Zhe <zhe.he@windriver.com>
> ---
>  tools/perf/builtin-ftrace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 66d5a66..0193128 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -173,7 +173,7 @@ static int set_tracing_cpumask(struct cpu_map *cpumap)
>  	int last_cpu;
>  
>  	last_cpu = cpu_map__cpu(cpumap, cpumap->nr - 1);
> -	mask_size = (last_cpu + 3) / 4 + 1;
> +	mask_size = last_cpu / 4 + 2; /* one more byte for EOS */
>  	mask_size += last_cpu / 32; /* ',' is needed for every 32th cpus */

ugh..  why do we care about last_cpu value in here at all?

feels like using static buffer would be more reasonable

jirka

>  
>  	cpumask = malloc(mask_size);
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] perf: Fix failure to set cpumask when only one cpu

[He Zhe]

On 8/2/19 9:06 PM, Jiri Olsa wrote:
> On Fri, Aug 02, 2019 at 04:29:51PM +0800, zhe.he@windriver.com wrote:
>> From: He Zhe <zhe.he@windriver.com>
>>
>> The buffer containing string used to set cpumask is overwritten by end of
>> string later in cpu_map__snprint_mask due to not enough memory space, when
>> there is only one cpu. And thus causes the following failure.
>>
>> $ perf ftrace ls
>> failed to reset ftrace
>>
>> This patch fixes the calculation of cpumask string size.
>>
>> Signed-off-by: He Zhe <zhe.he@windriver.com>
>> ---
>>  tools/perf/builtin-ftrace.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
>> index 66d5a66..0193128 100644
>> --- a/tools/perf/builtin-ftrace.c
>> +++ b/tools/perf/builtin-ftrace.c
>> @@ -173,7 +173,7 @@ static int set_tracing_cpumask(struct cpu_map *cpumap)
>>  	int last_cpu;
>>  
>>  	last_cpu = cpu_map__cpu(cpumap, cpumap->nr - 1);
>> -	mask_size = (last_cpu + 3) / 4 + 1;
>> +	mask_size = last_cpu / 4 + 2; /* one more byte for EOS */
>>  	mask_size += last_cpu / 32; /* ',' is needed for every 32th cpus */
> ugh..  why do we care about last_cpu value in here at all?
>
> feels like using static buffer would be more reasonable

Thanks, and yes, a static buffer would be easy to handle. A 2KB buffer is enough
to cover 8196 cpus, the maximum numbers of cpus we can run with for now.

Let's see if there is any other concerns.

Zhe

>
> jirka
>
>>  
>>  	cpumask = malloc(mask_size);
>> -- 
>> 2.7.4
>>

Re: [PATCH 1/2] perf: Fix failure to set cpumask when only one cpu

[Arnaldo Carvalho de Melo]

Em Fri, Aug 02, 2019 at 04:29:51PM +0800, zhe.he@windriver.com escreveu:
> From: He Zhe <zhe.he@windriver.com>
> 
> The buffer containing string used to set cpumask is overwritten by end of
> string later in cpu_map__snprint_mask due to not enough memory space, when
> there is only one cpu. And thus causes the following failure.
> 
> $ perf ftrace ls
> failed to reset ftrace
> 
> This patch fixes the calculation of cpumask string size.

Please next time add this as well:

Fixes: dc23103278c5 ("perf ftrace: Add support for -a and -C option")

Applied,

- Arnaldo
 
> Signed-off-by: He Zhe <zhe.he@windriver.com>
> ---
>  tools/perf/builtin-ftrace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 66d5a66..0193128 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -173,7 +173,7 @@ static int set_tracing_cpumask(struct cpu_map *cpumap)
>  	int last_cpu;
>  
>  	last_cpu = cpu_map__cpu(cpumap, cpumap->nr - 1);
> -	mask_size = (last_cpu + 3) / 4 + 1;
> +	mask_size = last_cpu / 4 + 2; /* one more byte for EOS */
>  	mask_size += last_cpu / 32; /* ',' is needed for every 32th cpus */
>  
>  	cpumask = malloc(mask_size);
> -- 
> 2.7.4

-- 

- Arnaldo

Re: [PATCH 2/2] perf: Fix writing to illegal memory in handling cpumap mask

[Arnaldo Carvalho de Melo]

Em Fri, Aug 02, 2019 at 04:29:52PM +0800, zhe.he@windriver.com escreveu:
> From: He Zhe <zhe.he@windriver.com>
> 
> cpu_map__snprint_mask would write to illegal memory pointed by zalloc(0)
> when there is only one cpu.
> 
> This patch fixes the calculation and adds sanity check against the input
> parameters.

Thanks, applied, and added the missing:

Fixes: 4400ac8a9a90 ("perf cpumap: Introduce cpu_map__snprint_mask()")

- Arnaldo
 
> Signed-off-by: He Zhe <zhe.he@windriver.com>
> ---
>  tools/perf/util/cpumap.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index 3acfbe3..39cce66 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -751,7 +751,10 @@ size_t cpu_map__snprint_mask(struct cpu_map *map, char *buf, size_t size)
>  	unsigned char *bitmap;
>  	int last_cpu = cpu_map__cpu(map, map->nr - 1);
>  
> -	bitmap = zalloc((last_cpu + 7) / 8);
> +	if (buf == NULL)
> +		return 0;
> +
> +	bitmap = zalloc(last_cpu / 8 + 1);
>  	if (bitmap == NULL) {
>  		buf[0] = '\0';
>  		return 0;
> -- 
> 2.7.4

-- 

- Arnaldo

[tip:perf/urgent] perf ftrace: Fix failure to set cpumask when only one cpu is present

[tip-bot for He Zhe]

Commit-ID:  cf30ae726c011e0372fd4c2d588466c8b50a8907
Gitweb:     https://git.kernel.org/tip/cf30ae726c011e0372fd4c2d588466c8b50a8907
Author:     He Zhe <zhe.he@windriver.com>
AuthorDate: Fri, 2 Aug 2019 16:29:51 +0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 8 Aug 2019 15:41:10 -0300

perf ftrace: Fix failure to set cpumask when only one cpu is present

The buffer containing the string used to set cpumask is overwritten at
the end of the string later in cpu_map__snprint_mask due to not enough
memory space, when there is only one cpu.

And thus causes the following failure:

  $ perf ftrace ls
  failed to reset ftrace
  $

This patch fixes the calculation of the cpumask string size.

Signed-off-by: He Zhe <zhe.he@windriver.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Fixes: dc23103278c5 ("perf ftrace: Add support for -a and -C option")
Link: http://lkml.kernel.org/r/1564734592-15624-1-git-send-email-zhe.he@windriver.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-ftrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 66d5a6658daf..019312810405 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -173,7 +173,7 @@ static int set_tracing_cpumask(struct cpu_map *cpumap)
 	int last_cpu;
 
 	last_cpu = cpu_map__cpu(cpumap, cpumap->nr - 1);
-	mask_size = (last_cpu + 3) / 4 + 1;
+	mask_size = last_cpu / 4 + 2; /* one more byte for EOS */
 	mask_size += last_cpu / 32; /* ',' is needed for every 32th cpus */
 
 	cpumask = malloc(mask_size);

[tip:perf/urgent] perf cpumap: Fix writing to illegal memory in handling cpumap mask

[tip-bot for He Zhe]

Commit-ID:  5f5e25f1c7933a6e1673515c0b1d5acd82fea1ed
Gitweb:     https://git.kernel.org/tip/5f5e25f1c7933a6e1673515c0b1d5acd82fea1ed
Author:     He Zhe <zhe.he@windriver.com>
AuthorDate: Fri, 2 Aug 2019 16:29:52 +0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 8 Aug 2019 15:41:10 -0300

perf cpumap: Fix writing to illegal memory in handling cpumap mask

cpu_map__snprint_mask() would write to illegal memory pointed by
zalloc(0) when there is only one cpu.

This patch fixes the calculation and adds sanity check against the input
parameters.

Signed-off-by: He Zhe <zhe.he@windriver.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Fixes: 4400ac8a9a90 ("perf cpumap: Introduce cpu_map__snprint_mask()")
Link: http://lkml.kernel.org/r/1564734592-15624-2-git-send-email-zhe.he@windriver.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/cpumap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 3acfbe34ebaf..39cce66b4ebc 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -751,7 +751,10 @@ size_t cpu_map__snprint_mask(struct cpu_map *map, char *buf, size_t size)
 	unsigned char *bitmap;
 	int last_cpu = cpu_map__cpu(map, map->nr - 1);
 
-	bitmap = zalloc((last_cpu + 7) / 8);
+	if (buf == NULL)
+		return 0;
+
+	bitmap = zalloc(last_cpu / 8 + 1);
 	if (bitmap == NULL) {
 		buf[0] = '\0';
 		return 0;