Re: 2.6.14-rc3-rt2

Re: 2.6.14-rc3-rt2

[Andi Kleen]

On Thursday 06 October 2005 11:48, Steven Rostedt wrote:
> On Thu, 6 Oct 2005, Ingo Molnar wrote:
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > > > ahh ... I would not be surprised if this caused actual problems on
> > > > x64 in the upstream kernel too: using save_flags() over u32 will
> > > > corrupt a word on the stack ...
> > >
> > > Actually, it's still safe upstream.  The locks are taken via a function
> > > defined as:
> > >
> > > unsigned long acpi_os_acquire_lock(acpi_handle handle)
> > > {
> > > 	unsigned long flags;
> > > 	spin_lock_irqsave((spinlock_t *) handle, flags);
> > > 	return flags;
> > > }
> > >
> > > So a u32 flags with
> > >
> > >   flags = acpi_os_acquire_lock(lock);
> > >
> > > would be safe, unless a 64 bit machine stored the value of IR in the
> > > upper word, which I don't know of any archs that do that.
> >
> > ok. But this still looks very volatile. Nowhere do we guarantee (or can
> > we guarantee) that silently zeroing out the upper 32 bits of flags is
> > safe!
>
> Andi,
>
> So, should I send my patch upstream?

It's a theoretical only issue for mainline right now. The only architectures 
using the ACPI code are i386,x86-64,ia64. The first two are ok with 
truncating. The IA64 PSR is longer than 32bit, but unless I'm misreading the 
code they only care about the "i" bit which is also in the lower 32bit (Tony 
can probably confirm/deny) 

Still might be good to clean up, but certainly not a urgent issue.

-Andi

[PATCH] cleanup u32 flags in acpi spin_lock calls.

[Steven Rostedt]

This is not an urgent issue, but was noticed in Ingo's RT kernel. There
are some places in ACPI that save flags as a u32 instead of a unsigned
long.  This is done indirectly by calling acpi_os_acquire_lock, which uses
unsigned long, but the flags returned are saved in the acpi code as a u32.

Since todays archs that use acpi, only care about the LS 32 bits of the
word, this is not really an issue.  But if there is an arch in the future
that changes that assumption, or (as RT does) some internal change in the
kernel that looks at the MSB of flags on a restore, this will be broken
for 64 bit machines.

This patch is just to make the ACPI code "clean".  That is, to use the
proper type for flags.

-- Steve

Note: You may notice that this patch has an -rt in the names, it does
      apply cleanly to 2.6.14-rc3.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


--- linux-2.6.14-rc3-rt9/drivers/acpi/events/evgpe.c.orig	2005-10-06 04:15:40.000000000 -0400
+++ linux-2.6.14-rc3-rt9/drivers/acpi/events/evgpe.c	2005-10-06 04:15:46.000000000 -0400
@@ -377,7 +377,7 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_x
 	struct acpi_gpe_register_info *gpe_register_info;
 	u32 status_reg;
 	u32 enable_reg;
-	u32 flags;
+	unsigned long flags;
 	acpi_status status;
 	struct acpi_gpe_block_info *gpe_block;
 	acpi_native_uint i;
--- linux-2.6.14-rc3-rt9/drivers/acpi/events/evgpeblk.c.orig	2005-10-06 04:00:34.000000000 -0400
+++ linux-2.6.14-rc3-rt9/drivers/acpi/events/evgpeblk.c	2005-10-06 04:00:58.000000000 -0400
@@ -136,7 +136,7 @@ acpi_status acpi_ev_walk_gpe_list(ACPI_G
 	struct acpi_gpe_block_info *gpe_block;
 	struct acpi_gpe_xrupt_info *gpe_xrupt_info;
 	acpi_status status = AE_OK;
-	u32 flags;
+	unsigned long flags;

 	ACPI_FUNCTION_TRACE("ev_walk_gpe_list");

@@ -479,7 +479,7 @@ static struct acpi_gpe_xrupt_info *acpi_
 	struct acpi_gpe_xrupt_info *next_gpe_xrupt;
 	struct acpi_gpe_xrupt_info *gpe_xrupt;
 	acpi_status status;
-	u32 flags;
+	unsigned long flags;

 	ACPI_FUNCTION_TRACE("ev_get_gpe_xrupt_block");

@@ -553,7 +553,7 @@ static acpi_status
 acpi_ev_delete_gpe_xrupt(struct acpi_gpe_xrupt_info *gpe_xrupt)
 {
 	acpi_status status;
-	u32 flags;
+	unsigned long flags;

 	ACPI_FUNCTION_TRACE("ev_delete_gpe_xrupt");

@@ -610,7 +610,7 @@ acpi_ev_install_gpe_block(struct acpi_gp
 	struct acpi_gpe_block_info *next_gpe_block;
 	struct acpi_gpe_xrupt_info *gpe_xrupt_block;
 	acpi_status status;
-	u32 flags;
+	unsigned long flags;

 	ACPI_FUNCTION_TRACE("ev_install_gpe_block");

@@ -663,7 +663,7 @@ acpi_ev_install_gpe_block(struct acpi_gp
 acpi_status acpi_ev_delete_gpe_block(struct acpi_gpe_block_info *gpe_block)
 {
 	acpi_status status;
-	u32 flags;
+	unsigned long flags;

 	ACPI_FUNCTION_TRACE("ev_install_gpe_block");

--- linux-2.6.14-rc3-rt9/drivers/acpi/events/evxface.c.orig	2005-10-06 04:16:27.000000000 -0400
+++ linux-2.6.14-rc3-rt9/drivers/acpi/events/evxface.c	2005-10-06 04:16:43.000000000 -0400
@@ -562,7 +562,7 @@ acpi_install_gpe_handler(acpi_handle gpe
 	struct acpi_gpe_event_info *gpe_event_info;
 	struct acpi_handler_info *handler;
 	acpi_status status;
-	u32 flags;
+	unsigned long flags;

 	ACPI_FUNCTION_TRACE("acpi_install_gpe_handler");

@@ -653,7 +653,7 @@ acpi_remove_gpe_handler(acpi_handle gpe_
 	struct acpi_gpe_event_info *gpe_event_info;
 	struct acpi_handler_info *handler;
 	acpi_status status;
-	u32 flags;
+	unsigned long flags;

 	ACPI_FUNCTION_TRACE("acpi_remove_gpe_handler");

Re: 2.6.14-rc3-rt2

[Luck, Tony]

> It's a theoretical only issue for mainline right now. The only architectures 
> using the ACPI code are i386,x86-64,ia64. The first two are ok with 
> truncating. The IA64 PSR is longer than 32bit, but unless I'm misreading the 
> code they only care about the "i" bit which is also in the lower 32bit (Tony 
> can probably confirm/deny) 

Andi is right ... if you follow the "acpi_os_release_lock" trail, you
eventually get to include/asm-ia64/system.h with the following definition
for __local_irq_restore:

#define __local_irq_restore(x)  ia64_intrin_local_irq_restore((x) & IA64_PSR_I)

and the IA64_PSR_I bit is bit 14 ... safely in the low 32 bits.  So this is
a correct fix, but will have no effect on ia64.

-Tony