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

[Ingo Molnar <mingo@elte.hu>]

* 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