Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-19 Thread Will Deacon
On Tue, Dec 17, 2019 at 10:32:35AM -0800, Linus Torvalds wrote: > On Tue, Dec 17, 2019 at 10:04 AM Linus Torvalds > wrote: > > > > Let me think about it. > > How about we just get rid of the union entirely, and just use > 'unsigned long' or 'unsigned long long' depending on the size. > >

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-18 Thread Michael Ellerman
Linus Torvalds writes: > On Tue, Dec 17, 2019 at 10:04 AM Linus Torvalds > wrote: >> >> Let me think about it. > > How about we just get rid of the union entirely, and just use > 'unsigned long' or 'unsigned long long' depending on the size. > > Something like the attached patch - it still

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-18 Thread Will Deacon
On Wed, Dec 18, 2019 at 11:22:05AM +0100, Christian Borntraeger wrote: > On 12.12.19 21:49, Linus Torvalds wrote: > > On Thu, Dec 12, 2019 at 11:34 AM Will Deacon wrote: > >> > >> The root of my concern in all of this, and what started me looking at it in > >> the first place, is the interaction

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-18 Thread Christian Borntraeger
On 12.12.19 21:49, Linus Torvalds wrote: > On Thu, Dec 12, 2019 at 11:34 AM Will Deacon wrote: >> >> The root of my concern in all of this, and what started me looking at it in >> the first place, is the interaction with 'typeof()'. Inheriting 'volatile' >> for a pointer means that local

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-17 Thread Linus Torvalds
On Tue, Dec 17, 2019 at 10:04 AM Linus Torvalds wrote: > > Let me think about it. How about we just get rid of the union entirely, and just use 'unsigned long' or 'unsigned long long' depending on the size. Something like the attached patch - it still requires that it be an arithmetic type, but

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-17 Thread Will Deacon
On Tue, Dec 17, 2019 at 10:05:53AM -0800, Linus Torvalds wrote: > On Tue, Dec 17, 2019 at 10:04 AM Linus Torvalds > wrote: > > > > Let me think about it. > > .. and in the short term, maybe for code generation, the right thing > is to just do the cast in the bitops, where we can just cast to >

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-17 Thread Linus Torvalds
On Tue, Dec 17, 2019 at 10:04 AM Linus Torvalds wrote: > > Let me think about it. .. and in the short term, maybe for code generation, the right thing is to just do the cast in the bitops, where we can just cast to "unsigned long *" and remove the volatile that way. I'm still hoping there's a

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-17 Thread Linus Torvalds
On Tue, Dec 17, 2019 at 9:07 AM Will Deacon wrote: > > However, I'm really banging my head against the compiler trying to get > your trick above to work for pointer types when the pointed-to-type is > not defined. You are right, of course. The trick works fine with arithmetic types, but since it

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-17 Thread Will Deacon
On Thu, Dec 12, 2019 at 12:49:52PM -0800, Linus Torvalds wrote: > On Thu, Dec 12, 2019 at 11:34 AM Will Deacon wrote: > > > > The root of my concern in all of this, and what started me looking at it in > > the first place, is the interaction with 'typeof()'. Inheriting 'volatile' > > for a

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-16 Thread Arnd Bergmann
On Mon, Dec 16, 2019 at 11:28 AM Will Deacon wrote: > On Fri, Dec 13, 2019 at 02:17:08PM +0100, Arnd Bergmann wrote: > > On Thu, Dec 12, 2019 at 9:50 PM Linus Torvalds > > wrote: > > > On Thu, Dec 12, 2019 at 11:34 AM Will Deacon wrote: > > > > The root of my concern in all of this, and what

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-16 Thread Peter Zijlstra
On Mon, Dec 16, 2019 at 10:28:06AM +, Will Deacon wrote: > However, enabling this for 32-bit ARM is total carnage; as Linus mentioned, > a whole bunch of code appears to be relying on atomic 64-bit access of > READ_ONCE(); the perf ring buffer, io_uring, the scheduler, pm_runtime, > cpuidle,

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-16 Thread Will Deacon
On Fri, Dec 13, 2019 at 02:17:08PM +0100, Arnd Bergmann wrote: > On Thu, Dec 12, 2019 at 9:50 PM Linus Torvalds > wrote: > > On Thu, Dec 12, 2019 at 11:34 AM Will Deacon wrote: > > > The root of my concern in all of this, and what started me looking at it > > > in > > > the first place, is the

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-13 Thread Linus Torvalds
On Fri, Dec 13, 2019 at 1:33 PM Arnd Bergmann wrote: > > A few hundred randconfig (x86, arm32 and arm64) builds later I > still only found one other instance: Just send me the pull request to READ_ONCE() and WRITE_ONCE() be arithmetic types, and your two trivial fixes, and let's get this over

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-13 Thread Arnd Bergmann
On Fri, Dec 13, 2019 at 2:17 PM Arnd Bergmann wrote: > > On Thu, Dec 12, 2019 at 9:50 PM Linus Torvalds > wrote: > I'll have my randconfig builder look for instances, so far I found one, > see below. My feeling is that it would be better to enforce at least > the size being a 1/2/4/8, to avoid

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-13 Thread Michael Ellerman
Segher Boessenkool writes: > Hi! > > On Fri, Dec 13, 2019 at 11:07:55PM +1100, Michael Ellerman wrote: >> I tried this: >> >> > @@ -295,6 +296,23 @@ void __write_once_size(volatile void *p, void *res, >> > int size) >> > */ >> > #define READ_ONCE_NOCHECK(x) __READ_ONCE(x, 0) >> > >> >

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-13 Thread Luc Van Oostenryck
On Fri, Dec 13, 2019 at 01:56:18PM +0100, Peter Zijlstra wrote: > > Excellent! I had to change it to something like: > > #define unqual_typeof(x)typeof(({_Atomic typeof(x) ___x __maybe_unused; > ___x; })) > > but that does indeed work! > > Now I suppose we should wrap that in a symbol

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-13 Thread Segher Boessenkool
Hi! On Fri, Dec 13, 2019 at 11:07:55PM +1100, Michael Ellerman wrote: > I tried this: > > > @@ -295,6 +296,23 @@ void __write_once_size(volatile void *p, void *res, > > int size) > > */ > > #define READ_ONCE_NOCHECK(x) __READ_ONCE(x, 0) > > > > +#else /* GCC_VERSION < 40800 */ > > + > >

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-13 Thread Arnd Bergmann
On Thu, Dec 12, 2019 at 9:50 PM Linus Torvalds wrote: > On Thu, Dec 12, 2019 at 11:34 AM Will Deacon wrote: > > The root of my concern in all of this, and what started me looking at it in > > the first place, is the interaction with 'typeof()'. Inheriting 'volatile' > > for a pointer means that

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-13 Thread Peter Zijlstra
On Fri, Dec 13, 2019 at 11:47:06AM +0100, Luc Van Oostenryck wrote: > On Thu, Dec 12, 2019 at 09:53:38PM +0100, Peter Zijlstra wrote: > > Now, looking at the current GCC source: > > > > > > https://github.com/gcc-mirror/gcc/blob/97d7270f894395e513667a031a0c309d1819d05e/gcc/c/c-parser.c#L3707 >

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-13 Thread Michael Ellerman
Peter Zijlstra writes: > On Thu, Dec 12, 2019 at 10:07:56AM +, Will Deacon wrote: > >> > So your proposed change _should_ be fine. Will, I'm assuming you never >> > saw this on your ARGH64 builds when you did this code ? >> >> I did see it, but (a) looking at the code out-of-line makes it

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-13 Thread Luc Van Oostenryck
On Thu, Dec 12, 2019 at 09:53:38PM +0100, Peter Zijlstra wrote: > Now, looking at the current GCC source: > > > https://github.com/gcc-mirror/gcc/blob/97d7270f894395e513667a031a0c309d1819d05e/gcc/c/c-parser.c#L3707 > > it seems that __typeof__() is supposed to strip all qualifiers from >

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Peter Zijlstra
On Thu, Dec 12, 2019 at 09:21:57PM +0100, Peter Zijlstra wrote: > On Thu, Dec 12, 2019 at 07:34:01PM +, Will Deacon wrote: > > void ool_store_release(volatile unsigned long *ptr, unsigned long val) > > { > > smp_store_release(ptr, val); > > } > > > > : > >0:

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Linus Torvalds
On Thu, Dec 12, 2019 at 11:34 AM Will Deacon wrote: > > The root of my concern in all of this, and what started me looking at it in > the first place, is the interaction with 'typeof()'. Inheriting 'volatile' > for a pointer means that local variables in macros declared using typeof() > suddenly

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Peter Zijlstra
On Thu, Dec 12, 2019 at 07:34:01PM +, Will Deacon wrote: > void ool_store_release(volatile unsigned long *ptr, unsigned long val) > { > smp_store_release(ptr, val); > } > > : >0: a9be7bfdstp x29, x30, [sp, #-32]! >4: 9002adrpx2, 0

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Will Deacon
Hi Linus, On Thu, Dec 12, 2019 at 10:43:05AM -0800, Linus Torvalds wrote: > On Thu, Dec 12, 2019 at 10:06 AM Will Deacon wrote: > > > > I'm currently trying to solve the issue by removing volatile from the bitop > > function signatures > > I really think that's the wrong thing to do. > > The

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Linus Torvalds
On Thu, Dec 12, 2019 at 10:06 AM Will Deacon wrote: > > I'm currently trying to solve the issue by removing volatile from the bitop > function signatures I really think that's the wrong thing to do. The bitop signature really should be "volatile" (and it should be "const volatile" for test_bit,

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Christian Borntraeger
On 12.12.19 19:06, Will Deacon wrote: > On Thu, Dec 12, 2019 at 09:41:32AM -0800, Linus Torvalds wrote: >> On Thu, Dec 12, 2019 at 2:46 AM Peter Zijlstra wrote: >>> >>> +#ifdef GCC_VERSION < 40800 >> >> Where does that 4.8 version check come from, and why? >> >> Yeah, I know, but this really

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Will Deacon
On Thu, Dec 12, 2019 at 09:41:32AM -0800, Linus Torvalds wrote: > On Thu, Dec 12, 2019 at 2:46 AM Peter Zijlstra wrote: > > > > +#ifdef GCC_VERSION < 40800 > > Where does that 4.8 version check come from, and why? > > Yeah, I know, but this really wants a comment. Sadly it looks like gcc >

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Segher Boessenkool
On Thu, Dec 12, 2019 at 09:41:32AM -0800, Linus Torvalds wrote: > Yeah, I know, but this really wants a comment. Sadly it looks like gcc > bugzilla is down, so > >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145 > > currently gives an "Internal Server Error" for me. We're being DoSsed

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Linus Torvalds
On Thu, Dec 12, 2019 at 2:46 AM Peter Zijlstra wrote: > > +#ifdef GCC_VERSION < 40800 Where does that 4.8 version check come from, and why? Yeah, I know, but this really wants a comment. Sadly it looks like gcc bugzilla is down, so https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Will Deacon
On Thu, Dec 12, 2019 at 05:04:27PM +, Will Deacon wrote: > On Thu, Dec 12, 2019 at 11:46:10AM +0100, Peter Zijlstra wrote: > > On Thu, Dec 12, 2019 at 10:07:56AM +, Will Deacon wrote: > > > > > > So your proposed change _should_ be fine. Will, I'm assuming you never > > > > saw this on

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Will Deacon
On Thu, Dec 12, 2019 at 11:46:10AM +0100, Peter Zijlstra wrote: > On Thu, Dec 12, 2019 at 10:07:56AM +, Will Deacon wrote: > > > > So your proposed change _should_ be fine. Will, I'm assuming you never > > > saw this on your ARGH64 builds when you did this code ? > > > > I did see it, but

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Segher Boessenkool
Hi, On Thu, Dec 12, 2019 at 04:42:13PM +1100, Michael Ellerman wrote: > Some of the generic versions don't generate good code compared to our > versions, but that's because READ_ONCE() is triggering stack protector > to be enabled. The *big* difference is the generic code has a special path that

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Peter Zijlstra
On Thu, Dec 12, 2019 at 10:07:56AM +, Will Deacon wrote: > > So your proposed change _should_ be fine. Will, I'm assuming you never > > saw this on your ARGH64 builds when you did this code ? > > I did see it, but (a) looking at the code out-of-line makes it look a lot > worse than it

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Will Deacon
On Thu, Dec 12, 2019 at 09:01:05AM +0100, Peter Zijlstra wrote: > On Thu, Dec 12, 2019 at 04:42:13PM +1100, Michael Ellerman wrote: > > Peter Zijlstra writes: > > > On Fri, Dec 06, 2019 at 11:46:11PM +1100, Michael Ellerman wrote: > > Some of the generic versions don't generate good code compared

Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-12 Thread Peter Zijlstra
On Thu, Dec 12, 2019 at 04:42:13PM +1100, Michael Ellerman wrote: > [ trimmed CC a bit ] > > Peter Zijlstra writes: > > On Fri, Dec 06, 2019 at 11:46:11PM +1100, Michael Ellerman wrote: > ... > > you write: > > > > "Currently bitops-instrumented.h assumes that the architecture provides > >

READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))

2019-12-11 Thread Michael Ellerman
[ trimmed CC a bit ] Peter Zijlstra writes: > On Fri, Dec 06, 2019 at 11:46:11PM +1100, Michael Ellerman wrote: ... > you write: > > "Currently bitops-instrumented.h assumes that the architecture provides > atomic, non-atomic and locking bitops (e.g. both set_bit and __set_bit). > This is true