From: Numfor Mbiziwo-Tiapo <[email protected]>
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/[email protected]/
v2. removes the validate_next check and merges the 2 changes into one as
requested by Masami Hiramatsu <[email protected]>
Signed-off-by: Ian Rogers <[email protected]>
Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
---
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); })
--
2.28.0.1011.ga647a8990f-goog
On October 15, 2020 9:12:16 AM PDT, Ian Rogers <[email protected]> wrote:
>From: Numfor Mbiziwo-Tiapo <[email protected]>
>
>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/[email protected]/
>
>v2. removes the validate_next check and merges the 2 changes into one
>as
>requested by Masami Hiramatsu <[email protected]>
>
>Signed-off-by: Ian Rogers <[email protected]>
>Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
>---
> 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.
This is B.S. at best, and unless the compiler turns the memcpy() right back into what you started with, deleterious for performance.
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.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Thu, Oct 15, 2020 at 2:35 PM <[email protected]> wrote:
>
> On October 15, 2020 9:12:16 AM PDT, Ian Rogers <[email protected]> wrote:
> >From: Numfor Mbiziwo-Tiapo <[email protected]>
> >
> >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/[email protected]/
> >
> >v2. removes the validate_next check and merges the 2 changes into one
> >as
> >requested by Masami Hiramatsu <[email protected]>
> >
> >Signed-off-by: Ian Rogers <[email protected]>
> >Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
> >---
> > 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+vYwA@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/[email protected]/
For motivation, beyond just having perf be sanitizer clean, see discussion here:
https://lore.kernel.org/lkml/CAP-5=fUoSGy3NAzTSbF3YLEPABSs7oPsxLkCx36XkEzzm341yw@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.
On Thu, 15 Oct 2020 09:12:16 -0700
Ian Rogers <[email protected]> wrote:
> From: Numfor Mbiziwo-Tiapo <[email protected]>
>
> 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/[email protected]/
>
> v2. removes the validate_next check and merges the 2 changes into one as
> requested by Masami Hiramatsu <[email protected]>
Looks good to me. This may be OK on x86, but now this code will be run
on other arches for cross compilation.
Acked-by: Masami Hiramatsu <[email protected]>
Thank you,
>
> Signed-off-by: Ian Rogers <[email protected]>
> Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
> ---
> 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); })
> --
> 2.28.0.1011.ga647a8990f-goog
>
--
Masami Hiramatsu <[email protected]>
On Thu, Oct 15, 2020 at 02:34:57PM -0700, [email protected] wrote:
> Wait, what?
>
> You are taking about x86-specific code, and on x86 unaligned memory
> accesses are supported, well-defined, and ubiquitous.
Objtool uses this x86 instruction decoder, people are cross-buildling
objtool to cross-build x86 kernels.
So it's entirely possible to run the x86 insn decode on BE RISC, which
very much doesn't like unaligned loads.
From: Ian Rogers
> Sent: 15 October 2020 22:47
...
> 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/[email protected]/
You can't always use memcpy() to copy unaligned data.
If the compiler can find any reason why the misaligned address
should be aligned (eg because it's type prior to some casts
requires alignment) it will use word-sized accesses that will fault.
Now in this specific code the pointers are probably all 'char *'
so have alignment 1 - so memcpy() will DTRT.
But it may generate an expensive function call.
There are 'read unaligned 32bit' (etc) macros out there.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: [email protected]
> Sent: 15 October 2020 22:35
> >Don't perform unaligned loads in __get_next and __peek_nbyte_next as
> >these are forms of undefined behavior.
...
> > #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; })
Interesting other idea.
Can you add an 'aligned(1)' into the '*(t *)' cast?
I think '*(t aligned(1) *)' is the right cast.
(gcc seems to accept it either size of the type.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)