Xen devel - Jan 2008 - [PATCH] stack overflow during pv-guest restore

When secondary cpus are initialized during an i386 pv-guest restore
(due to save/restore or live migration), and the guest has
a load that generates a fair number of interrupts (e.g., parallel kernel make),
a stack overflow can occur because cpu_initialize_context() has
a 2800 byte structure it declares on its stack.  linux-i386 has 4K stacks, by
default.
Using 2800 bytes out of 4K by a single function in a call list isn''t
nice;
add the beginning of interrupt handling at just the right point, before the
switch to the interrupt stack is made... and... the scale tips...

Simple fix: malloc & free structure as needed.

Would fail save/restore testing of an i386-guest running a parallel, kernel make
after
50->100 save/restores;  with the fix, haven''t seen it fail after 650
save/restores.

Note: this is a basic port of this function in Jeremy Fitzharinge''s Xen
implementation of pv-ops
       in upstream Linux, part of 15/32 patch, Xen SMP guest support.

Original patch done on rhel5; did a simple diff & merge to
xen-unstable''s version
of smpboot.c to generate the attached patch, so it cleanly applies; but
haven''t built/run/tested the xen-unstable version.

Signed-off-by: Donald Dutile <ddutile@redhat.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

I think this can be fixed with a much smaller change: Since the only caller
of cpu_initialize_context() is __cpu_up(), and since __cpu_up() invocation
is serialized, the variable in question could simply be static.

Jan
>>> Don Dutile <ddutile@redhat.com> 31.01.08 21:05 >>>
When secondary cpus are initialized during an i386 pv-guest restore
(due to save/restore or live migration), and the guest has
a load that generates a fair number of interrupts (e.g., parallel kernel make),
a stack overflow can occur because cpu_initialize_context() has
a 2800 byte structure it declares on its stack.  linux-i386 has 4K stacks, by
default.
Using 2800 bytes out of 4K by a single function in a call list isn''t
nice;
add the beginning of interrupt handling at just the right point, before the
switch to the interrupt stack is made... and... the scale tips...

Simple fix: malloc & free structure as needed.

Would fail save/restore testing of an i386-guest running a parallel, kernel make
after
50->100 save/restores;  with the fix, haven''t seen it fail after 650
save/restores.

Note: this is a basic port of this function in Jeremy Fitzharinge''s Xen
implementation of pv-ops
       in upstream Linux, part of 15/32 patch, Xen SMP guest support.

Original patch done on rhel5; did a simple diff & merge to
xen-unstable''s version
of smpboot.c to generate the attached patch, so it cleanly applies; but
haven''t built/run/tested the xen-unstable version.

Signed-off-by: Donald Dutile <ddutile@redhat.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Good point, I''ll confirm that and take that approach instead, protected
with
some kind of BUG_ON(!spin_trylock) on a private lock for sanity''s sake.

 -- Keir

On 1/2/08 09:33, "Jan Beulich" <jbeulich@novell.com> wrote:
> I think this can be fixed with a much smaller change: Since the only caller
> of cpu_initialize_context() is __cpu_up(), and since __cpu_up() invocation
> is serialized, the variable in question could simply be static.
> 
> Jan
> 
>>>> Don Dutile <ddutile@redhat.com> 31.01.08 21:05
>>>
> 
> When secondary cpus are initialized during an i386 pv-guest restore
> (due to save/restore or live migration), and the guest has
> a load that generates a fair number of interrupts (e.g., parallel kernel
> make),
> a stack overflow can occur because cpu_initialize_context() has
> a 2800 byte structure it declares on its stack.  linux-i386 has 4K stacks,
by
> default.
> Using 2800 bytes out of 4K by a single function in a call list
isn''t nice;
> add the beginning of interrupt handling at just the right point, before the
> switch to the interrupt stack is made... and... the scale tips...
> 
> Simple fix: malloc & free structure as needed.
> 
> Would fail save/restore testing of an i386-guest running a parallel, kernel
> make after
> 50->100 save/restores;  with the fix, haven''t seen it fail
after 650
> save/restores.
> 
> Note: this is a basic port of this function in Jeremy
Fitzharinge''s Xen
> implementation of pv-ops
>        in upstream Linux, part of 15/32 patch, Xen SMP guest support.
> 
> Original patch done on rhel5; did a simple diff & merge to
xen-unstable''s
> version
> of smpboot.c to generate the attached patch, so it cleanly applies; but
> haven''t built/run/tested the xen-unstable version.
> 
> Signed-off-by: Donald Dutile <ddutile@redhat.com>
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan (& Keir)

That''s what I did the first time (used a simple static) I generated
a fix & tested it, because a lock is taken out (long) before __cpu_up() is
invoked,
to ensure only one cpu is in that code path anyhow.

*But* as I explained below, a modified version of cpu_initialize_context()
was already upstream and it went with a kzalloc-kfree implementation, so
maybe you want to ask Jeremy why he didn''t go with a simple static as
well.

So, the bottom line: the fix tries to follow upstream... typically
that''s
what we do (or change upstream, so all of us can follow it).

- Don

Jan Beulich wrote:
> I think this can be fixed with a much smaller change: Since the only caller
> of cpu_initialize_context() is __cpu_up(), and since __cpu_up() invocation
> is serialized, the variable in question could simply be static.
> 
> Jan
> 
>>>> Don Dutile <ddutile@redhat.com> 31.01.08 21:05
>>>
> 
> When secondary cpus are initialized during an i386 pv-guest restore
> (due to save/restore or live migration), and the guest has
> a load that generates a fair number of interrupts (e.g., parallel kernel
make),
> a stack overflow can occur because cpu_initialize_context() has
> a 2800 byte structure it declares on its stack.  linux-i386 has 4K stacks,
by default.
> Using 2800 bytes out of 4K by a single function in a call list
isn''t nice;
> add the beginning of interrupt handling at just the right point, before the
> switch to the interrupt stack is made... and... the scale tips...
> 
> Simple fix: malloc & free structure as needed.
> 
> Would fail save/restore testing of an i386-guest running a parallel, kernel
make after
> 50->100 save/restores;  with the fix, haven''t seen it fail
after 650 save/restores.
> 
> Note: this is a basic port of this function in Jeremy
Fitzharinge''s Xen implementation of pv-ops
>        in upstream Linux, part of 15/32 patch, Xen SMP guest support.
> 
> Original patch done on rhel5; did a simple diff & merge to
xen-unstable''s version
> of smpboot.c to generate the attached patch, so it cleanly applies; but
> haven''t built/run/tested the xen-unstable version.
> 
> Signed-off-by: Donald Dutile <ddutile@redhat.com>
> 
> 
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel