From: Segher Boessenkool <segher@kernel.crashing.org>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: Peter Zijlstra <peterz@infradead.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
dja@axtens.net, linux-kernel@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, christophe.leroy@c-s.fr,
linux-arch@vger.kernel.org, Will Deacon <will@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Arnd Bergmann <arnd@arndb.de>,
Christian Borntraeger <borntraeger@de.ibm.com>
Subject: Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))
Date: Thu, 12 Dec 2019 09:10:51 -0600 [thread overview]
Message-ID: <20191212151051.GF3152@gate.crashing.org> (raw)
In-Reply-To: <875zimp0ay.fsf@mpe.ellerman.id.au>
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 does not
do an atomic access at all. Either that is a good idea or not, but we
probably should not change the behaviour here, not without benchmarking
anyway.
> For example, comparing an out-of-line copy of the generic and ppc
> versions of test_and_set_bit_lock():
(With what GCC version, and what exact flags?)
(A stand-alone testcase would be nice too, btw).
(Michael gave me one, thanks!)
> If you squint, the generated code for the actual logic is pretty similar, but
> the stack protector gunk makes a big mess.
And with stack protector it cannot shrink-wrap the exit, one of the bigger
performance costs of the stack protector. The extra branch in the generic
code isn't fun either (but maybe it is good for performance?
> It's particularly bad here
> because the ppc version doesn't even need a stack frame.
You are hit by this:
if (... || (RECORD_OR_UNION_TYPE_P (var_type)
&& record_or_union_type_has_array_p (var_type)) ...)
(in the GCC code, stack_protect_decl_p (), cfgexpand.c)
for the variable __u from
#define __READ_ONCE(x, check) \
({ \
union { typeof(x) __val; char __c[1]; } __u; \
__read_once_size(&(x), __u.__c, sizeof(x)); \
smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
__u.__val; \
})
This is all optimised away later, but at the point this decision is made
GCC does not know that.
> So READ_ONCE() + STACKPROTECTOR_STRONG is problematic. The root cause is
> presumably that READ_ONCE() does an access to an on-stack variable,
> which triggers the heuristics in the compiler that the stack needs
> protecting.
Not exactly, but the problem is READ_ONCE alright.
> It seems like a compiler "mis-feature" that a constant-sized access to the stack
> triggers the stack protector logic, especially when the access is eventually
> optimised away. But I guess that's probably what we get for doing tricks like
> READ_ONCE() in the first place :/
__c is an array. That is all that matters. I don't think it is very
reasonable to fault GCC for this.
> I tried going back to the version of READ_ONCE() that doesn't use a
> union, ie. effectively reverting dd36929720f4 ("kernel: make READ_ONCE()
> valid on const arguments") to get:
>
> #define READ_ONCE(x) \
> ({ typeof(x) __val; __read_once_size(&x, &__val, sizeof(__val)); __val; })
With that, it is that the address of __val is taken:
...
|| TREE_ADDRESSABLE (var)
...
> But it makes no difference, the stack protector stuff still triggers. So
> I guess it's simply taking the address of a stack variable that triggers
> it.
Not in the earlier testcase. Btw, there is no such thing as a "stack
variable" at that point in the compiler: it just is a local var.
> There seems to be a function attribute to enable stack protector for a
> function, but not one to disable it:
> https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/Common-Function-Attributes.html#index-stack_005fprotect-function-attribute
Yes.
> That may not be a good solution even if it did exist, because it would
> potentially disable stack protector in places where we do want it
> enabled.
Right, I don't think we want that, such an attribute invites people to
write dangerous code. (You already can just put the functions that you
want to be unsafe in a separate source file... It sounds even sillier
that way, heh).
Segher
next prev parent reply other threads:[~2019-12-12 15:11 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-06 12:46 [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops) Michael Ellerman
2019-12-06 13:16 ` Peter Zijlstra
2019-12-10 5:38 ` Michael Ellerman
2019-12-10 10:15 ` Peter Zijlstra
2019-12-11 0:29 ` Michael Ellerman
2019-12-12 5:42 ` READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops)) Michael Ellerman
2019-12-12 8:01 ` Peter Zijlstra
2019-12-12 10:07 ` Will Deacon
2019-12-12 10:46 ` Peter Zijlstra
2019-12-12 17:04 ` Will Deacon
2019-12-12 17:16 ` Will Deacon
2019-12-12 17:41 ` Linus Torvalds
2019-12-12 17:50 ` Segher Boessenkool
2019-12-12 18:06 ` Will Deacon
2019-12-12 18:29 ` Christian Borntraeger
2019-12-12 18:43 ` Linus Torvalds
2019-12-12 19:34 ` Will Deacon
2019-12-12 20:21 ` Peter Zijlstra
2019-12-12 20:53 ` Peter Zijlstra
2019-12-13 10:47 ` Luc Van Oostenryck
2019-12-13 12:56 ` Peter Zijlstra
2019-12-13 14:28 ` Luc Van Oostenryck
2019-12-12 20:49 ` Linus Torvalds
2019-12-13 13:17 ` Arnd Bergmann
2019-12-13 21:32 ` Arnd Bergmann
2019-12-13 22:01 ` Linus Torvalds
2019-12-16 10:28 ` Will Deacon
2019-12-16 11:47 ` Peter Zijlstra
2019-12-16 12:06 ` Arnd Bergmann
2019-12-17 17:07 ` Will Deacon
2019-12-17 18:04 ` Linus Torvalds
2019-12-17 18:05 ` Linus Torvalds
2019-12-17 18:31 ` Will Deacon
2019-12-17 18:32 ` Linus Torvalds
2019-12-18 12:17 ` Michael Ellerman
2019-12-19 12:11 ` Will Deacon
2019-12-18 10:22 ` Christian Borntraeger
2019-12-18 10:35 ` Will Deacon
2019-12-13 12:07 ` Michael Ellerman
2019-12-13 13:53 ` Segher Boessenkool
2019-12-13 21:06 ` Michael Ellerman
2019-12-12 15:10 ` Segher Boessenkool [this message]
2019-12-06 22:15 ` [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops) pr-tracker-bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191212151051.GF3152@gate.crashing.org \
--to=segher@kernel.crashing.org \
--cc=arnd@arndb.de \
--cc=borntraeger@de.ibm.com \
--cc=christophe.leroy@c-s.fr \
--cc=dja@axtens.net \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mark.rutland@arm.com \
--cc=mpe@ellerman.id.au \
--cc=peterz@infradead.org \
--cc=torvalds@linux-foundation.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 NNTP newsgroup(s).