[PATCH 0/3] Perf UBsan Patches

[PATCH 0/3] Perf UBsan Patches

[Numfor Mbiziwo-Tiapo]

These patches are all errors found by running perf test with the
ubsan (undefined behavior sanitizer version of perf).

To repro the bug fixed in the "Fix insn.c misaligned address 
error" patch you must first apply the changes in the
Fix backward-ring-buffer.c format-truncation error and
Fix ordered-events.c array-bounds error patches since these
are necessary to get the ubsan version of perf to build.

To build the ubsan version, run:
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"

Numfor Mbiziwo-Tiapo (3):
  Fix backward-ring-buffer.c format-truncation error
  Fix ordered-events.c array-bounds error
  Fix insn.c misaligned address error

 tools/perf/tests/backward-ring-buffer.c | 2 +-
 tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
 tools/perf/util/ordered-events.c        | 2 ++
 3 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.22.0.657.g960e92d24f-goog

[PATCH 1/3] Fix backward-ring-buffer.c format-truncation error

[Numfor Mbiziwo-Tiapo]

Perf does not build with the ubsan (undefined behavior sanitizer)
and there is an error that says:

tests/backward-ring-buffer.c:23:45: error: ‘%d’ directive output
may be truncated writing between 1 and 10 bytes into a region of
size 8 [-Werror=format-truncation=]
   snprintf(proc_name, sizeof(proc_name), "p:%d\n", i);

This can be reproduced by running (from the tip directory):
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"

Th error occurs because they are writing to the 10 byte buffer - the
index 'i' of the for loop and the 2 byte hardcoded string. If somehow 'i'
was greater than 8 bytes (10 - 2), then the snprintf function would
truncate the string. Increasing the size of the buffer fixes the error.

Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
---
 tools/perf/tests/backward-ring-buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/tests/backward-ring-buffer.c b/tools/perf/tests/backward-ring-buffer.c
