task_struct: stack_canary is not needed without CC_STACKPROTECTOR
PATCH 1/1 task_struct: stack_canary is not needed without CC_STACKPROTECTOR
From: Hiroshi Shimamoto <[email protected]>
The field stack_canary is only used with CC_STACKPROTECTOR. This patch reduces task_struct size without CC_STACKPROTECTOR.
--- include/linux/sched.h | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
include/linux/sched.h +2 -0
@@ -1247,8 +1247,10 @@ struct task_struct {pid_t pid;pid_t tgid; +#ifdef CONFIG_CC_STACKPROTECTOR /* Canary value for the -fstack-protector gcc feature */ unsigned long stack_canary; +#endif /* * pointers to (original) parent process, youngest child, younger sibling,
Signature · 1 line
1.6.3.3
Quoted 4 lines · Hiroshi Shimamoto
From: Hiroshi Shimamoto <[email protected]>
The field
stack_canaryis only used withCC_STACKPROTECTOR. This patch reducestask_structsize withoutCC_STACKPROTECTOR.
Adding a ifdef in the middle of a widely used structure is nasty. It means that if someone changes the option then the newly loaded modules don't work anymore (yes that's not officially supported, but works most of the time and is often convenient in practice)
So when you add a ifdef please move the field to the end at least.
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only.
PATCH v2 1/1 task_struct: stack_canary is not needed without CC_STACKPROTECTOR
Quoted 7 lines · Andi Kleen
Quoted 4 lines · Hiroshi Shimamoto
From: Hiroshi Shimamoto <[email protected]>
The field
stack_canaryis only used withCC_STACKPROTECTOR. This patch reducestask_structsize withoutCC_STACKPROTECTOR.
Quoted 6 lines
Adding a ifdef in the middle of a widely used structure is nasty. It means that if someone changes the option then the newly loaded modules don't work anymore (yes that's not officially supported, but works most of the time and is often convenient in practice)
So when you add a ifdef please move the field to the end at least.
Thanks for the comment, that's reasonable.
Here's the update.
====
From: Hiroshi Shimamoto <[email protected]>
The field stack_canary is only used with CC_STACKPROTECTOR. This patch reduces task_struct size without CC_STACKPROTECTOR.
--- include/linux/sched.h | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
include/linux/sched.h +4 -3
@@ -1247,9 +1247,6 @@ struct task_struct {pid_t pid;pid_t tgid; -/* Canary value for the -fstack-protector gcc feature */ -unsigned long stack_canary; -/* * pointers to (original) parent process, youngest child, younger sibling, * older sibling, respectively. (p->father can be replaced with
@@ -1494,6 +1491,10 @@ struct task_struct {/* bitmask of trace recursion */ unsigned long trace_recursion;#endif /* CONFIG_TRACING */ +#ifdef CONFIG_CC_STACKPROTECTOR +/* Canary value for the -fstack-protector gcc feature */ +unsigned long stack_canary; +#endif };/* Future-safe accessor for struct task_struct's cpus_allowed. */
Signature · 1 line
1.6.3.3
Ingo Molnar
AppliedQuoted 5 lines · Hiroshi Shimamoto
Quoted 4 lines · Andi Kleen
Quoted 2 lines · Hiroshi Shimamoto
From: Hiroshi Shimamoto <[email protected]>
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
The field
stack_canaryis only used withCC_STACKPROTECTOR. This patch reducestask_structsize withoutCC_STACKPROTECTOR.Adding a ifdef in the middle of a widely used structure is nasty. It means that if someone changes the option then the newly loaded modules don't work anymore (yes that's not officially supported, but works most of the time and is often convenient in practice)
( Ugh. Not having clean builds and clean modules is utterly
dangerous and taints the kernel. I ignore all bugreports from people that do that - a kernel that has been butchered like that is just not trustable. )
Quoted 2 lines
Quoted 2 lines
So when you add a ifdef please move the field to the end at least.
Moving the stack canary it last is futile and makes no sense whatsoever, for three independent reasons:
It's stupidly shortsighted: there's 20 other config options in the middle of struct task struct already. Half of struct task_struct is #ifdef-ed, and there can only be one 'last' field.
It's merge unfriendly: moving fields last in structs can cause patch conflict problems: new subsystems/features tend to append to task_struct, colliding with this patch. task_struct is frequently patched.
It hurts performance: the canary is used very frequently on stackprotector kernels and has been placed on a hot cacheline intentionally. Moving it last just adds a small but real performance regression.
Really, Andi, if you give 'advice' like this you should be declared armed and dangerous ... ;-)
Quoted 1 line
Here's the update.
I've applied v1, thanks Hiroshi!
Ingo
[tip:sched/core] sched, task_struct: stack_canary is not needed without CC_STACKPROTECTOR
In reply to Hiroshi ShimamotoCommit-ID: 1314562a9ae5f39f6f595656023c1baf970831ef Gitweb: http://git.kernel.org/tip/1314562a9ae5f39f6f595656023c1baf970831ef Author: Hiroshi Shimamoto <[email protected]> AuthorDate: Tue, 18 Aug 2009 15:06:02 +0900 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 18 Aug 2009 15:45:11 +0200
sched, task_struct: stack_canary is not needed without CC_STACKPROTECTOR
The field stack_canary is only used with CC_STACKPROTECTOR. This patch reduces task_struct size without CC_STACKPROTECTOR.
LKML-Reference: <[email protected]>
--- include/linux/sched.h | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
include/linux/sched.h +2 -0
@@ -1237,8 +1237,10 @@ struct task_struct {pid_t pid;pid_t tgid; +#ifdef CONFIG_CC_STACKPROTECTOR /* Canary value for the -fstack-protector gcc feature */ unsigned long stack_canary; +#endif /* * pointers to (original) parent process, youngest child, younger sibling,