[PATCH] siimage: fix kernel oops on PPC 44x

[PATCH] siimage: fix kernel oops on PPC 44x

[Sergei Shtylyov]

Fix kernel oops due to machine check occuring in init_chipset_siimage() on PPC
44x platforms.  These 32-bit CPUs have 36-bit physical address and PCI I/O and
memory spaces are mapped beyond 4 GB; arch/ppc/ code has a fixup in ioremap()
that creates an illusion of the PCI I/O and memory resources being mapped below
4 GB, while arch/powerpc/ code got rid of this fixup with PPC 44x having instead
CONFIG_RESOURCES_64BIT=y -- this causes the resources to be truncated to 32-bit
'unsigned long' type in this driver, and so non-existant memory being ioremap'ed
and then accessed...

Thanks to Valentine Barshak for providing an initial patch and explanations.

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

 drivers/ide/pci/siimage.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/drivers/ide/pci/siimage.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/siimage.c
+++ linux-2.6/drivers/ide/pci/siimage.c
@@ -492,7 +492,7 @@ static void proc_reports_siimage (struct
  
 static unsigned int setup_mmio_siimage (struct pci_dev *dev, const char *name)
 {
-	unsigned long bar5	= pci_resource_start(dev, 5);
+	resource_size_t bar5	= pci_resource_start(dev, 5);
 	unsigned long barsize	= pci_resource_len(dev, 5);
 	u8 tmpbyte	= 0;
 	void __iomem *ioaddr;

Re: [PATCH] siimage: fix kernel oops on PPC 44x

[Bartlomiej Zolnierkiewicz]

On Monday 07 April 2008, Sergei Shtylyov wrote:
> Fix kernel oops due to machine check occuring in init_chipset_siimage() on PPC
> 44x platforms.  These 32-bit CPUs have 36-bit physical address and PCI I/O and
> memory spaces are mapped beyond 4 GB; arch/ppc/ code has a fixup in ioremap()
> that creates an illusion of the PCI I/O and memory resources being mapped below
> 4 GB, while arch/powerpc/ code got rid of this fixup with PPC 44x having instead
> CONFIG_RESOURCES_64BIT=y -- this causes the resources to be truncated to 32-bit
> 'unsigned long' type in this driver, and so non-existant memory being ioremap'ed
> and then accessed...
> 
> Thanks to Valentine Barshak for providing an initial patch and explanations.
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

applied and pushed to Linus, thanks!

I guess that it would be worth to audit the rest of IDE code for
pci_resource_{start,end}() vs 'unsigned long' occurences and fix them.

[ Even if they work at the moment they are just bugs waiting to happened
  when we add support for some new platforms or rewrite the code... ]

Re: [PATCH] siimage: fix kernel oops on PPC 44x

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>Fix kernel oops due to machine check occuring in init_chipset_siimage() on PPC
>>44x platforms.  These 32-bit CPUs have 36-bit physical address and PCI I/O and
>>memory spaces are mapped beyond 4 GB; arch/ppc/ code has a fixup in ioremap()
>>that creates an illusion of the PCI I/O and memory resources being mapped below
>>4 GB, while arch/powerpc/ code got rid of this fixup with PPC 44x having instead
>>CONFIG_RESOURCES_64BIT=y -- this causes the resources to be truncated to 32-bit
>>'unsigned long' type in this driver, and so non-existant memory being ioremap'ed
>>and then accessed...

>>Thanks to Valentine Barshak for providing an initial patch and explanations.

>>Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

> applied and pushed to Linus, thanks!

> I guess that it would be worth to audit the rest of IDE code for

    Already done. Some drivers, like sgiioc4, scc_pata, and pmac are prone to
that at least in theory.  Although I doubt that they ever get used in such
environments as PPC 44x platform kernels, i.e. 32-bit kernel and PCI mapped 
beyond 4 GB.

> pci_resource_{start,end}() vs 'unsigned long' occurences and fix them.

    There are quite a lot of those overall but they only pose danger if the 
resource in question is in memory space since the I/O space always uses 
'unsigned long' addresses. So, IDE core and drivers using only I/O resources 
should not be prone to that kind of issue.

> [ Even if they work at the moment they are just bugs waiting to happened
>   when we add support for some new platforms or rewrite the code... ]

WBR, Sergei

Re: [PATCH] siimage: fix kernel oops on PPC 44x

[Bartlomiej Zolnierkiewicz]

On Tuesday 08 April 2008, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>Fix kernel oops due to machine check occuring in init_chipset_siimage() on PPC
> >>44x platforms.  These 32-bit CPUs have 36-bit physical address and PCI I/O and
> >>memory spaces are mapped beyond 4 GB; arch/ppc/ code has a fixup in ioremap()
> >>that creates an illusion of the PCI I/O and memory resources being mapped below
> >>4 GB, while arch/powerpc/ code got rid of this fixup with PPC 44x having instead
> >>CONFIG_RESOURCES_64BIT=y -- this causes the resources to be truncated to 32-bit
> >>'unsigned long' type in this driver, and so non-existant memory being ioremap'ed
> >>and then accessed...
> 
> >>Thanks to Valentine Barshak for providing an initial patch and explanations.
> 
> >>Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
> > applied and pushed to Linus, thanks!
> 
> > I guess that it would be worth to audit the rest of IDE code for
> 
>     Already done. Some drivers, like sgiioc4, scc_pata, and pmac are prone to
> that at least in theory.  Although I doubt that they ever get used in such
> environments as PPC 44x platform kernels, i.e. 32-bit kernel and PCI mapped 
> beyond 4 GB.
> 
> > pci_resource_{start,end}() vs 'unsigned long' occurences and fix them.
> 
>     There are quite a lot of those overall but they only pose danger if the 
> resource in question is in memory space since the I/O space always uses 
> 'unsigned long' addresses. So, IDE core and drivers using only I/O resources 
> should not be prone to that kind of issue.

Thanks for taking a look (good to hear that we are fine for now).

> > [ Even if they work at the moment they are just bugs waiting to happened
> >   when we add support for some new platforms or rewrite the code... ]

I still think that it is worth to switch to always using resource_size_t
with pci_resource{start,end}() - increase of the code size should be minimal
and negligable (also it would happen only for CONFIG_RESOURCES_64BIT=y)
but in the return we will keep the code consistent and hint people who're
writing new code (and are looking at the existing code as a base).

[ this is kernel-wide comment, w.r.t. to IDE - I'll try updating it when
  I have some time (unless of course somebody sends me a patch earlier :) ]

Thanks,
Bart