index 6d598cc071ae..1a9c3becf5ff 100644
--- a/tools/perf/tests/backward-ring-buffer.c
+++ b/tools/perf/tests/backward-ring-buffer.c
@@ -18,7 +18,7 @@ static void testcase(void)
 	int i;
 
 	for (i = 0; i < NR_ITERS; i++) {
-		char proc_name[10];
+		char proc_name[15];
 
 		snprintf(proc_name, sizeof(proc_name), "p:%d\n", i);
 		prctl(PR_SET_NAME, proc_name);
-- 
2.22.0.657.g960e92d24f-goog

[PATCH 2/3] Fix ordered-events.c array-bounds error

[Numfor Mbiziwo-Tiapo]

Perf does not build with the ubsan (undefined behavior sanitizer)
and there is an error that says:

tools/perf/util/debug.h:38:2:
 error: array subscript is above array bounds [-Werror=array-bounds]
  eprintf_time(n, var, t, fmt, ##__VA_ARGS__)
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

tools/perf/util/debug.h:40:34:
 note: in expansion of macro ‘pr_time_N’
 #define pr_oe_time(t, fmt, ...)  pr_time_N(1, debug_ordered_events,
 t, pr_fmt(fmt), ##__VA_ARGS__)

util/ordered-events.c:329:2: note: in expansion of macro ‘pr_oe_time’
  pr_oe_time(oe->next_flush, "next_flush - ordered_events__flush
  POST %s, nr_events %u\n",

This can be reproduced by running (from the tip directory):
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"

The error stems from the 'str' array in the __ordered_events__flush
function in tools/perf/util/ordered-events.c. On line 319 of this
file, they use values of the variable 'how' (which has the type enum
oeflush - defined in ordered-events.h) as indices for the 'str' array.
Since 'how' has 5 values and the 'str' array only has 3, when the 4th
and 5th values of 'how' (OE_FLUSH__TOP and OE_FLUSH__TIME) are used as
indices, this will go out of the bounds of the 'str' array.
Adding the matching strings from the enum values into the 'str' array
fixes this.

Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
---
 tools/perf/util/ordered-events.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
index 897589507d97..c092b0c39d2b 100644
--- a/tools/perf/util/ordered-events.c
+++ b/tools/perf/util/ordered-events.c
@@ -270,6 +270,8 @@ static int __ordered_events__flush(struct ordered_events *oe, enum oe_flush how,
 		"FINAL",
 		"ROUND",
 		"HALF ",
+		"TOP",
+		"TIME",
 	};
 	int err;
 	bool show_progress = false;
-- 
2.22.0.657.g960e92d24f-goog

[PATCH 3/3] Fix insn.c misaligned address error

[Numfor Mbiziwo-Tiapo]

The ubsan (undefined behavior sanitizer) version of perf throws an
error on the 'x86 instruction decoder - new instructions' function
of perf test.

To reproduce this run:
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"

then run: tools/perf/perf test 62 -v

The error occurs in the __get_next macro (line 34) where an int is
read from a potentially unaligned address. Using memcpy instead of
assignment from an unaligned pointer.

Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
---
 tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/intel-pt-decoder/insn.c b/tools/perf/util/intel-pt-decoder/insn.c
index ca983e2bea8b..de1944c60aa9 100644
--- a/tools/perf/util/intel-pt-decoder/insn.c
+++ b/tools/perf/util/intel-pt-decoder/insn.c
@@ -31,7 +31,8 @@
 	((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
 
 #define __get_next(t, insn)	\
-	({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
+	({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
+		insn->next_byte += sizeof(t); r; })
 
 #define __peek_nbyte_next(t, insn, n)	\
 	({ t r = *(t*)((insn)->next_byte + n); r; })
-- 
2.22.0.657.g960e92d24f-goog

RE: [PATCH 3/3] Fix insn.c misaligned address error

[David Laight]

From: Numfor Mbiziwo-Tiapo
> Sent: 24 July 2019 19:45
> 
> The ubsan (undefined behavior sanitizer) version of perf throws an
> error on the 'x86 instruction decoder - new instructions' function
> of perf test.
> 
> To reproduce this run:
> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> 
> then run: tools/perf/perf test 62 -v
> 
> The error occurs in the __get_next macro (line 34) where an int is
> read from a potentially unaligned address. Using memcpy instead of
> assignment from an unaligned pointer.
...
>  #define __get_next(t, insn)	\
> -	({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> +	({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
> +		insn->next_byte += sizeof(t); r; })

Isn't there a get_unaligned_u32() (or similar) that can be used?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH 1/3] Fix backward-ring-buffer.c format-truncation error

[David Laight]

From: Numfor Mbiziwo-Tiapo
> Sent: 24 July 2019 19:45
>
> Perf does not build with the ubsan (undefined behavior sanitizer)
> and there is an error that says:
> 
> tests/backward-ring-buffer.c:23:45: error: ‘%d’ directive output
> may be truncated writing between 1 and 10 bytes into a region of
> size 8 [-Werror=format-truncation=]
>    snprintf(proc_name, sizeof(proc_name), "p:%d\n", i);
> 
> This can be reproduced by running (from the tip directory):
> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> 
> Th error occurs because they are writing to the 10 byte buffer - the
> index 'i' of the for loop and the 2 byte hardcoded string. If somehow 'i'
> was greater than 8 bytes (10 - 2), then the snprintf function would
> truncate the string. Increasing the size of the buffer fixes the error.

Get the compiler fixed so that it knows the domain of the value can never
exceed the compile-time constant NR_ITERS.

> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
>  tools/perf/tests/backward-ring-buffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/tests/backward-ring-buffer.c b/tools/perf/tests/backward-ring-buffer.c
> index 6d598cc071ae..1a9c3becf5ff 100644
> --- a/tools/perf/tests/backward-ring-buffer.c
> +++ b/tools/perf/tests/backward-ring-buffer.c
> @@ -18,7 +18,7 @@ static void testcase(void)
>  	int i;
> 
>  	for (i = 0; i < NR_ITERS; i++) {
> -		char proc_name[10];
> +		char proc_name[15];

At least use [16]

> 
>  		snprintf(proc_name, sizeof(proc_name), "p:%d\n", i);
>  		prctl(PR_SET_NAME, proc_name);
> --
> 2.22.0.657.g960e92d24f-goog

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 3/3] Fix insn.c misaligned address error

[Ian Rogers]

On Thu, Jul 25, 2019 at 6:06 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Numfor Mbiziwo-Tiapo
> > Sent: 24 July 2019 19:45
> >
> > The ubsan (undefined behavior sanitizer) version of perf throws an
> > error on the 'x86 instruction decoder - new instructions' function
> > of perf test.
> >
> > To reproduce this run:
> > make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> >
> > then run: tools/perf/perf test 62 -v
> >
> > The error occurs in the __get_next macro (line 34) where an int is
> > read from a potentially unaligned address. Using memcpy instead of
> > assignment from an unaligned pointer.
> ...
> >  #define __get_next(t, insn)  \
> > -     ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> > +     ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
> > +             insn->next_byte += sizeof(t); r; })
>
> Isn't there a get_unaligned_u32() (or similar) that can be used?


memcpy is a compiler intrinsic. get_unaligned_u32 would mean either a
'if (sizeof(t) == sizeof(u32)) get_unaligned_u32(.. ' for all sizes or
changing all call sites of __get_next. Numfor's change feels right as
it is the least invasive.

Thanks,
Ian Rogers
(resent to make plain text)

>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

Re: [PATCH 2/3] Fix ordered-events.c array-bounds error

[Arnaldo Carvalho de Melo]

Em Wed, Jul 24, 2019 at 11:45:11AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> Perf does not build with the ubsan (undefined behavior sanitizer)
> and there is an error that says:
> 
> tools/perf/util/debug.h:38:2:
>  error: array subscript is above array bounds [-Werror=array-bounds]
>   eprintf_time(n, var, t, fmt, ##__VA_ARGS__)
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> tools/perf/util/debug.h:40:34:
>  note: in expansion of macro ‘pr_time_N’
>  #define pr_oe_time(t, fmt, ...)  pr_time_N(1, debug_ordered_events,
>  t, pr_fmt(fmt), ##__VA_ARGS__)
> 
> util/ordered-events.c:329:2: note: in expansion of macro ‘pr_oe_time’
>   pr_oe_time(oe->next_flush, "next_flush - ordered_events__flush
>   POST %s, nr_events %u\n",
> 
> This can be reproduced by running (from the tip directory):
> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> 
> The error stems from the 'str' array in the __ordered_events__flush
> function in tools/perf/util/ordered-events.c. On line 319 of this
> file, they use values of the variable 'how' (which has the type enum
> oeflush - defined in ordered-events.h) as indices for the 'str' array.
> Since 'how' has 5 values and the 'str' array only has 3, when the 4th
> and 5th values of 'how' (OE_FLUSH__TOP and OE_FLUSH__TIME) are used as
> indices, this will go out of the bounds of the 'str' array.
> Adding the matching strings from the enum values into the 'str' array
> fixes this.

^[[acme@quaco perf]$ patch -p1 < /wb/1.patch
patching file tools/perf/util/ordered-events.c
patch: **** malformed patch at line 146: s *oe, enum oe_flush how,

[acme@quaco perf]$ git dif



Applying by hand
 
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
>  tools/perf/util/ordered-events.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
> index 897589507d97..c092b0c39d2b 100644
> --- a/tools/perf/util/ordered-events.c
> +++ b/tools/perf/util/ordered-events.c
> @@ -270,6 +270,8 @@ static int __ordered_events__flush(struct ordered_events *oe, enum oe_flush how,
>  		"FINAL",
>  		"ROUND",
>  		"HALF ",
> +		"TOP",
> +		"TIME",
>  	};
>  	int err;
>  	bool show_progress = false;
> -- 
> 2.22.0.657.g960e92d24f-goog

-- 

- Arnaldo

Re: [PATCH 2/3] Fix ordered-events.c array-bounds error

[Arnaldo Carvalho de Melo]

Em Wed, Jul 24, 2019 at 11:45:11AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> Perf does not build with the ubsan (undefined behavior sanitizer)
> and there is an error that says:
> 
> tools/perf/util/debug.h:38:2:
>  error: array subscript is above array bounds [-Werror=array-bounds]
>   eprintf_time(n, var, t, fmt, ##__VA_ARGS__)
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> tools/perf/util/debug.h:40:34:
>  note: in expansion of macro ‘pr_time_N’
>  #define pr_oe_time(t, fmt, ...)  pr_time_N(1, debug_ordered_events,
>  t, pr_fmt(fmt), ##__VA_ARGS__)
> 
> util/ordered-events.c:329:2: note: in expansion of macro ‘pr_oe_time’
>   pr_oe_time(oe->next_flush, "next_flush - ordered_events__flush
>   POST %s, nr_events %u\n",
> 
> This can be reproduced by running (from the tip directory):
> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> 
> The error stems from the 'str' array in the __ordered_events__flush
> function in tools/perf/util/ordered-events.c. On line 319 of this
> file, they use values of the variable 'how' (which has the type enum
> oeflush - defined in ordered-events.h) as indices for the 'str' array.
> Since 'how' has 5 values and the 'str' array only has 3, when the 4th
> and 5th values of 'how' (OE_FLUSH__TOP and OE_FLUSH__TIME) are used as
> indices, this will go out of the bounds of the 'str' array.
> Adding the matching strings from the enum values into the 'str' array
> fixes this.
> 
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
>  tools/perf/util/ordered-events.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
> index 897589507d97..c092b0c39d2b 100644
> --- a/tools/perf/util/ordered-events.c
> +++ b/tools/perf/util/ordered-events.c
> @@ -270,6 +270,8 @@ static int __ordered_events__flush(struct ordered_events *oe, enum oe_flush how,
>  		"FINAL",
>  		"ROUND",
>  		"HALF ",
> +		"TOP",
> +		"TIME",
>  	};
>  	int err;
>  	bool show_progress = false;

Humm, this was fixed already by:

commit 1e5b0cf8672e622257df024074e6e09bfbcb7750
Author: Changbin Du <changbin.du@gmail.com>
Date:   Sat Mar 16 16:05:52 2019 +0800

    perf top: Fix global-buffer-overflow issue

    The array str[] should have six elements.

      =================================================================
      ==4322==ERROR: AddressSanitizer: global-buffer-overflow on address 0x56463844e300 at pc 0x564637e7ad0d bp 0x7f30c8c89d10 sp 0x7f30c8c89d00
      READ of size 8 at 0x56463844e300 thread T9
          #0 0x564637e7ad0c in __ordered_events__flush util/ordered-events.c:316
          #1 0x564637e7b0e4 in ordered_events__flush util/ordered-events.c:338
          #2 0x564637c6a57d in process_thread /home/changbin/work/linux/tools/perf/builtin-top.c:1073
          #3 0x7f30d173a163 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8163)
          #4 0x7f30cfffbdee in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x11adee)

Re: [PATCH 3/3] Fix insn.c misaligned address error

[Arnaldo Carvalho de Melo]

Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> The ubsan (undefined behavior sanitizer) version of perf throws an
> error on the 'x86 instruction decoder - new instructions' function
> of perf test.
> 
> To reproduce this run:
> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> 
> then run: tools/perf/perf test 62 -v
> 
> The error occurs in the __get_next macro (line 34) where an int is
> read from a potentially unaligned address. Using memcpy instead of
> assignment from an unaligned pointer.

Since this came from the kernel, don't we have to fix it there as well?
Masami, Adrian?

[acme@quaco perf]$ find . -name insn.c
./arch/x86/lib/insn.c
./arch/arm/kernel/insn.c
./arch/arm64/kernel/insn.c
./tools/objtool/arch/x86/lib/insn.c
./tools/perf/util/intel-pt-decoder/insn.c
[acme@quaco perf]$ diff -u ./tools/perf/util/intel-pt-decoder/insn.c ./arch/x86/lib/insn.c
--- ./tools/perf/util/intel-pt-decoder/insn.c	2019-07-06 16:59:05.734265998 -0300
+++ ./arch/x86/lib/insn.c	2019-07-06 16:59:01.369202998 -0300
@@ -10,8 +10,8 @@
 #else
 #include <string.h>
 #endif
-#include "inat.h"
-#include "insn.h"
+#include <asm/inat.h>
+#include <asm/insn.h>

 /* Verify next sizeof(t) bytes can be on the same instruction */
 #define validate_next(t, insn, n)	\
[acme@quaco perf]$


- Arnaldo
 
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
>  tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/intel-pt-decoder/insn.c b/tools/perf/util/intel-pt-decoder/insn.c
> index ca983e2bea8b..de1944c60aa9 100644
> --- a/tools/perf/util/intel-pt-decoder/insn.c
> +++ b/tools/perf/util/intel-pt-decoder/insn.c
> @@ -31,7 +31,8 @@
>  	((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
>  
>  #define __get_next(t, insn)	\
> -	({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> +	({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
> +		insn->next_byte += sizeof(t); r; })
>  
>  #define __peek_nbyte_next(t, insn, n)	\
>  	({ t r = *(t*)((insn)->next_byte + n); r; })
> -- 
> 2.22.0.657.g960e92d24f-goog

-- 

- Arnaldo

Re: [PATCH 1/3] Fix backward-ring-buffer.c format-truncation error

[Arnaldo Carvalho de Melo]

Em Wed, Jul 24, 2019 at 11:45:10AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> Perf does not build with the ubsan (undefined behavior sanitizer)
> and there is an error that says:
> 
> tests/backward-ring-buffer.c:23:45: error: ‘%d’ directive output
> may be truncated writing between 1 and 10 bytes into a region of
> size 8 [-Werror=format-truncation=]
>    snprintf(proc_name, sizeof(proc_name), "p:%d\n", i);
> 
> This can be reproduced by running (from the tip directory):
> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> 
> Th error occurs because they are writing to the 10 byte buffer - the
> index 'i' of the for loop and the 2 byte hardcoded string. If somehow 'i'
> was greater than 8 bytes (10 - 2), then the snprintf function would
> truncate the string. Increasing the size of the buffer fixes the error.
> 
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
>  tools/perf/tests/backward-ring-buffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/tests/backward-ring-buffer.c b/tools/perf/tests/backward-ring-buffer.c
> index 6d598cc071ae..1a9c3becf5ff 100644
> --- a/tools/perf/tests/backward-ring-buffer.c
> +++ b/tools/perf/tests/backward-ring-buffer.c
> @@ -18,7 +18,7 @@ static void testcase(void)
>  	int i;
>  
>  	for (i = 0; i < NR_ITERS; i++) {
> -		char proc_name[10];
> +		char proc_name[15];
>  
>  		snprintf(proc_name, sizeof(proc_name), "p:%d\n", i);
>  		prctl(PR_SET_NAME, proc_name);

This was fixed already by:

commit 11c1ea6f1a9bc97bf857fd12f72eacb6c69794e2
Author: Changbin Du <changbin.du@gmail.com>
Date:   Sat Mar 16 16:05:43 2019 +0800

    perf tools: Fix errors under optimization level '-Og'

    Optimization level '-Og' offers a reasonable level of optimization while
    maintaining fast compilation and a good debugging experience. This patch
    tries to make it work.

      $ make DEBUG=1 EXTRA_CFLAGS='-Og'
      bench/epoll-ctl.c: In function ‘do_threads’:
      bench/epoll-ctl.c:274:9: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
        return ret;
               ^~~
      ...

    Signed-off-by: Changbin Du <changbin.du@gmail.com>
    Reviewed-by: Jiri Olsa <jolsa@kernel.org>

Re: [PATCH 3/3] Fix insn.c misaligned address error

[Masami Hiramatsu]

On Fri, 26 Jul 2019 16:38:06 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> > The ubsan (undefined behavior sanitizer) version of perf throws an
> > error on the 'x86 instruction decoder - new instructions' function
> > of perf test.
> > 
> > To reproduce this run:
> > make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> > 
> > then run: tools/perf/perf test 62 -v
> > 
> > The error occurs in the __get_next macro (line 34) where an int is
> > read from a potentially unaligned address. Using memcpy instead of
> > assignment from an unaligned pointer.
> 
> Since this came from the kernel, don't we have to fix it there as well?
> Masami, Adrian?

I guess we don't need it, since x86 can access "unaligned address" and
x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
that part. Maybe if we run it on other arch as cross-arch application,
it may cause unaligned pointer issue.

Thank you,

> 
> [acme@quaco perf]$ find . -name insn.c
> ./arch/x86/lib/insn.c
> ./arch/arm/kernel/insn.c
> ./arch/arm64/kernel/insn.c
> ./tools/objtool/arch/x86/lib/insn.c
> ./tools/perf/util/intel-pt-decoder/insn.c
> [acme@quaco perf]$ diff -u ./tools/perf/util/intel-pt-decoder/insn.c ./arch/x86/lib/insn.c
> --- ./tools/perf/util/intel-pt-decoder/insn.c	2019-07-06 16:59:05.734265998 -0300
> +++ ./arch/x86/lib/insn.c	2019-07-06 16:59:01.369202998 -0300
> @@ -10,8 +10,8 @@
>  #else
>  #include <string.h>
>  #endif
> -#include "inat.h"
> -#include "insn.h"
> +#include <asm/inat.h>
> +#include <asm/insn.h>
> 
>  /* Verify next sizeof(t) bytes can be on the same instruction */
>  #define validate_next(t, insn, n)	\
> [acme@quaco perf]$
> 
> 
> - Arnaldo
>  
> > Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> > ---
> >  tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/intel-pt-decoder/insn.c b/tools/perf/util/intel-pt-decoder/insn.c
> > index ca983e2bea8b..de1944c60aa9 100644
> > --- a/tools/perf/util/intel-pt-decoder/insn.c
> > +++ b/tools/perf/util/intel-pt-decoder/insn.c
> > @@ -31,7 +31,8 @@
> >  	((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
> >  
> >  #define __get_next(t, insn)	\
> > -	({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> > +	({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
> > +		insn->next_byte += sizeof(t); r; })
> >  
> >  #define __peek_nbyte_next(t, insn, n)	\
> >  	({ t r = *(t*)((insn)->next_byte + n); r; })
> > -- 
> > 2.22.0.657.g960e92d24f-goog
> 
> -- 
> 
> - Arnaldo


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/3] Fix insn.c misaligned address error

[Adrian Hunter]

On 27/07/19 12:46 PM, Masami Hiramatsu wrote:
> On Fri, 26 Jul 2019 16:38:06 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
>> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
>>> The ubsan (undefined behavior sanitizer) version of perf throws an
>>> error on the 'x86 instruction decoder - new instructions' function
>>> of perf test.
>>>
>>> To reproduce this run:
>>> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
>>>
>>> then run: tools/perf/perf test 62 -v
>>>
>>> The error occurs in the __get_next macro (line 34) where an int is
>>> read from a potentially unaligned address. Using memcpy instead of
>>> assignment from an unaligned pointer.
>>
>> Since this came from the kernel, don't we have to fix it there as well?
>> Masami, Adrian?
> 
> I guess we don't need it, since x86 can access "unaligned address" and
> x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
> that part. Maybe if we run it on other arch as cross-arch application,
> it may cause unaligned pointer issue.

Yes, theoretically Intel PT decoding can be done on any arch.

But the memcpy is probably sub-optimal for x86, so the patch as it stands
does not seem suitable.  I notice the kernel has get_unaligned() and
put_unaligned().

Obviously it would be better for a patch to be accepted to
arch/x86/lib/insn.c also.

> 
> Thank you,
> 
>>
>> [acme@quaco perf]$ find . -name insn.c
>> ./arch/x86/lib/insn.c
>> ./arch/arm/kernel/insn.c
>> ./arch/arm64/kernel/insn.c
>> ./tools/objtool/arch/x86/lib/insn.c
>> ./tools/perf/util/intel-pt-decoder/insn.c
>> [acme@quaco perf]$ diff -u ./tools/perf/util/intel-pt-decoder/insn.c ./arch/x86/lib/insn.c
>> --- ./tools/perf/util/intel-pt-decoder/insn.c	2019-07-06 16:59:05.734265998 -0300
>> +++ ./arch/x86/lib/insn.c	2019-07-06 16:59:01.369202998 -0300
>> @@ -10,8 +10,8 @@
>>  #else
>>  #include <string.h>
>>  #endif
>> -#include "inat.h"
>> -#include "insn.h"
>> +#include <asm/inat.h>
>> +#include <asm/insn.h>
>>
>>  /* Verify next sizeof(t) bytes can be on the same instruction */
>>  #define validate_next(t, insn, n)	\
>> [acme@quaco perf]$
>>
>>
>> - Arnaldo
>>  
>>> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
>>> ---
>>>  tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/util/intel-pt-decoder/insn.c b/tools/perf/util/intel-pt-decoder/insn.c
>>> index ca983e2bea8b..de1944c60aa9 100644
>>> --- a/tools/perf/util/intel-pt-decoder/insn.c
>>> +++ b/tools/perf/util/intel-pt-decoder/insn.c
>>> @@ -31,7 +31,8 @@
>>>  	((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
>>>  
>>>  #define __get_next(t, insn)	\
>>> -	({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
>>> +	({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
>>> +		insn->next_byte += sizeof(t); r; })
>>>  
>>>  #define __peek_nbyte_next(t, insn, n)	\
>>>  	({ t r = *(t*)((insn)->next_byte + n); r; })
>>> -- 
>>> 2.22.0.657.g960e92d24f-goog
>>
>> -- 
>>
>> - Arnaldo
> 
> 

Re: [PATCH 3/3] Fix insn.c misaligned address error

[Ian Rogers]

On Mon, Jul 29, 2019 at 1:24 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 27/07/19 12:46 PM, Masami Hiramatsu wrote:
> > On Fri, 26 Jul 2019 16:38:06 -0300
> > Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> >
> >> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> >>> The ubsan (undefined behavior sanitizer) version of perf throws an
> >>> error on the 'x86 instruction decoder - new instructions' function
> >>> of perf test.
> >>>
> >>> To reproduce this run:
> >>> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> >>>
> >>> then run: tools/perf/perf test 62 -v
> >>>
> >>> The error occurs in the __get_next macro (line 34) where an int is
> >>> read from a potentially unaligned address. Using memcpy instead of
> >>> assignment from an unaligned pointer.
> >>
> >> Since this came from the kernel, don't we have to fix it there as well?
> >> Masami, Adrian?
> >
> > I guess we don't need it, since x86 can access "unaligned address" and
> > x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
> > that part. Maybe if we run it on other arch as cross-arch application,
> > it may cause unaligned pointer issue.

http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
"A pointer to an object or incomplete type may be converted to a
pointer to a different object or incomplete type. If the resulting
pointer is not correctly aligned for the pointed-to type, the behavior
is undefined."
I agree the code will generally run on x86.

> Yes, theoretically Intel PT decoding can be done on any arch.
>
> But the memcpy is probably sub-optimal for x86, so the patch as it stands
> does not seem suitable.  I notice the kernel has get_unaligned() and
> put_unaligned().

Why is a fixed sized memcpy suboptimal? The compiler can should turn
into a load.

Thanks,
Ian

> Obviously it would be better for a patch to be accepted to
> arch/x86/lib/insn.c also.
>
> >
> > Thank you,
> >
> >>
> >> [acme@quaco perf]$ find . -name insn.c
> >> ./arch/x86/lib/insn.c
> >> ./arch/arm/kernel/insn.c
> >> ./arch/arm64/kernel/insn.c
> >> ./tools/objtool/arch/x86/lib/insn.c
> >> ./tools/perf/util/intel-pt-decoder/insn.c
> >> [acme@quaco perf]$ diff -u ./tools/perf/util/intel-pt-decoder/insn.c ./arch/x86/lib/insn.c
> >> --- ./tools/perf/util/intel-pt-decoder/insn.c        2019-07-06 16:59:05.734265998 -0300
> >> +++ ./arch/x86/lib/insn.c    2019-07-06 16:59:01.369202998 -0300
> >> @@ -10,8 +10,8 @@
> >>  #else
> >>  #include <string.h>
> >>  #endif
> >> -#include "inat.h"
> >> -#include "insn.h"
> >> +#include <asm/inat.h>
> >> +#include <asm/insn.h>
> >>
> >>  /* Verify next sizeof(t) bytes can be on the same instruction */
> >>  #define validate_next(t, insn, n)   \
> >> [acme@quaco perf]$
> >>
> >>
> >> - Arnaldo
> >>
> >>> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> >>> ---
> >>>  tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/perf/util/intel-pt-decoder/insn.c b/tools/perf/util/intel-pt-decoder/insn.c
> >>> index ca983e2bea8b..de1944c60aa9 100644
> >>> --- a/tools/perf/util/intel-pt-decoder/insn.c
> >>> +++ b/tools/perf/util/intel-pt-decoder/insn.c
> >>> @@ -31,7 +31,8 @@
> >>>     ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
> >>>
> >>>  #define __get_next(t, insn)        \
> >>> -   ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> >>> +   ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
> >>> +           insn->next_byte += sizeof(t); r; })
> >>>
> >>>  #define __peek_nbyte_next(t, insn, n)      \
> >>>     ({ t r = *(t*)((insn)->next_byte + n); r; })
> >>> --
> >>> 2.22.0.657.g960e92d24f-goog
> >>
> >> --
> >>
> >> - Arnaldo
> >
> >
>

[PATCH v2] Fix annotate.c use of uninitialized value error

[Numfor Mbiziwo-Tiapo]

Our local MSAN (Memory Sanitizer) build of perf throws a warning
that comes from the "dso__disassemble_filename" function in
"tools/perf/util/annotate.c" when running perf record.

The warning stems from the call to readlink, in which "build_id_path"
was being read into "linkname". Since readlink does not null terminate,
an uninitialized memory access would later occur when "linkname" is
passed into the strstr function. This is simply fixed by null-terminating
"linkname" after the call to readlink.

To reproduce this warning, build perf by running:
make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\
 -fsanitize-memory-track-origins"

(Additionally, llvm might have to be installed and clang might have to
be specified as the compiler - export CC=/usr/bin/clang)

then running:
tools/perf/perf record -o - ls / | tools/perf/perf --no-pager annotate\
 -i - --stdio

Please see the cover letter for why false positive warnings may be
generated.

Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
---
 tools/perf/util/annotate.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 70de8f6b3aee..e1b075b52dce 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1627,6 +1627,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	char *build_id_filename;
 	char *build_id_path = NULL;
 	char *pos;
+	int len;
 
 	if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS &&
 	    !dso__is_kcore(dso))
@@ -1655,10 +1656,16 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
 		dirname(build_id_path);
 
-	if (dso__is_kcore(dso) ||
-	    readlink(build_id_path, linkname, sizeof(linkname)) < 0 ||
-	    strstr(linkname, DSO__NAME_KALLSYMS) ||
-	    access(filename, R_OK)) {
+	if (dso__is_kcore(dso))
+		goto fallback;
+
+	len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
+	if (len < 0)
+		goto fallback;
+
+	linkname[len] = '\0';
+	if (strstr(linkname, DSO__NAME_KALLSYMS) ||
+		access(filename, R_OK)) {
 fallback:
 		/*
 		 * If we don't have build-ids or the build-id file isn't in the
-- 
2.22.0.709.g102302147b-goog

Re: [PATCH 3/3] Fix insn.c misaligned address error

[Masami Hiramatsu]

On Mon, 29 Jul 2019 11:22:34 +0300
Adrian Hunter <adrian.hunter@intel.com> wrote:

> On 27/07/19 12:46 PM, Masami Hiramatsu wrote:
> > On Fri, 26 Jul 2019 16:38:06 -0300
> > Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > 
> >> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> >>> The ubsan (undefined behavior sanitizer) version of perf throws an
> >>> error on the 'x86 instruction decoder - new instructions' function
> >>> of perf test.
> >>>
> >>> To reproduce this run:
> >>> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> >>>
> >>> then run: tools/perf/perf test 62 -v
> >>>
> >>> The error occurs in the __get_next macro (line 34) where an int is
> >>> read from a potentially unaligned address. Using memcpy instead of
> >>> assignment from an unaligned pointer.
> >>
> >> Since this came from the kernel, don't we have to fix it there as well?
> >> Masami, Adrian?
> > 
> > I guess we don't need it, since x86 can access "unaligned address" and
> > x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
> > that part. Maybe if we run it on other arch as cross-arch application,
> > it may cause unaligned pointer issue.
> 
> Yes, theoretically Intel PT decoding can be done on any arch.
> 
> But the memcpy is probably sub-optimal for x86, so the patch as it stands
> does not seem suitable.  I notice the kernel has get_unaligned() and
> put_unaligned().
> 
> Obviously it would be better for a patch to be accepted to
> arch/x86/lib/insn.c also.

Hmm, then I rather like memcpy() for arch/x86/lib/insn.c, because it runs only
on x86.

Thank you,

> 
> > 
> > Thank you,
> > 
> >>
> >> [acme@quaco perf]$ find . -name insn.c
> >> ./arch/x86/lib/insn.c
> >> ./arch/arm/kernel/insn.c
> >> ./arch/arm64/kernel/insn.c
> >> ./tools/objtool/arch/x86/lib/insn.c
> >> ./tools/perf/util/intel-pt-decoder/insn.c
> >> [acme@quaco perf]$ diff -u ./tools/perf/util/intel-pt-decoder/insn.c ./arch/x86/lib/insn.c
> >> --- ./tools/perf/util/intel-pt-decoder/insn.c	2019-07-06 16:59:05.734265998 -0300
> >> +++ ./arch/x86/lib/insn.c	2019-07-06 16:59:01.369202998 -0300
> >> @@ -10,8 +10,8 @@
> >>  #else
> >>  #include <string.h>
> >>  #endif
> >> -#include "inat.h"
> >> -#include "insn.h"
> >> +#include <asm/inat.h>
> >> +#include <asm/insn.h>
> >>
> >>  /* Verify next sizeof(t) bytes can be on the same instruction */
> >>  #define validate_next(t, insn, n)	\
> >> [acme@quaco perf]$
> >>
> >>
> >> - Arnaldo
> >>  
> >>> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> >>> ---
> >>>  tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/perf/util/intel-pt-decoder/insn.c b/tools/perf/util/intel-pt-decoder/insn.c
> >>> index ca983e2bea8b..de1944c60aa9 100644
> >>> --- a/tools/perf/util/intel-pt-decoder/insn.c
> >>> +++ b/tools/perf/util/intel-pt-decoder/insn.c
> >>> @@ -31,7 +31,8 @@
> >>>  	((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
> >>>  
> >>>  #define __get_next(t, insn)	\
> >>> -	({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> >>> +	({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
> >>> +		insn->next_byte += sizeof(t); r; })
> >>>  
> >>>  #define __peek_nbyte_next(t, insn, n)	\
> >>>  	({ t r = *(t*)((insn)->next_byte + n); r; })
> >>> -- 
> >>> 2.22.0.657.g960e92d24f-goog
> >>
> >> -- 
> >>
> >> - Arnaldo
> > 
> > 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/3] Fix insn.c misaligned address error

[Adrian Hunter]

On 29/07/19 10:32 PM, Ian Rogers wrote:
> On Mon, Jul 29, 2019 at 1:24 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 27/07/19 12:46 PM, Masami Hiramatsu wrote:
>>> On Fri, 26 Jul 2019 16:38:06 -0300
>>> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>>>
>>>> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
>>>>> The ubsan (undefined behavior sanitizer) version of perf throws an
>>>>> error on the 'x86 instruction decoder - new instructions' function
>>>>> of perf test.
>>>>>
>>>>> To reproduce this run:
>>>>> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
>>>>>
>>>>> then run: tools/perf/perf test 62 -v
>>>>>
>>>>> The error occurs in the __get_next macro (line 34) where an int is
>>>>> read from a potentially unaligned address. Using memcpy instead of
>>>>> assignment from an unaligned pointer.
>>>>
>>>> Since this came from the kernel, don't we have to fix it there as well?
>>>> Masami, Adrian?
>>>
>>> I guess we don't need it, since x86 can access "unaligned address" and
>>> x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
>>> that part. Maybe if we run it on other arch as cross-arch application,
>>> it may cause unaligned pointer issue.
> 
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
> "A pointer to an object or incomplete type may be converted to a
> pointer to a different object or incomplete type. If the resulting
> pointer is not correctly aligned for the pointed-to type, the behavior
> is undefined."
> I agree the code will generally run on x86.
> 
>> Yes, theoretically Intel PT decoding can be done on any arch.
>>
>> But the memcpy is probably sub-optimal for x86, so the patch as it stands
>> does not seem suitable.  I notice the kernel has get_unaligned() and
>> put_unaligned().
> 
> Why is a fixed sized memcpy suboptimal? The compiler can should turn
> into a load.

True, I didn't click it was fixed size.

Re: [PATCH 3/3] Fix insn.c misaligned address error

[Adrian Hunter]

On 30/07/19 3:47 AM, Masami Hiramatsu wrote:
> On Mon, 29 Jul 2019 11:22:34 +0300
> Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
>> On 27/07/19 12:46 PM, Masami Hiramatsu wrote:
>>> On Fri, 26 Jul 2019 16:38:06 -0300
>>> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>>>
>>>> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
>>>>> The ubsan (undefined behavior sanitizer) version of perf throws an
>>>>> error on the 'x86 instruction decoder - new instructions' function
>>>>> of perf test.
>>>>>
>>>>> To reproduce this run:
>>>>> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
>>>>>
>>>>> then run: tools/perf/perf test 62 -v
>>>>>
>>>>> The error occurs in the __get_next macro (line 34) where an int is
>>>>> read from a potentially unaligned address. Using memcpy instead of
>>>>> assignment from an unaligned pointer.
>>>>
>>>> Since this came from the kernel, don't we have to fix it there as well?
>>>> Masami, Adrian?
>>>
>>> I guess we don't need it, since x86 can access "unaligned address" and
>>> x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
>>> that part. Maybe if we run it on other arch as cross-arch application,
>>> it may cause unaligned pointer issue.
>>
>> Yes, theoretically Intel PT decoding can be done on any arch.
>>
>> But the memcpy is probably sub-optimal for x86, so the patch as it stands
>> does not seem suitable.  I notice the kernel has get_unaligned() and
>> put_unaligned().
>>
>> Obviously it would be better for a patch to be accepted to
>> arch/x86/lib/insn.c also.
> 
> Hmm, then I rather like memcpy() for arch/x86/lib/insn.c, because it runs only
> on x86.

Yes, I was wrong about memcpy, and it is simpler for perf tools than
dragging out get_unaligned().

RE: [PATCH 3/3] Fix insn.c misaligned address error

[David Laight]

From: Adrian Hunter
> Sent: 30 July 2019 08:53
> On 30/07/19 3:47 AM, Masami Hiramatsu wrote:
> > On Mon, 29 Jul 2019 11:22:34 +0300
> > Adrian Hunter <adrian.hunter@intel.com> wrote:
> >
> >> On 27/07/19 12:46 PM, Masami Hiramatsu wrote:
> >>> On Fri, 26 Jul 2019 16:38:06 -0300
> >>> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> >>>
> >>>> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> >>>>> The ubsan (undefined behavior sanitizer) version of perf throws an
> >>>>> error on the 'x86 instruction decoder - new instructions' function
> >>>>> of perf test.
> >>>>>
> >>>>> To reproduce this run:
> >>>>> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> >>>>>
> >>>>> then run: tools/perf/perf test 62 -v
> >>>>>
> >>>>> The error occurs in the __get_next macro (line 34) where an int is
> >>>>> read from a potentially unaligned address. Using memcpy instead of
> >>>>> assignment from an unaligned pointer.
> >>>>
> >>>> Since this came from the kernel, don't we have to fix it there as well?
> >>>> Masami, Adrian?
> >>>
> >>> I guess we don't need it, since x86 can access "unaligned address" and
> >>> x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
> >>> that part. Maybe if we run it on other arch as cross-arch application,
> >>> it may cause unaligned pointer issue.
> >>
> >> Yes, theoretically Intel PT decoding can be done on any arch.
> >>
> >> But the memcpy is probably sub-optimal for x86, so the patch as it stands
> >> does not seem suitable.  I notice the kernel has get_unaligned() and
> >> put_unaligned().
> >>
> >> Obviously it would be better for a patch to be accepted to
> >> arch/x86/lib/insn.c also.
> >
> > Hmm, then I rather like memcpy() for arch/x86/lib/insn.c, because it runs only
> > on x86.
> 
> Yes, I was wrong about memcpy, and it is simpler for perf tools than
> dragging out get_unaligned().

It may well make the generated code worse because some optimisations
won't happen because they would need to be done before memcpy()
gets inlined.
I've certainly seen cases where a #define generates significantly
better code than an inline function.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2] Fix annotate.c use of uninitialized value error

[Jiri Olsa]

On Mon, Jul 29, 2019 at 01:57:50PM -0700, Numfor Mbiziwo-Tiapo wrote:
> Our local MSAN (Memory Sanitizer) build of perf throws a warning
> that comes from the "dso__disassemble_filename" function in
> "tools/perf/util/annotate.c" when running perf record.
> 
> The warning stems from the call to readlink, in which "build_id_path"
> was being read into "linkname". Since readlink does not null terminate,
> an uninitialized memory access would later occur when "linkname" is
> passed into the strstr function. This is simply fixed by null-terminating
> "linkname" after the call to readlink.
> 
> To reproduce this warning, build perf by running:
> make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\
>  -fsanitize-memory-track-origins"
> 
> (Additionally, llvm might have to be installed and clang might have to
> be specified as the compiler - export CC=/usr/bin/clang)
> 
> then running:
> tools/perf/perf record -o - ls / | tools/perf/perf --no-pager annotate\
>  -i - --stdio
> 
> Please see the cover letter for why false positive warnings may be
> generated.
> 
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/util/annotate.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 70de8f6b3aee..e1b075b52dce 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1627,6 +1627,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
>  	char *build_id_filename;
>  	char *build_id_path = NULL;
>  	char *pos;
> +	int len;
>  
>  	if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS &&
>  	    !dso__is_kcore(dso))
> @@ -1655,10 +1656,16 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
>  	if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
>  		dirname(build_id_path);
>  
> -	if (dso__is_kcore(dso) ||
> -	    readlink(build_id_path, linkname, sizeof(linkname)) < 0 ||
> -	    strstr(linkname, DSO__NAME_KALLSYMS) ||
> -	    access(filename, R_OK)) {
> +	if (dso__is_kcore(dso))
> +		goto fallback;
> +
> +	len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
> +	if (len < 0)
> +		goto fallback;
> +
> +	linkname[len] = '\0';
> +	if (strstr(linkname, DSO__NAME_KALLSYMS) ||
> +		access(filename, R_OK)) {
>  fallback:
>  		/*
>  		 * If we don't have build-ids or the build-id file isn't in the
> -- 
> 2.22.0.709.g102302147b-goog
> 

Re: [PATCH v2] Fix annotate.c use of uninitialized value error

[Ian Rogers]

It looks like this wasn't merged to tip. Does anything need addressing
to get it merged?

Thanks,
Ian

On Wed, Aug 7, 2019 at 4:32 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Jul 29, 2019 at 01:57:50PM -0700, Numfor Mbiziwo-Tiapo wrote:
> > Our local MSAN (Memory Sanitizer) build of perf throws a warning
> > that comes from the "dso__disassemble_filename" function in
> > "tools/perf/util/annotate.c" when running perf record.
> >
> > The warning stems from the call to readlink, in which "build_id_path"
> > was being read into "linkname". Since readlink does not null terminate,
> > an uninitialized memory access would later occur when "linkname" is
> > passed into the strstr function. This is simply fixed by null-terminating
> > "linkname" after the call to readlink.
> >
> > To reproduce this warning, build perf by running:
> > make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\
> >  -fsanitize-memory-track-origins"
> >
> > (Additionally, llvm might have to be installed and clang might have to
> > be specified as the compiler - export CC=/usr/bin/clang)
> >
> > then running:
> > tools/perf/perf record -o - ls / | tools/perf/perf --no-pager annotate\
> >  -i - --stdio
> >
> > Please see the cover letter for why false positive warnings may be
> > generated.
> >
> > Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
>
> thanks,
> jirka
>
> > ---
> >  tools/perf/util/annotate.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 70de8f6b3aee..e1b075b52dce 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -1627,6 +1627,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
> >       char *build_id_filename;
> >       char *build_id_path = NULL;
> >       char *pos;
> > +     int len;
> >
> >       if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS &&
> >           !dso__is_kcore(dso))
> > @@ -1655,10 +1656,16 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
> >       if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
> >               dirname(build_id_path);
> >
> > -     if (dso__is_kcore(dso) ||
> > -         readlink(build_id_path, linkname, sizeof(linkname)) < 0 ||
> > -         strstr(linkname, DSO__NAME_KALLSYMS) ||
> > -         access(filename, R_OK)) {
> > +     if (dso__is_kcore(dso))
> > +             goto fallback;
> > +
> > +     len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
> > +     if (len < 0)
> > +             goto fallback;
> > +
> > +     linkname[len] = '\0';
> > +     if (strstr(linkname, DSO__NAME_KALLSYMS) ||
> > +             access(filename, R_OK)) {
> >  fallback:
> >               /*
> >                * If we don't have build-ids or the build-id file isn't in the
> > --
> > 2.22.0.709.g102302147b-goog
> >

Re: [PATCH v2] Fix annotate.c use of uninitialized value error

[Ian Rogers]

On Fri, Oct 25, 2019 at 3:11 PM Ian Rogers <irogers@google.com> wrote:
>
> It looks like this wasn't merged to tip. Does anything need addressing
> to get it merged?
>
> Thanks,
> Ian
>
> On Wed, Aug 7, 2019 at 4:32 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Mon, Jul 29, 2019 at 01:57:50PM -0700, Numfor Mbiziwo-Tiapo wrote:
> > > Our local MSAN (Memory Sanitizer) build of perf throws a warning
> > > that comes from the "dso__disassemble_filename" function in
> > > "tools/perf/util/annotate.c" when running perf record.
> > >
> > > The warning stems from the call to readlink, in which "build_id_path"
> > > was being read into "linkname". Since readlink does not null terminate,
> > > an uninitialized memory access would later occur when "linkname" is
> > > passed into the strstr function. This is simply fixed by null-terminating
> > > "linkname" after the call to readlink.
> > >
> > > To reproduce this warning, build perf by running:
> > > make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\
> > >  -fsanitize-memory-track-origins"
> > >
> > > (Additionally, llvm might have to be installed and clang might have to
> > > be specified as the compiler - export CC=/usr/bin/clang)
> > >
> > > then running:
> > > tools/perf/perf record -o - ls / | tools/perf/perf --no-pager annotate\
> > >  -i - --stdio
> > >
> > > Please see the cover letter for why false positive warnings may be
> > > generated.
> > >
> > > Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> >
> > Acked-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Arnaldo, I think this got overlooked. Thanks,

Ian

> > thanks,
> > jirka
> >
> > > ---
> > >  tools/perf/util/annotate.c | 15 +++++++++++----
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > > index 70de8f6b3aee..e1b075b52dce 100644
> > > --- a/tools/perf/util/annotate.c
> > > +++ b/tools/perf/util/annotate.c
> > > @@ -1627,6 +1627,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
> > >       char *build_id_filename;
> > >       char *build_id_path = NULL;
> > >       char *pos;
> > > +     int len;
> > >
> > >       if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS &&
> > >           !dso__is_kcore(dso))
> > > @@ -1655,10 +1656,16 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
> > >       if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
> > >               dirname(build_id_path);
> > >
> > > -     if (dso__is_kcore(dso) ||
> > > -         readlink(build_id_path, linkname, sizeof(linkname)) < 0 ||
> > > -         strstr(linkname, DSO__NAME_KALLSYMS) ||
> > > -         access(filename, R_OK)) {
> > > +     if (dso__is_kcore(dso))
> > > +             goto fallback;
> > > +
> > > +     len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
> > > +     if (len < 0)
> > > +             goto fallback;
> > > +
> > > +     linkname[len] = '\0';
> > > +     if (strstr(linkname, DSO__NAME_KALLSYMS) ||
> > > +             access(filename, R_OK)) {
> > >  fallback:
> > >               /*
> > >                * If we don't have build-ids or the build-id file isn't in the
> > > --
> > > 2.22.0.709.g102302147b-goog
> > >

Re: [PATCH v2] Fix annotate.c use of uninitialized value error

[Arnaldo Carvalho de Melo]

Em Wed, Jul 08, 2020 at 05:54:47PM -0700, Ian Rogers escreveu:
> On Fri, Oct 25, 2019 at 3:11 PM Ian Rogers <irogers@google.com> wrote:
> >
> > It looks like this wasn't merged to tip. Does anything need addressing
> > to get it merged?

Finally processed, thanks for the multiple reminders,

- Arnaldo

> > Thanks,
> > Ian
> >
> > On Wed, Aug 7, 2019 at 4:32 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Mon, Jul 29, 2019 at 01:57:50PM -0700, Numfor Mbiziwo-Tiapo wrote:
> > > > Our local MSAN (Memory Sanitizer) build of perf throws a warning
> > > > that comes from the "dso__disassemble_filename" function in
> > > > "tools/perf/util/annotate.c" when running perf record.
> > > >
> > > > The warning stems from the call to readlink, in which "build_id_path"
> > > > was being read into "linkname". Since readlink does not null terminate,
> > > > an uninitialized memory access would later occur when "linkname" is
> > > > passed into the strstr function. This is simply fixed by null-terminating
> > > > "linkname" after the call to readlink.
> > > >
> > > > To reproduce this warning, build perf by running:
> > > > make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\
> > > >  -fsanitize-memory-track-origins"
> > > >
> > > > (Additionally, llvm might have to be installed and clang might have to
> > > > be specified as the compiler - export CC=/usr/bin/clang)
> > > >
> > > > then running:
> > > > tools/perf/perf record -o - ls / | tools/perf/perf --no-pager annotate\
> > > >  -i - --stdio
> > > >
> > > > Please see the cover letter for why false positive warnings may be
> > > > generated.
> > > >
> > > > Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> > >
> > > Acked-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Ian Rogers <irogers@google.com>
> 
> Arnaldo, I think this got overlooked. Thanks,
> 
> Ian
> 
> > > thanks,
> > > jirka
> > >
> > > > ---
> > > >  tools/perf/util/annotate.c | 15 +++++++++++----
> > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > > > index 70de8f6b3aee..e1b075b52dce 100644
> > > > --- a/tools/perf/util/annotate.c
> > > > +++ b/tools/perf/util/annotate.c
> > > > @@ -1627,6 +1627,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
> > > >       char *build_id_filename;
> > > >       char *build_id_path = NULL;
> > > >       char *pos;
> > > > +     int len;
> > > >
> > > >       if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS &&
> > > >           !dso__is_kcore(dso))
> > > > @@ -1655,10 +1656,16 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
> > > >       if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
> > > >               dirname(build_id_path);
> > > >
> > > > -     if (dso__is_kcore(dso) ||
> > > > -         readlink(build_id_path, linkname, sizeof(linkname)) < 0 ||
> > > > -         strstr(linkname, DSO__NAME_KALLSYMS) ||
> > > > -         access(filename, R_OK)) {
> > > > +     if (dso__is_kcore(dso))
> > > > +             goto fallback;
> > > > +
> > > > +     len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
> > > > +     if (len < 0)
> > > > +             goto fallback;
> > > > +
> > > > +     linkname[len] = '\0';
> > > > +     if (strstr(linkname, DSO__NAME_KALLSYMS) ||
> > > > +             access(filename, R_OK)) {
> > > >  fallback:
> > > >               /*
> > > >                * If we don't have build-ids or the build-id file isn't in the
> > > > --
> > > > 2.22.0.709.g102302147b-goog
> > > >

-- 

- Arnaldo