[PATCH] task_struct: stack_canary is not needed without CC_STACKPROTECTOR

[PATCH] task_struct: stack_canary is not needed without CC_STACKPROTECTOR

[Hiroshi Shimamoto]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

The field stack_canary is only used with CC_STACKPROTECTOR.
This patch reduces task_struct size without CC_STACKPROTECTOR.

Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
---
 include/linux/sched.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index af1e328..962e457 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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,
-- 
1.6.3.3

Re: [PATCH] task_struct: stack_canary is not needed without CC_STACKPROTECTOR

[Andi Kleen]

Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> writes:

> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
>
> The field stack_canary is only used with CC_STACKPROTECTOR.
> This patch reduces task_struct size without CC_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

-- 
ak@linux.intel.com -- Speaking for myself only.

[PATCH v2] task_struct: stack_canary is not needed without CC_STACKPROTECTOR

[Hiroshi Shimamoto]

Andi Kleen wrote:
> Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> writes:
> 
>> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
>>
>> The field stack_canary is only used with CC_STACKPROTECTOR.
>> This patch reduces task_struct size without CC_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.

Thanks for the comment, that's reasonable.
Here's the update.

====
From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

The field stack_canary is only used with CC_STACKPROTECTOR.
This patch reduces task_struct size without CC_STACKPROTECTOR.

Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
---
 include/linux/sched.h |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index af1e328..9a1d68d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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. */
-- 
1.6.3.3

Re: [PATCH v2] task_struct: stack_canary is not needed without CC_STACKPROTECTOR

[Ingo Molnar]

* Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> wrote:

> Andi Kleen wrote:
> > Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> writes:
> > 
> >> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> >>
> >> The field stack_canary is only used with CC_STACKPROTECTOR.
> >> This patch reduces task_struct size without CC_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. )

> > 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 ... ;-)

> 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

[tip-bot for Hiroshi Shimamoto]

Commit-ID:  1314562a9ae5f39f6f595656023c1baf970831ef
Gitweb:     http://git.kernel.org/tip/1314562a9ae5f39f6f595656023c1baf970831ef
Author:     Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
AuthorDate: Tue, 18 Aug 2009 15:06:02 +0900
Committer:  Ingo Molnar <mingo@elte.hu>
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.

Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
LKML-Reference: <4A8A44CA.2020701@ct.jp.nec.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 include/linux/sched.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 195d72d..7bc2d92 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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,