Messages in this thread | | | Date | Tue, 30 Jul 2019 09:47:45 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH 3/3] Fix insn.c misaligned address error |
| |
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>
| |