* [Bug report] __arch_hweight32/64 x86
@ 2023-06-26 20:49 Sebastian Sumpf
2023-06-26 21:06 ` Borislav Petkov
0 siblings, 1 reply; 7+ messages in thread
From: Sebastian Sumpf @ 2023-06-26 20:49 UTC (permalink / raw
To: tglx, mingo, bp, dave.hansen, x86; +Cc: linux-kernel
Hello kernel developers,
I recently stumbled over a bug in the inline assembly of '__arch_hweight32' and
'__arch_hweight64' in 'arch/x86/include/asm/arch_hweight.h' (present in 6.4 as
well):
! asm (ALTERNATIVE("call __sw_hweight64", "popcntq %1, %0",
! X86_FEATURE_POPCNT)
! : "="REG_OUT (res)
! : REG_IN (w));
The 'ALTERNATIVE' macro checks for the popcnt feature. In case this fails the
'__sw_hweight' C-function is called from inline assembly with rax and rdi as
intput/output operands. However, the code does not contain a clobber list of any
callee safe registers that might be touched by the '__sw_height64' C-function.
Therefore, these registers will not be restored upon function return by the
compiler. This leads to varying exceptions/bad behavior caused by the thus
corrupted registers later on.
Kind regards,
Sebastian
--
Sebastian Sumpf
Genode Labs
http://www.genode-labs.com · http://genode.org
Genode Labs GmbH · Amtsgericht Dresden · HRB 28424 · Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug report] __arch_hweight32/64 x86
2023-06-26 20:49 [Bug report] __arch_hweight32/64 x86 Sebastian Sumpf
@ 2023-06-26 21:06 ` Borislav Petkov
2023-06-26 21:42 ` Sebastian Sumpf
0 siblings, 1 reply; 7+ messages in thread
From: Borislav Petkov @ 2023-06-26 21:06 UTC (permalink / raw
To: Sebastian Sumpf; +Cc: tglx, mingo, dave.hansen, x86, linux-kernel
Hello,
On Mon, Jun 26, 2023 at 10:49:44PM +0200, Sebastian Sumpf wrote:
> The 'ALTERNATIVE' macro checks for the popcnt feature. In case this fails
> the '__sw_hweight' C-function is called from inline assembly with rax and
> rdi as intput/output operands. However, the code does not contain a clobber
> list of any callee safe registers that might be touched by the
> '__sw_height64' C-function.
Which registers are those? Can you be more specific?
> Therefore, these registers will not be restored
> upon function return by the compiler. This leads to varying
> exceptions/bad behavior caused by the thus corrupted registers later
> on.
How do I reproduce what you're observing so that I can take a look?
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug report] __arch_hweight32/64 x86
2023-06-26 21:06 ` Borislav Petkov
@ 2023-06-26 21:42 ` Sebastian Sumpf
2023-06-26 22:41 ` Dave Hansen
0 siblings, 1 reply; 7+ messages in thread
From: Sebastian Sumpf @ 2023-06-26 21:42 UTC (permalink / raw
To: Borislav Petkov; +Cc: tglx, mingo, dave.hansen, x86, linux-kernel
Hello Borislav,
On 6/26/23 23:06, Borislav Petkov wrote:
> Hello,
>
> On Mon, Jun 26, 2023 at 10:49:44PM +0200, Sebastian Sumpf wrote:
>> The 'ALTERNATIVE' macro checks for the popcnt feature. In case this fails
>> the '__sw_hweight' C-function is called from inline assembly with rax and
>> rdi as intput/output operands. However, the code does not contain a clobber
>> list of any callee safe registers that might be touched by the
>> '__sw_height64' C-function.
>> Which registers are those? Can you be more specific?
Registers that are free to use by a called C-function are: rcx, rdx, rsi, rdi,
and r8-r11 for x86_64. See the "System V Application Binary Interface
AMD64 Architecture Processor Supplement" [1] page 21. x86_32 I would have to
look up.
>> Therefore, these registers will not be restored
>> upon function return by the compiler. This leads to varying
>> exceptions/bad behavior caused by the thus corrupted registers later
>> on.
>
> How do I reproduce what you're observing so that I can take a look?
This is hard to tell, I would disable the " X86_FEATURE_POPCNT" feature and use
the ' CONFIG_ARCH_HAS_FAST_MULTIPLIER' option in order to use the multiplier
implementation in '__sw_hweight64' in 'lib/hweight.c' At least that is what
triggered it here.
Regards,
Sebastian
[1] http://www.uclibc.org/docs/psABI-x86_64.pdf
--
Sebastian Sumpf
Genode Labs
http://www.genode-labs.com · http://genode.org
Genode Labs GmbH · Amtsgericht Dresden · HRB 28424 · Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug report] __arch_hweight32/64 x86
2023-06-26 21:42 ` Sebastian Sumpf
@ 2023-06-26 22:41 ` Dave Hansen
2023-06-27 4:25 ` Borislav Petkov
0 siblings, 1 reply; 7+ messages in thread
From: Dave Hansen @ 2023-06-26 22:41 UTC (permalink / raw
To: Sebastian Sumpf, Borislav Petkov
Cc: tglx, mingo, dave.hansen, x86, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]
On 6/26/23 14:42, Sebastian Sumpf wrote:
>
>>> Therefore, these registers will not be restored
>>> upon function return by the compiler. This leads to varying
>>> exceptions/bad behavior caused by the thus corrupted registers later
>>> on.
>>
>> How do I reproduce what you're observing so that I can take a look?
>
> This is hard to tell, I would disable the " X86_FEATURE_POPCNT" feature
> and use the ' CONFIG_ARCH_HAS_FAST_MULTIPLIER' option in order to use
> the multiplier implementation in '__sw_hweight64' in 'lib/hweight.c' At
> least that is what triggered it here.
Looks like you'd also have to be using UML:
$ grep hweight lib/Makefile
obj-$(CONFIG_GENERIC_HWEIGHT) += hweight.o
$ grep -r GENERIC_HWEIGHT arch/x86
arch/x86/um/Kconfig:config GENERIC_HWEIGHT
I'm not even sure that UML needs GENERIC_HWEIGHT. From a quick glance,
it looks like x86 used to use GENERIC_HWEIGHT, but got arch-specific
versions later. UML just never moved over to the arch-specific versions.
I _think_ the attached patch might just fix the problems with the C
version and bring the x86/UML implementation back in line with the rest
of x86.
Thoughts?
[-- Attachment #2: um-hweight.patch --]
[-- Type: text/x-patch, Size: 289 bytes --]
diff --git a/arch/x86/um/Kconfig b/arch/x86/um/Kconfig
index 186f13268401..76d507860be4 100644
--- a/arch/x86/um/Kconfig
+++ b/arch/x86/um/Kconfig
@@ -44,6 +44,3 @@ config ARCH_HAS_SC_SIGNALS
config ARCH_REUSE_HOST_VSYSCALL_AREA
def_bool !64BIT
-
-config GENERIC_HWEIGHT
- def_bool y
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Bug report] __arch_hweight32/64 x86
2023-06-26 22:41 ` Dave Hansen
@ 2023-06-27 4:25 ` Borislav Petkov
2023-06-27 16:57 ` Sebastian Sumpf
2023-06-29 9:10 ` David Laight
0 siblings, 2 replies; 7+ messages in thread
From: Borislav Petkov @ 2023-06-27 4:25 UTC (permalink / raw
To: Dave Hansen, Richard Weinberger
Cc: Sebastian Sumpf, tglx, mingo, dave.hansen, x86, linux-kernel
On Mon, Jun 26, 2023 at 03:41:27PM -0700, Dave Hansen wrote:
> I'm not even sure that UML needs GENERIC_HWEIGHT. From a quick glance,
> it looks like x86 used to use GENERIC_HWEIGHT, but got arch-specific
> versions later. UML just never moved over to the arch-specific versions.
Thanks - that could very well be the explanation.
That bug report made me blink a couple of times since I did take extra
precaution to not clobber regs in arch/x86/lib/hweight.S as this was
part of the whole pain back then with calling a function from asm where
gcc doesn't even know we're calling a function, see:
f5967101e9de ("x86/hweight: Get rid of the special calling convention")
> I _think_ the attached patch might just fix the problems with the C
> version and bring the x86/UML implementation back in line with the rest
> of x86.
>
> Thoughts?
> diff --git a/arch/x86/um/Kconfig b/arch/x86/um/Kconfig
> index 186f13268401..76d507860be4 100644
> --- a/arch/x86/um/Kconfig
> +++ b/arch/x86/um/Kconfig
> @@ -44,6 +44,3 @@ config ARCH_HAS_SC_SIGNALS
>
> config ARCH_REUSE_HOST_VSYSCALL_AREA
> def_bool !64BIT
> -
> -config GENERIC_HWEIGHT
> - def_bool y
Yeah, we should do it. UML should not do anything different wrt calling
conventions so it should be able to handle the arch/x86/lib/hweight.S
versions just fine.
Richi?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug report] __arch_hweight32/64 x86
2023-06-27 4:25 ` Borislav Petkov
@ 2023-06-27 16:57 ` Sebastian Sumpf
2023-06-29 9:10 ` David Laight
1 sibling, 0 replies; 7+ messages in thread
From: Sebastian Sumpf @ 2023-06-27 16:57 UTC (permalink / raw
To: Borislav Petkov, Dave Hansen, Richard Weinberger
Cc: tglx, mingo, dave.hansen, x86, linux-kernel
On 6/27/23 06:25, Borislav Petkov wrote:
> On Mon, Jun 26, 2023 at 03:41:27PM -0700, Dave Hansen wrote:
>> I'm not even sure that UML needs GENERIC_HWEIGHT. From a quick glance,
>> it looks like x86 used to use GENERIC_HWEIGHT, but got arch-specific
>> versions later. UML just never moved over to the arch-specific versions.
>
> Thanks - that could very well be the explanation.
>
> That bug report made me blink a couple of times since I did take extra
> precaution to not clobber regs in arch/x86/lib/hweight.S as this was
> part of the whole pain back then with calling a function from asm where
> gcc doesn't even know we're calling a function, see:
>
> f5967101e9de ("x86/hweight: Get rid of the special calling convention")
>
>> I _think_ the attached patch might just fix the problems with the C
>> version and bring the x86/UML implementation back in line with the rest
>> of x86.
>>
>> Thoughts?
>
>> diff --git a/arch/x86/um/Kconfig b/arch/x86/um/Kconfig
>> index 186f13268401..76d507860be4 100644
>> --- a/arch/x86/um/Kconfig
>> +++ b/arch/x86/um/Kconfig
>> @@ -44,6 +44,3 @@ config ARCH_HAS_SC_SIGNALS
>>
>> config ARCH_REUSE_HOST_VSYSCALL_AREA
>> def_bool !64BIT
>> -
>> -config GENERIC_HWEIGHT
>> - def_bool y
>
> Yeah, we should do it. UML should not do anything different wrt calling
> conventions so it should be able to handle the arch/x86/lib/hweight.S
> versions just fine.
Thank you for the quick resolution! It fixes the problem for me and sorry for
not explaining well enough.
> Richi?
>
--
Sebastian Sumpf
Genode Labs
http://www.genode-labs.com · http://genode.org
Genode Labs GmbH · Amtsgericht Dresden · HRB 28424 · Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [Bug report] __arch_hweight32/64 x86
2023-06-27 4:25 ` Borislav Petkov
2023-06-27 16:57 ` Sebastian Sumpf
@ 2023-06-29 9:10 ` David Laight
1 sibling, 0 replies; 7+ messages in thread
From: David Laight @ 2023-06-29 9:10 UTC (permalink / raw
To: 'Borislav Petkov', Dave Hansen, Richard Weinberger
Cc: Sebastian Sumpf, tglx@linutronix.de, mingo@redhat.com,
dave.hansen@linux.intel.com, x86@kernel.org,
linux-kernel@vger.kernel.org
From: Borislav Petkov
> Sent: 27 June 2023 05:26
>
> On Mon, Jun 26, 2023 at 03:41:27PM -0700, Dave Hansen wrote:
> > I'm not even sure that UML needs GENERIC_HWEIGHT. From a quick glance,
> > it looks like x86 used to use GENERIC_HWEIGHT, but got arch-specific
> > versions later. UML just never moved over to the arch-specific versions.
>
> Thanks - that could very well be the explanation.
>
> That bug report made me blink a couple of times since I did take extra
> precaution to not clobber regs in arch/x86/lib/hweight.S as this was
> part of the whole pain back then with calling a function from asm where
> gcc doesn't even know we're calling a function, see:
Perhaps the asm called function should use a different global
label than the one in the C file.
Then it wouldn't be as fragile.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-06-29 9:13 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-26 20:49 [Bug report] __arch_hweight32/64 x86 Sebastian Sumpf
2023-06-26 21:06 ` Borislav Petkov
2023-06-26 21:42 ` Sebastian Sumpf
2023-06-26 22:41 ` Dave Hansen
2023-06-27 4:25 ` Borislav Petkov
2023-06-27 16:57 ` Sebastian Sumpf
2023-06-29 9:10 ` David Laight
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).