On Thu, Oct 15, 2020 at 2:35 PM <h...@zytor.com> wrote: > > On October 15, 2020 9:12:16 AM PDT, Ian Rogers <irog...@google.com> wrote: > >From: Numfor Mbiziwo-Tiapo <n...@google.com> > > > >Don't perform unaligned loads in __get_next and __peek_nbyte_next as > >these are forms of undefined behavior. > > > >These problems were identified using the undefined behavior sanitizer > >(ubsan) with the tools version of the code and perf test. Part of this > >patch was previously posted here: > >https://lore.kernel.org/lkml/20190724184512.162887-4-n...@google.com/ > > > >v2. removes the validate_next check and merges the 2 changes into one > >as > >requested by Masami Hiramatsu <mhira...@kernel.org> > > > >Signed-off-by: Ian Rogers <irog...@google.com> > >Signed-off-by: Numfor Mbiziwo-Tiapo <n...@google.com> > >--- > > arch/x86/lib/insn.c | 4 ++-- > > tools/arch/x86/lib/insn.c | 4 ++-- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > >diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c > >index 404279563891..be88ab250146 100644 > >--- a/arch/x86/lib/insn.c > >+++ b/arch/x86/lib/insn.c > >@@ -20,10 +20,10 @@ > > ((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; }) > >+ ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); r; }) > > > > #define get_next(t, insn) \ > > ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; > >__get_next(t, insn); }) > >diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c > >index 0151dfc6da61..92358c71a59e 100644 > >--- a/tools/arch/x86/lib/insn.c > >+++ b/tools/arch/x86/lib/insn.c > >@@ -20,10 +20,10 @@ > > ((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; }) > >+ ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); r; }) > > > > #define get_next(t, insn) \ > > ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; > >__get_next(t, insn); }) > > Wait, what? > > You are taking about x86-specific code, and on x86 unaligned memory accesses > are supported, well-defined, and ubiquitous.
On why this is undefined behavior: https://lore.kernel.org/lkml/CAP-5=fU2XBoOa2=00vcuwyqsluzmsmzuxy63zjt9rz-nj+v...@mail.gmail.com/ > This is B.S. at best, and unless the compiler turns the memcpy() right back > into what you started with, deleterious for performance. On performance, the memcpys are fixed size and so lowered to loads on x86 by any reasonable compiler. See the thread above. > If you have a *very* good reason for this kind of churn, wrap it in the > unaligned access macros, but using memcpy() is insane. All you are doing is > making the code worse. The decoder is a shared code and using unaligned macros makes life hard for the other users of the code. Memcpy is the "standard" workaround for this kind of undefined behavior. https://lore.kernel.org/lkml/e4269cb2-d8e6-da26-6afd-a9df72d4b...@intel.com/ For motivation, beyond just having perf be sanitizer clean, see discussion here: https://lore.kernel.org/lkml/CAP-5=fuosgy3naztsbf3ylepabss7opsxlkcx36xkezzm34...@mail.gmail.com/ https://lore.kernel.org/lkml/160208761921.7002.1321765913567405137.tip-bot2@tip-bot2/ Thanks, Ian > -- > Sent from my Android device with K-9 Mail. Please excuse my brevity.
