* [PATCH] task_struct: stack_canary is not needed without CC_STACKPROTECTOR
@ 2009-08-18 6:06 Hiroshi Shimamoto
2009-08-18 7:34 ` Andi Kleen
2009-08-18 13:48 ` [tip:sched/core] sched, " tip-bot for Hiroshi Shimamoto
0 siblings, 2 replies; 5+ messages in thread
From: Hiroshi Shimamoto @ 2009-08-18 6:06 UTC (permalink / raw)
To: Ingo Molnar; +Cc: linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] task_struct: stack_canary is not needed without CC_STACKPROTECTOR
2009-08-18 6:06 [PATCH] task_struct: stack_canary is not needed without CC_STACKPROTECTOR Hiroshi Shimamoto
@ 2009-08-18 7:34 ` Andi Kleen
2009-08-18 8:01 ` [PATCH v2] " Hiroshi Shimamoto
2009-08-18 13:48 ` [tip:sched/core] sched, " tip-bot for Hiroshi Shimamoto
1 sibling, 1 reply; 5+ messages in thread
From: Andi Kleen @ 2009-08-18 7:34 UTC (permalink / raw)
To: Hiroshi Shimamoto; +Cc: Ingo Molnar, linux-kernel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] task_struct: stack_canary is not needed without CC_STACKPROTECTOR
2009-08-18 7:34 ` Andi Kleen
@ 2009-08-18 8:01 ` Hiroshi Shimamoto
2009-08-18 13:43 ` Ingo Molnar
0 siblings, 1 reply; 5+ messages in thread
From: Hiroshi Shimamoto @ 2009-08-18 8:01 UTC (permalink / raw)
To: Andi Kleen; +Cc: Ingo Molnar, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] task_struct: stack_canary is not needed without CC_STACKPROTECTOR
2009-08-18 8:01 ` [PATCH v2] " Hiroshi Shimamoto
@ 2009-08-18 13:43 ` Ingo Molnar
0 siblings, 0 replies; 5+ messages in thread
From: Ingo Molnar @ 2009-08-18 13:43 UTC (permalink / raw)
To: Hiroshi Shimamoto
Cc: Andi Kleen, linux-kernel, Arjan van de Ven, Thomas Gleixner
* 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tip:sched/core] sched, task_struct: stack_canary is not needed without CC_STACKPROTECTOR
2009-08-18 6:06 [PATCH] task_struct: stack_canary is not needed without CC_STACKPROTECTOR Hiroshi Shimamoto
2009-08-18 7:34 ` Andi Kleen
@ 2009-08-18 13:48 ` tip-bot for Hiroshi Shimamoto
1 sibling, 0 replies; 5+ messages in thread
From: tip-bot for Hiroshi Shimamoto @ 2009-08-18 13:48 UTC (permalink / raw)
To: linux-tip-commits; +Cc: linux-kernel, h-shimamoto, hpa, mingo, tglx, mingo
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,
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-08-18 13:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-08-18 6:06 [PATCH] task_struct: stack_canary is not needed without CC_STACKPROTECTOR Hiroshi Shimamoto
2009-08-18 7:34 ` Andi Kleen
2009-08-18 8:01 ` [PATCH v2] " Hiroshi Shimamoto
2009-08-18 13:43 ` Ingo Molnar
2009-08-18 13:48 ` [tip:sched/core] sched, " tip-bot for Hiroshi Shimamoto
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.