arch/x86/include/asm/uaccess_64.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
This works around what seems to be an optimization bug in gcc (at least
13.3.0), where it predicts access_ok() to fail despite the hint to the
contrary.
_copy_to_user contains:
if (access_ok(to, n)) {
instrument_copy_to_user(to, from, n);
n = raw_copy_to_user(to, from, n);
}
Where access_ok is likely(__access_ok(addr, size)), yet the compiler
emits conditional jumps forward for the case where it succeeds:
<+0>: endbr64
<+4>: mov %rdx,%rcx
<+7>: mov %rdx,%rax
<+10>: xor %edx,%edx
<+12>: add %rdi,%rcx
<+15>: setb %dl
<+18>: movabs $0x123456789abcdef,%r8
<+28>: test %rdx,%rdx
<+31>: jne 0xffffffff81b3b7c6 <_copy_to_user+38>
<+33>: cmp %rcx,%r8
<+36>: jae 0xffffffff81b3b7cb <_copy_to_user+43>
<+38>: jmp 0xffffffff822673e0 <__x86_return_thunk>
<+43>: nop
<+44>: nop
<+45>: nop
<+46>: mov %rax,%rcx
<+49>: rep movsb %ds:(%rsi),%es:(%rdi)
<+51>: nop
<+52>: nop
<+53>: nop
<+54>: mov %rcx,%rax
<+57>: nop
<+58>: nop
<+59>: nop
<+60>: jmp 0xffffffff822673e0 <__x86_return_thunk>
Patching _copy_to_user() to likely() around the access_ok() use does
not change the asm.
However, spelling out the prediction *within* __access_ok() does the
trick:
<+0>: endbr64
<+4>: xor %eax,%eax
<+6>: mov %rdx,%rcx
<+9>: add %rdi,%rdx
<+12>: setb %al
<+15>: movabs $0x123456789abcdef,%r8
<+25>: test %rax,%rax
<+28>: jne 0xffffffff81b315e6 <_copy_to_user+54>
<+30>: cmp %rdx,%r8
<+33>: jb 0xffffffff81b315e6 <_copy_to_user+54>
<+35>: nop
<+36>: nop
<+37>: nop
<+38>: rep movsb %ds:(%rsi),%es:(%rdi)
<+40>: nop
<+41>: nop
<+42>: nop
<+43>: nop
<+44>: nop
<+45>: nop
<+46>: mov %rcx,%rax
<+49>: jmp 0xffffffff82255ba0 <__x86_return_thunk>
<+54>: mov %rcx,%rax
<+57>: jmp 0xffffffff82255ba0 <__x86_return_thunk>
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
I did not investigate what's going on here. It may be other spots are
also suffering.
If someone commits to figuring out what went wrong I'll be happy to drop
this patch. Otherwise this can be worked around at least for access_ok()
consumers.
arch/x86/include/asm/uaccess_64.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index c52f0133425b..30c912375260 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -98,11 +98,11 @@ static inline void __user *mask_user_address(const void __user *ptr)
static inline bool __access_ok(const void __user *ptr, unsigned long size)
{
if (__builtin_constant_p(size <= PAGE_SIZE) && size <= PAGE_SIZE) {
- return valid_user_address(ptr);
+ return likely(valid_user_address(ptr));
} else {
unsigned long sum = size + (__force unsigned long)ptr;
- return valid_user_address(sum) && sum >= (__force unsigned long)ptr;
+ return likely(valid_user_address(sum) && sum >= (__force unsigned long)ptr);
}
}
#define __access_ok __access_ok
--
2.43.0
* Mateusz Guzik <mjguzik@gmail.com> wrote:
> This works around what seems to be an optimization bug in gcc (at least
> 13.3.0), where it predicts access_ok() to fail despite the hint to the
> contrary.
>
> _copy_to_user contains:
> if (access_ok(to, n)) {
> instrument_copy_to_user(to, from, n);
> n = raw_copy_to_user(to, from, n);
> }
>
> Where access_ok is likely(__access_ok(addr, size)), yet the compiler
> emits conditional jumps forward for the case where it succeeds:
>
> <+0>: endbr64
> <+4>: mov %rdx,%rcx
> <+7>: mov %rdx,%rax
> <+10>: xor %edx,%edx
> <+12>: add %rdi,%rcx
> <+15>: setb %dl
> <+18>: movabs $0x123456789abcdef,%r8
> <+28>: test %rdx,%rdx
> <+31>: jne 0xffffffff81b3b7c6 <_copy_to_user+38>
> <+33>: cmp %rcx,%r8
> <+36>: jae 0xffffffff81b3b7cb <_copy_to_user+43>
> <+38>: jmp 0xffffffff822673e0 <__x86_return_thunk>
> <+43>: nop
> <+44>: nop
> <+45>: nop
> <+46>: mov %rax,%rcx
> <+49>: rep movsb %ds:(%rsi),%es:(%rdi)
> <+51>: nop
> <+52>: nop
> <+53>: nop
> <+54>: mov %rcx,%rax
> <+57>: nop
> <+58>: nop
> <+59>: nop
> <+60>: jmp 0xffffffff822673e0 <__x86_return_thunk>
>
> Patching _copy_to_user() to likely() around the access_ok() use does
> not change the asm.
>
> However, spelling out the prediction *within* __access_ok() does the
> trick:
> <+0>: endbr64
> <+4>: xor %eax,%eax
> <+6>: mov %rdx,%rcx
> <+9>: add %rdi,%rdx
> <+12>: setb %al
> <+15>: movabs $0x123456789abcdef,%r8
> <+25>: test %rax,%rax
> <+28>: jne 0xffffffff81b315e6 <_copy_to_user+54>
> <+30>: cmp %rdx,%r8
> <+33>: jb 0xffffffff81b315e6 <_copy_to_user+54>
> <+35>: nop
> <+36>: nop
> <+37>: nop
> <+38>: rep movsb %ds:(%rsi),%es:(%rdi)
> <+40>: nop
> <+41>: nop
> <+42>: nop
> <+43>: nop
> <+44>: nop
> <+45>: nop
> <+46>: mov %rcx,%rax
> <+49>: jmp 0xffffffff82255ba0 <__x86_return_thunk>
> <+54>: mov %rcx,%rax
> <+57>: jmp 0xffffffff82255ba0 <__x86_return_thunk>
>
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---
>
> I did not investigate what's going on here. It may be other spots are
> also suffering.
>
> If someone commits to figuring out what went wrong I'll be happy to drop
> this patch. Otherwise this can be worked around at least for access_ok()
> consumers.
>
> arch/x86/include/asm/uaccess_64.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
> index c52f0133425b..30c912375260 100644
> --- a/arch/x86/include/asm/uaccess_64.h
> +++ b/arch/x86/include/asm/uaccess_64.h
> @@ -98,11 +98,11 @@ static inline void __user *mask_user_address(const void __user *ptr)
> static inline bool __access_ok(const void __user *ptr, unsigned long size)
> {
> if (__builtin_constant_p(size <= PAGE_SIZE) && size <= PAGE_SIZE) {
> - return valid_user_address(ptr);
> + return likely(valid_user_address(ptr));
> } else {
> unsigned long sum = size + (__force unsigned long)ptr;
>
> - return valid_user_address(sum) && sum >= (__force unsigned long)ptr;
> + return likely(valid_user_address(sum) && sum >= (__force unsigned long)ptr);
Cannot we put this into valid_user_address() definition, via something
like the below patch?
It's also the right place to have the hint: that user addresses are
valid is the common case we optimize for.
Thanks,
Ingo
arch/x86/include/asm/uaccess_64.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index c52f0133425b..4c13883371aa 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -54,7 +54,7 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm,
#endif
#define valid_user_address(x) \
- ((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX))
+ likely((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX))
/*
* Masking the user address is an alternative to a conditional
On Tue, Apr 1, 2025 at 10:35 PM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> > This works around what seems to be an optimization bug in gcc (at least
> > 13.3.0), where it predicts access_ok() to fail despite the hint to the
> > contrary.
> >
> > _copy_to_user contains:
> > if (access_ok(to, n)) {
> > instrument_copy_to_user(to, from, n);
> > n = raw_copy_to_user(to, from, n);
> > }
> >
> > Where access_ok is likely(__access_ok(addr, size)), yet the compiler
> > emits conditional jumps forward for the case where it succeeds:
> >
> > <+0>: endbr64
> > <+4>: mov %rdx,%rcx
> > <+7>: mov %rdx,%rax
> > <+10>: xor %edx,%edx
> > <+12>: add %rdi,%rcx
> > <+15>: setb %dl
> > <+18>: movabs $0x123456789abcdef,%r8
> > <+28>: test %rdx,%rdx
> > <+31>: jne 0xffffffff81b3b7c6 <_copy_to_user+38>
> > <+33>: cmp %rcx,%r8
> > <+36>: jae 0xffffffff81b3b7cb <_copy_to_user+43>
> > <+38>: jmp 0xffffffff822673e0 <__x86_return_thunk>
> > <+43>: nop
> > <+44>: nop
> > <+45>: nop
> > <+46>: mov %rax,%rcx
> > <+49>: rep movsb %ds:(%rsi),%es:(%rdi)
> > <+51>: nop
> > <+52>: nop
> > <+53>: nop
> > <+54>: mov %rcx,%rax
> > <+57>: nop
> > <+58>: nop
> > <+59>: nop
> > <+60>: jmp 0xffffffff822673e0 <__x86_return_thunk>
> >
> > Patching _copy_to_user() to likely() around the access_ok() use does
> > not change the asm.
> >
> > However, spelling out the prediction *within* __access_ok() does the
> > trick:
> > <+0>: endbr64
> > <+4>: xor %eax,%eax
> > <+6>: mov %rdx,%rcx
> > <+9>: add %rdi,%rdx
> > <+12>: setb %al
> > <+15>: movabs $0x123456789abcdef,%r8
> > <+25>: test %rax,%rax
> > <+28>: jne 0xffffffff81b315e6 <_copy_to_user+54>
> > <+30>: cmp %rdx,%r8
> > <+33>: jb 0xffffffff81b315e6 <_copy_to_user+54>
> > <+35>: nop
> > <+36>: nop
> > <+37>: nop
> > <+38>: rep movsb %ds:(%rsi),%es:(%rdi)
> > <+40>: nop
> > <+41>: nop
> > <+42>: nop
> > <+43>: nop
> > <+44>: nop
> > <+45>: nop
> > <+46>: mov %rcx,%rax
> > <+49>: jmp 0xffffffff82255ba0 <__x86_return_thunk>
> > <+54>: mov %rcx,%rax
> > <+57>: jmp 0xffffffff82255ba0 <__x86_return_thunk>
Mateusz, can you please file a bug in GCC bug tracker [1], following
the instructions in [2]?
[1] https://gcc.gnu.org/bugzilla/
[2] https://gcc.gnu.org/bugs/
Thanks,
Uros.
* Ingo Molnar <mingo@kernel.org> wrote: > It's also the right place to have the hint: that user addresses are > valid is the common case we optimize for. > > Thanks, > > Ingo > > arch/x86/include/asm/uaccess_64.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h > index c52f0133425b..4c13883371aa 100644 > --- a/arch/x86/include/asm/uaccess_64.h > +++ b/arch/x86/include/asm/uaccess_64.h > @@ -54,7 +54,7 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm, > #endif > > #define valid_user_address(x) \ > - ((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX)) > + likely((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX)) Should we go this way, this is the safe macro variant: #define valid_user_address(x) \ (likely((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX))) But this compiler bug sounds weird ... Thanks, Ingo
On Tue, Apr 1, 2025 at 10:43 PM Ingo Molnar <mingo@kernel.org> wrote: > > > * Ingo Molnar <mingo@kernel.org> wrote: > > > It's also the right place to have the hint: that user addresses are > > valid is the common case we optimize for. > > > > Thanks, > > > > Ingo > > > > arch/x86/include/asm/uaccess_64.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h > > index c52f0133425b..4c13883371aa 100644 > > --- a/arch/x86/include/asm/uaccess_64.h > > +++ b/arch/x86/include/asm/uaccess_64.h > > @@ -54,7 +54,7 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm, > > #endif > > > > #define valid_user_address(x) \ > > - ((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX)) > > + likely((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX)) > > Should we go this way, this is the safe macro variant: > > #define valid_user_address(x) \ > (likely((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX))) > Note the are 2 tests and the other one does not get covered by *this* likely: valid_user_address(sum) && sum >= (__force unsigned long)ptr; as in sum >= ptr is left be. However, I confirmed that with your patch the issue also goes away so I guess it is fine. I think it would be the safest to likely within valid_user_address() like in your patch, and likely on the entire expression like in mine. That said, there will be no hard feelz if you just commit your patch and drop mine. > But this compiler bug sounds weird ... > It is very weird. A person running a different distro with a 6.13 kernel and new gcc confirmed they have the same bad asm, so this is not just something screwy in my setup. -- Mateusz Guzik <mjguzik gmail.com>
* Mateusz Guzik <mjguzik@gmail.com> wrote: > On Tue, Apr 1, 2025 at 10:43 PM Ingo Molnar <mingo@kernel.org> wrote: > > > > > > * Ingo Molnar <mingo@kernel.org> wrote: > > > > > It's also the right place to have the hint: that user addresses are > > > valid is the common case we optimize for. > > > > > > Thanks, > > > > > > Ingo > > > > > > arch/x86/include/asm/uaccess_64.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h > > > index c52f0133425b..4c13883371aa 100644 > > > --- a/arch/x86/include/asm/uaccess_64.h > > > +++ b/arch/x86/include/asm/uaccess_64.h > > > @@ -54,7 +54,7 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm, > > > #endif > > > > > > #define valid_user_address(x) \ > > > - ((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX)) > > > + likely((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX)) > > > > Should we go this way, this is the safe macro variant: > > > > #define valid_user_address(x) \ > > (likely((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX))) > > > > Note the are 2 tests and the other one does not get covered by *this* likely: > valid_user_address(sum) && sum >= (__force unsigned long)ptr; > > as in sum >= ptr is left be. > > However, I confirmed that with your patch the issue also goes away so > I guess it is fine. > > I think it would be the safest to likely within valid_user_address() > like in your patch, and likely on the entire expression like in mine. > > That said, there will be no hard feelz if you just commit your patch > and drop mine. Feel free to turn it into a Co-developed-by patch: From: Mateusz Guzik <mjguzik@gmail.com> ... Co-developed-by: Ingo Molnar <mingo@kernel.org> Signed-off-by: Ingo Molnar <mingo@kernel.org> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> Because all I did was to transform your fix into something a bit more maintainable. I didn't even test it. Thanks, Ingo
On Tue, Apr 1, 2025 at 11:00 PM Ingo Molnar <mingo@kernel.org> wrote: > > > * Mateusz Guzik <mjguzik@gmail.com> wrote: > > > On Tue, Apr 1, 2025 at 10:43 PM Ingo Molnar <mingo@kernel.org> wrote: > > > > > > > > > * Ingo Molnar <mingo@kernel.org> wrote: > > > > > > > It's also the right place to have the hint: that user addresses are > > > > valid is the common case we optimize for. > > > > > > > > Thanks, > > > > > > > > Ingo > > > > > > > > arch/x86/include/asm/uaccess_64.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h > > > > index c52f0133425b..4c13883371aa 100644 > > > > --- a/arch/x86/include/asm/uaccess_64.h > > > > +++ b/arch/x86/include/asm/uaccess_64.h > > > > @@ -54,7 +54,7 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm, > > > > #endif > > > > > > > > #define valid_user_address(x) \ > > > > - ((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX)) > > > > + likely((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX)) > > > > > > Should we go this way, this is the safe macro variant: > > > > > > #define valid_user_address(x) \ > > > (likely((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX))) > > > > > > > Note the are 2 tests and the other one does not get covered by *this* likely: > > valid_user_address(sum) && sum >= (__force unsigned long)ptr; > > > > as in sum >= ptr is left be. > > > > However, I confirmed that with your patch the issue also goes away so > > I guess it is fine. > > > > I think it would be the safest to likely within valid_user_address() > > like in your patch, and likely on the entire expression like in mine. > > > > That said, there will be no hard feelz if you just commit your patch > > and drop mine. > > Feel free to turn it into a Co-developed-by patch: > > From: Mateusz Guzik <mjguzik@gmail.com> > ... > > Co-developed-by: Ingo Molnar <mingo@kernel.org> > Signed-off-by: Ingo Molnar <mingo@kernel.org> > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > > Because all I did was to transform your fix into something a bit more > maintainable. I didn't even test it. > This is 2 lines changed in total, both trivially. I think it is going to look rather silly to have this "co-developed", but I can do it if you insist. My suggestion would be that you commit your thing and add: Reported-and-tested-by: Mateusz Guzik <mjguzik@gmail.com> feel free to steal the disasm before/after -- Mateusz Guzik <mjguzik gmail.com>
The following commit has been merged into the x86/asm branch of tip:
Commit-ID: 6f9bd8ae0340326a7c711bc681321bf9a4e15fb2
Gitweb: https://git.kernel.org/tip/6f9bd8ae0340326a7c711bc681321bf9a4e15fb2
Author: Mateusz Guzik <mjguzik@gmail.com>
AuthorDate: Tue, 01 Apr 2025 22:30:29 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 09 Apr 2025 21:40:17 +02:00
x86/uaccess: Predict valid_user_address() returning true
This works around what seems to be an optimization bug in GCC (at least
13.3.0), where it predicts access_ok() to fail despite the hint to the
contrary.
_copy_to_user() contains:
if (access_ok(to, n)) {
instrument_copy_to_user(to, from, n);
n = raw_copy_to_user(to, from, n);
}
Where access_ok() is likely(__access_ok(addr, size)), yet the compiler
emits conditional jumps forward for the case where it succeeds:
<+0>: endbr64
<+4>: mov %rdx,%rcx
<+7>: mov %rdx,%rax
<+10>: xor %edx,%edx
<+12>: add %rdi,%rcx
<+15>: setb %dl
<+18>: movabs $0x123456789abcdef,%r8
<+28>: test %rdx,%rdx
<+31>: jne 0xffffffff81b3b7c6 <_copy_to_user+38>
<+33>: cmp %rcx,%r8
<+36>: jae 0xffffffff81b3b7cb <_copy_to_user+43>
<+38>: jmp 0xffffffff822673e0 <__x86_return_thunk>
<+43>: nop
<+44>: nop
<+45>: nop
<+46>: mov %rax,%rcx
<+49>: rep movsb %ds:(%rsi),%es:(%rdi)
<+51>: nop
<+52>: nop
<+53>: nop
<+54>: mov %rcx,%rax
<+57>: nop
<+58>: nop
<+59>: nop
<+60>: jmp 0xffffffff822673e0 <__x86_return_thunk>
Patching _copy_to_user() to likely() around the access_ok() use does
not change the asm.
However, spelling out the prediction *within* valid_user_address() does the
trick:
<+0>: endbr64
<+4>: xor %eax,%eax
<+6>: mov %rdx,%rcx
<+9>: add %rdi,%rdx
<+12>: setb %al
<+15>: movabs $0x123456789abcdef,%r8
<+25>: test %rax,%rax
<+28>: jne 0xffffffff81b315e6 <_copy_to_user+54>
<+30>: cmp %rdx,%r8
<+33>: jb 0xffffffff81b315e6 <_copy_to_user+54>
<+35>: nop
<+36>: nop
<+37>: nop
<+38>: rep movsb %ds:(%rsi),%es:(%rdi)
<+40>: nop
<+41>: nop
<+42>: nop
<+43>: nop
<+44>: nop
<+45>: nop
<+46>: mov %rcx,%rax
<+49>: jmp 0xffffffff82255ba0 <__x86_return_thunk>
<+54>: mov %rcx,%rax
<+57>: jmp 0xffffffff82255ba0 <__x86_return_thunk>
Since we kinda expect valid_user_address() to be likely anyway,
add the likely() annotation that also happens to work around
this compiler bug.
[ mingo: Moved the unlikely() branch into valid_user_address() & updated the changelog ]
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/r/20250401203029.1132135-1-mjguzik@gmail.com
---
arch/x86/include/asm/uaccess_64.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index c52f013..4c13883 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -54,7 +54,7 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm,
#endif
#define valid_user_address(x) \
- ((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX))
+ likely((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX))
/*
* Masking the user address is an alternative to a conditional© 2016 - 2025 Red Hat, Inc.