lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 16 Apr 2020 07:50:00 +0200 From: Christophe Leroy <christophe.leroy@....fr> To: Segher Boessenkool <segher@...nel.crashing.org> Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>, Paul Mackerras <paulus@...ba.org>, Michael Ellerman <mpe@...erman.id.au>, npiggin@...il.com, linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org Subject: Re: [PATCH] powerpc/uaccess: Use flexible addressing with __put_user()/__get_user() Hi, Le 16/04/2020 à 00:06, Segher Boessenkool a écrit : > Hi! > > On Wed, Apr 15, 2020 at 09:20:26AM +0000, Christophe Leroy wrote: >> At the time being, __put_user()/__get_user() and friends only use >> register indirect with immediate index addressing, with the index >> set to 0. Ex: >> >> lwz reg1, 0(reg2) > > This is called a "D-form" instruction, or sometimes "offset addressing". > Don't talk about an "index", it confuses things, because the *other* > kind is called "indexed" already, also in the ISA docs! (X-form, aka > indexed addressing, [reg+reg], where D-form does [reg+imm], and both > forms can do [reg]). In the "Programming Environments Manual for 32-Bit Implementations of the PowerPC™ Architecture", they list the following addressing modes: Load and store operations have three categories of effective address generation that depend on the operands specified: • Register indirect with immediate index mode • Register indirect with index mode • Register indirect mode > >> Give the compiler the opportunity to use other adressing modes >> whenever possible, to get more optimised code. > > Great :-) > >> --- a/arch/powerpc/include/asm/uaccess.h >> +++ b/arch/powerpc/include/asm/uaccess.h >> @@ -114,7 +114,7 @@ extern long __put_user_bad(void); >> */ >> #define __put_user_asm(x, addr, err, op) \ >> __asm__ __volatile__( \ >> - "1: " op " %1,0(%2) # put_user\n" \ >> + "1: " op "%U2%X2 %1,%2 # put_user\n" \ >> "2:\n" \ >> ".section .fixup,\"ax\"\n" \ >> "3: li %0,%3\n" \ >> @@ -122,7 +122,7 @@ extern long __put_user_bad(void); >> ".previous\n" \ >> EX_TABLE(1b, 3b) \ >> : "=r" (err) \ >> - : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err)) >> + : "r" (x), "m" (*addr), "i" (-EFAULT), "0" (err)) > > %Un on an "m" operand doesn't do much: you need to make it "m<>" if you > want pre-modify ("update") insns to be generated. (You then will want > to make sure that operand is used in a way GCC can understand; since it > is used only once here, that works fine). Ah ? Indeed I got the idea from include/asm/io.h where there is: #define DEF_MMIO_IN_D(name, size, insn) \ static inline u##size name(const volatile u##size __iomem *addr) \ { \ u##size ret; \ __asm__ __volatile__("sync;"#insn"%U1%X1 %0,%1;twi 0,%0,0;isync"\ : "=r" (ret) : "m" (*addr) : "memory"); \ return ret; \ } It should be "m<>" there as well ? > >> @@ -130,8 +130,8 @@ extern long __put_user_bad(void); >> #else /* __powerpc64__ */ >> #define __put_user_asm2(x, addr, err) \ >> __asm__ __volatile__( \ >> - "1: stw %1,0(%2)\n" \ >> - "2: stw %1+1,4(%2)\n" \ >> + "1: stw%U2%X2 %1,%2\n" \ >> + "2: stw%U2%X2 %L1,%L2\n" \ >> "3:\n" \ >> ".section .fixup,\"ax\"\n" \ >> "4: li %0,%3\n" \ >> @@ -140,7 +140,7 @@ extern long __put_user_bad(void); >> EX_TABLE(1b, 4b) \ >> EX_TABLE(2b, 4b) \ >> : "=r" (err) \ >> - : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err)) >> + : "r" (x), "m" (*addr), "i" (-EFAULT), "0" (err)) > > Here, it doesn't work. You don't want two consecutive update insns in > any case. Easiest is to just not use "m<>", and then, don't use %Un > (which won't do anything, but it is confusing). Can't we leave the Un on the second stw ? > > Same for the reads. > > Rest looks fine, and update should be good with that fixed as said. > > Reviewed-by: Segher Boessenkool <segher@...nel.crashing.org> > > > Segher > Thanks for the review Christophe
Powered by blists - more mailing lists