Messages in this thread | | | Date | Tue, 18 Aug 2009 15:43:28 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH v2] task_struct: stack_canary is not needed without CC_STACKPROTECTOR |
| |
* 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
| |