[PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[Linas Vepstas]

Paul,

I presume this looks reasonable, please apply.

--linas


There are a variety of code paths that lead to rtas_stop_self()
being called, primarily through cpu_die(). However, rtas_stop_self() 
has a BUG_ON(rtas_args->token == RTAS_UNKNOWN_SERVICE); in it, and
this rtas token is only set up if CONFIG_HOTPLUG_CPU is defined.

Rather than wrapping all of the callers of rtas_stop_self()
with CONFIG_HOTPLUG_CPU, it seems wiser to just unwrap the token
definition.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

----
 arch/powerpc/kernel/rtas.c |    2 --
 1 file changed, 2 deletions(-)

Index: linux-2.6.19-rc4-git3/arch/powerpc/kernel/rtas.c
===================================================================
--- linux-2.6.19-rc4-git3.orig/arch/powerpc/kernel/rtas.c	2006-11-10 12:13:17.000000000 -0600
+++ linux-2.6.19-rc4-git3/arch/powerpc/kernel/rtas.c	2006-11-10 12:13:40.000000000 -0600
@@ -879,9 +879,7 @@ void __init rtas_initialize(void)
 #endif
 	rtas_rmo_buf = lmb_alloc_base(RTAS_RMOBUF_MAX, PAGE_SIZE, rtas_region);
 
-#ifdef CONFIG_HOTPLUG_CPU
 	rtas_stop_self_args.token = rtas_token("stop-self");
-#endif
 #ifdef CONFIG_RTAS_ERROR_LOGGING
 	rtas_last_error_token = rtas_token("rtas-last-error");
 #endif

Re: [PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[Nathan Lynch]

Linas Vepstas wrote:
> 
> There are a variety of code paths that lead to rtas_stop_self()
> being called, primarily through cpu_die(). However, rtas_stop_self() 
> has a BUG_ON(rtas_args->token == RTAS_UNKNOWN_SERVICE); in it, and
> this rtas token is only set up if CONFIG_HOTPLUG_CPU is defined.
> 
> Rather than wrapping all of the callers of rtas_stop_self()
> with CONFIG_HOTPLUG_CPU, it seems wiser to just unwrap the token
> definition.

Is there actually a code path that calls rtas_stop_self with
CONFIG_HOTPLUG_CPU=n?  That would be a bug, I think.

Re: [PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[Paul Mackerras]

Linas Vepstas writes:

> I presume this looks reasonable, please apply.

OK, is this a fix for a bug that needs to be fixed before 2.6.19 is
release?

Thanks,
Paul.

Re: [PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[Linas Vepstas]

On Sat, Nov 11, 2006 at 10:55:09AM +1100, Paul Mackerras wrote:
> Linas Vepstas writes:
> 
> > I presume this looks reasonable, please apply.
> 
> OK, is this a fix for a bug that needs to be fixed before 2.6.19 is
> release?

Probably not. This was uncovered during "unusual" testing, with user
catting values int sysfs files.  Since you need to be root to do this,
I don't think its a security flaw.

--linas

Re: [PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[Linas Vepstas]

On Fri, Nov 10, 2006 at 02:01:52PM -0600, Nathan Lynch wrote:
> Linas Vepstas wrote:
> > 
> > There are a variety of code paths that lead to rtas_stop_self()
> > being called, primarily through cpu_die(). However, rtas_stop_self() 
> > has a BUG_ON(rtas_args->token == RTAS_UNKNOWN_SERVICE); in it, and
> > this rtas token is only set up if CONFIG_HOTPLUG_CPU is defined.
> > 
> > Rather than wrapping all of the callers of rtas_stop_self()
> > with CONFIG_HOTPLUG_CPU, it seems wiser to just unwrap the token
> > definition.
> 
> Is there actually a code path that calls rtas_stop_self with
> CONFIG_HOTPLUG_CPU=n?  That would be a bug, I think.

Yeah, that was my first reaction too, right next to "user error". 
But then I started tracing the code paths, and after they multiplied
a bit, it was clear that the answer wouldn't be clear. 

So then I philosophized a bit: if the code cannot be called
unless CONFIG_HOTPLUG_CPU=y, then I should add #ifdef CONFIG_HOTPLUG_CPU
to make sure that the code doesn't even get compiled in.

Well, there used to be such ifdefs (according to Joel) but someone 
removed the ifdefs. I'm guessing that these got removed during the
powerpc tree migration.

So... Rather than wrapping all of the callers of rtas_stop_self() ...

--linas

Re: [PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]

On Fri, 2006-11-10 at 19:01 -0600, Linas Vepstas wrote:
> On Fri, Nov 10, 2006 at 02:01:52PM -0600, Nathan Lynch wrote:
> > Linas Vepstas wrote:
> > > 
> > > There are a variety of code paths that lead to rtas_stop_self()
> > > being called, primarily through cpu_die(). However, rtas_stop_self() 
> > > has a BUG_ON(rtas_args->token == RTAS_UNKNOWN_SERVICE); in it, and
> > > this rtas token is only set up if CONFIG_HOTPLUG_CPU is defined.
> > > 
> > > Rather than wrapping all of the callers of rtas_stop_self()
> > > with CONFIG_HOTPLUG_CPU, it seems wiser to just unwrap the token
> > > definition.
> > 
> > Is there actually a code path that calls rtas_stop_self with
> > CONFIG_HOTPLUG_CPU=n?  That would be a bug, I think.
> 
> Yeah, that was my first reaction too, right next to "user error". 
> But then I started tracing the code paths, and after they multiplied
> a bit, it was clear that the answer wouldn't be clear. 
> 
> So then I philosophized a bit: if the code cannot be called
> unless CONFIG_HOTPLUG_CPU=y, then I should add #ifdef CONFIG_HOTPLUG_CPU
> to make sure that the code doesn't even get compiled in.
> 
> Well, there used to be such ifdefs (according to Joel) but someone 
> removed the ifdefs. I'm guessing that these got removed during the
> powerpc tree migration.
> 
> So... Rather than wrapping all of the callers of rtas_stop_self() ...

I can only find one caller? Is it called via a #define or something?

Wouldn't it be preferable to just change rtas.h like so:

#ifdef CONFIG_HOTPLUG_CPU
extern void rtas_stop_self(void);
#else
static inline void rtas_stop_self(void) { }
#endif

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[jschopp]

> I can only find one caller? Is it called via a #define or something?
> 
> Wouldn't it be preferable to just change rtas.h like so:
> 
> #ifdef CONFIG_HOTPLUG_CPU
> extern void rtas_stop_self(void);
> #else
> static inline void rtas_stop_self(void) { }
> #endif
> 

There seem to be a couple problems here.  First I'm guessing the file you said there was 
catting into was the online attribute, which shouldn't appear, or at least shouldn't do 
anything, if cpu hotplug isn't configured.  It's a stupid thing to do, but it shouldn't 
crash the kernel.  I guess one of us should fix that, though it probably doesn't merit 
2.6.19 consideration.

The second problem is rtas_stop_self shouldn't be defined or called either.  Michael's 
solution above seems best for that to me.  Care to give us a patch Michael?

Re: [PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[Linas Vepstas]

On Mon, Nov 13, 2006 at 12:21:04AM -0600, jschopp wrote:
> >I can only find one caller? Is it called via a #define or something?

Right. I was looking at pSeries_mach_cpu_die(), which is called
as md.cpu_die from many places.

> >Wouldn't it be preferable to just change rtas.h like so:
> >
> >#ifdef CONFIG_HOTPLUG_CPU
> >extern void rtas_stop_self(void);
> >#else
> >static inline void rtas_stop_self(void) { }
> >#endif

Well, the problem is that this would just pass the BUG() to 
the next level, in pSeries_mach_cpu_die():

static void pSeries_mach_cpu_die(void)
{
   local_irq_disable();
   idle_task_exit();
   xics_teardown_cpu(0);
   rtas_stop_self();
   /* Should never get here... */
   BUG();
   for(;;);
}

so does that mean that CONFIG_HOTPLUG_CPU should also be used to 
stop the complation of pSeries_mach_cpu_die() ? 

--linas

Re: [PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[Nathan Lynch]

Linas Vepstas wrote:
> On Sat, Nov 11, 2006 at 10:55:09AM +1100, Paul Mackerras wrote:
> > Linas Vepstas writes:
> > 
> > > I presume this looks reasonable, please apply.
> > 
> > OK, is this a fix for a bug that needs to be fixed before 2.6.19 is
> > release?
> 
> Probably not. This was uncovered during "unusual" testing, with user
> catting values int sysfs files.  Since you need to be root to do this,
> I don't think its a security flaw.

I'm still not clear on what problem this patch is intended to address.

If the 'online' cpu attribute in sysfs can be used to attempt an
offline operation on a kernel with CONFIG_HOTPLUG_CPU=n, that's what
needs to be fixed.

Re: [PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[Linas Vepstas]

On Mon, Nov 13, 2006 at 12:57:04PM -0600, Nathan Lynch wrote:
> 
> I'm still not clear on what problem this patch is intended to address.

There's a code path that is compiled unconditonally, that expects 
this token to be defined. 

> If the 'online' cpu attribute in sysfs can be used to attempt an
> offline operation on a kernel with CONFIG_HOTPLUG_CPU=n, that's what
> needs to be fixed.

There are many places where cpu_die() might be called, and
I am sufficiently lazy to not want to audit them to make sure
that none of these code paths are taken if CONFIG_HOTPLUG_CPU=n.

Besides, if I audit all of these code paths today, and find
that none of them are ever called when  CONFIG_HOTPLUG_CPU=n,
I still have no assurance that someone might not unwittingly
change one of the code paths in the future. 

Code that might fail because someone makes an unwitting change
in the future is "brittle". The goal of my patch was to remove
the brittleness.

Besides, I notice that platforms/powermac/smp.c defines
cpu_die() as well, which tells me that there are code paths
that lead to its being called, even if CONFIG_HOTPLUG_CPU=n.
That would seem to conclude the audit, right?

--linas

Re: [PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

On Mon, 2006-11-13 at 12:05 -0600, Linas Vepstas wrote:
> On Mon, Nov 13, 2006 at 12:21:04AM -0600, jschopp wrote:
> > >I can only find one caller? Is it called via a #define or something?
> 
> Right. I was looking at pSeries_mach_cpu_die(), which is called
> as md.cpu_die from many places.
> 
> > >Wouldn't it be preferable to just change rtas.h like so:
> > >
> > >#ifdef CONFIG_HOTPLUG_CPU
> > >extern void rtas_stop_self(void);
> > >#else
> > >static inline void rtas_stop_self(void) { }
> > >#endif
> 
> Well, the problem is that this would just pass the BUG() to 
> the next level, in pSeries_mach_cpu_die():
> 
> static void pSeries_mach_cpu_die(void)
> {
>    local_irq_disable();
>    idle_task_exit();
>    xics_teardown_cpu(0);
>    rtas_stop_self();
>    /* Should never get here... */
>    BUG();
>    for(;;);
> }
> 
> so does that mean that CONFIG_HOTPLUG_CPU should also be used to 
> stop the complation of pSeries_mach_cpu_die() ? 

Yes. And its connection to ppc_md.cpu_die.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH]: PowerPC: make sure the rtas stop-self token is defined.

[Linas Vepstas]

On Tue, Nov 14, 2006 at 12:17:44PM +1100, Michael Ellerman wrote:
> On Mon, 2006-11-13 at 12:05 -0600, Linas Vepstas wrote:
> > 
> > Well, the problem is that this would just pass the BUG() to 
> > the next level, in pSeries_mach_cpu_die():
> > 
> > static void pSeries_mach_cpu_die(void)
> > {
> >    local_irq_disable();
> >    idle_task_exit();
> >    xics_teardown_cpu(0);
> >    rtas_stop_self();
> >    /* Should never get here... */
> >    BUG();
> >    for(;;);
> > }
> > 
> > so does that mean that CONFIG_HOTPLUG_CPU should also be used to 
> > stop the complation of pSeries_mach_cpu_die() ? 
> 
> Yes. And its connection to ppc_md.cpu_die.

OK, will send patch for this shortly. I reviewed the other cases,
and see now that this would e the consistent thing to do.

--linas