[PATCH] kdump: don't call __ioremap() for pfn = 0

[PATCH] kdump: don't call __ioremap() for pfn = 0

[Sachin P. Sant]

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

Hi

While using dd command to retrive the dump from /dev/oldmem, there comes
a rare case where pfn value is zero. In this case the __ioremap() call 
returns
NULL and hence copying fails.

# dd if=/dev/oldmem of=/dev/null
dd: reading `/dev/oldmem': Bad address
0+0 records in
0+0 records out
0 bytes (0 B) copied, 0.000121 seconds, 0.0 kB/s

Attached is a patch to fix this problem. During such rare cases don't call
__ioremap() to do the address translation, instead use __va() .

Tested with 2.6.18.

Thanks
-Sachin

[-- Attachment #2: kdump-no-ioremap-for-pfn-zero-fix --]
[-- Type: text/plain, Size: 1027 bytes --]

* During some rare cases [like using dd command with /dev/oldmem] the pfn value
  can be zero. Do not call __ioremap for zero pfn value, instead use __va to 
  calculate the virtual address.

Signed-off-by: Sachin Sant <sachinp@in.ibm.com>

diff -Naurp a/arch/powerpc/kernel/crash_dump.c b/arch/powerpc/kernel/crash_dump.c
--- a/arch/powerpc/kernel/crash_dump.c	2006-09-20 09:12:06.000000000 +0530
+++ b/arch/powerpc/kernel/crash_dump.c	2006-09-21 09:06:35.000000000 +0530
@@ -101,7 +101,15 @@ ssize_t copy_oldmem_page(unsigned long p
 	if (!csize)
 		return 0;
 
-	vaddr = __ioremap(pfn << PAGE_SHIFT, PAGE_SIZE, 0);
+	/* During some rare cases [like using dd command with /dev/oldmem] 
+	 * the pfn value can be zero. Do not call __ioremap for zero
+	 * pfn value, instead use __va to calculate the virtual address.
+	 */
+
+	if (pfn == 0)
+		vaddr = __va(pfn << PAGE_SHIFT);
+	else
+		vaddr = __ioremap(pfn << PAGE_SHIFT, PAGE_SIZE, 0);
 
 	if (userbuf) {
 		if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {

Re: [PATCH] kdump: don't call __ioremap() for pfn = 0

[Michael Ellerman]

On Thu, 2006-09-21 at 10:07 +0530, Sachin P. Sant wrote:
> Hi
> 
> While using dd command to retrive the dump from /dev/oldmem, there comes
> a rare case where pfn value is zero. In this case the __ioremap() call 
> returns
> NULL and hence copying fails.
> 
> # dd if=/dev/oldmem of=/dev/null
> dd: reading `/dev/oldmem': Bad address
> 0+0 records in
> 0+0 records out
> 0 bytes (0 B) copied, 0.000121 seconds, 0.0 kB/s
> 
> Attached is a patch to fix this problem. During such rare cases don't call
> __ioremap() to do the address translation, instead use __va() .

It's not really rare, it's just when we're reading /dev/oldmem directly.

We can actually use the __va() trick for the whole linear mapping rather
than just pfn 0, which saves the ioremap. We also shouldn't really be
trying to iounmap(__va(0)).

So perhaps something more like this? Although it's a bit ugly because of
the need to conditionally call iounmap().

Paul you said we shouldn't be using ioremap(), but I really can't find
an alternative - map_vm_area() looks close but it requires struct pages
which we don't have. And I think your main objection was a cacheable
mapping, which we're not doing anyway by calling __ioremap().

...

Fix /dev/oldmem for kdump

A change to __ioremap() broke reading /dev/oldmem because we're no
longer able to ioremap pfn 0 (d177c207ba16b1db31283e2d1fee7ad4a863584b).

We actually don't need to ioremap for anything that's part of the linear
mapping, so just read it directly.

Also make sure we're only reading one page or less at a time.

Index: to-merge/arch/powerpc/kernel/crash_dump.c
===================================================================
--- to-merge.orig/arch/powerpc/kernel/crash_dump.c
+++ to-merge/arch/powerpc/kernel/crash_dump.c
@@ -80,6 +80,20 @@ static int __init parse_savemaxmem(char 
 }
 __setup("savemaxmem=", parse_savemaxmem);
 
+
+static size_t copy_oldmem_vaddr(void *vaddr, char *buf, size_t csize,
+				unsigned long offset, int userbuf)
+{
+	if (userbuf) {
+		if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {
+			return -EFAULT;
+		}
+	} else
+		memcpy(buf, (vaddr + offset), csize);
+
+	return csize;
+}
+
 /**
  * copy_oldmem_page - copy one page from "oldmem"
  * @pfn: page frame number to be copied
@@ -101,16 +115,16 @@ ssize_t copy_oldmem_page(unsigned long p
 	if (!csize)
 		return 0;
 
-	vaddr = __ioremap(pfn << PAGE_SHIFT, PAGE_SIZE, 0);
+	csize = min(csize, PAGE_SIZE);
 
-	if (userbuf) {
-		if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {
-			iounmap(vaddr);
-			return -EFAULT;
-		}
-	} else
-		memcpy(buf, (vaddr + offset), csize);
+	if (pfn < max_pfn) {
+		vaddr = __va(pfn << PAGE_SHIFT);
+		csize = copy_oldmem_vaddr(vaddr, buf, csize, offset, userbuf);
+	} else {
+		vaddr = __ioremap(pfn << PAGE_SHIFT, PAGE_SIZE, 0);
+		csize = copy_oldmem_vaddr(vaddr, buf, csize, offset, userbuf);
+		iounmap(vaddr);
+	}
 
-	iounmap(vaddr);
 	return csize;
 }

Re: [Fastboot] [PATCH] kdump: don't call __ioremap() for pfn = 0

[Vivek Goyal]

On Thu, Sep 21, 2006 at 09:02:37PM +1000, Michael Ellerman wrote:
> On Thu, 2006-09-21 at 10:07 +0530, Sachin P. Sant wrote:
> > Hi
> > 
> > While using dd command to retrive the dump from /dev/oldmem, there comes
> > a rare case where pfn value is zero. In this case the __ioremap() call 
> > returns
> > NULL and hence copying fails.
> > 
> > # dd if=/dev/oldmem of=/dev/null
> > dd: reading `/dev/oldmem': Bad address
> > 0+0 records in
> > 0+0 records out
> > 0 bytes (0 B) copied, 0.000121 seconds, 0.0 kB/s
> > 
> > Attached is a patch to fix this problem. During such rare cases don't call
> > __ioremap() to do the address translation, instead use __va() .
> 
> It's not really rare, it's just when we're reading /dev/oldmem directly.
> 
> We can actually use the __va() trick for the whole linear mapping rather
> than just pfn 0, which saves the ioremap. We also shouldn't really be
> trying to iounmap(__va(0)).
> 

Makes sense. We can take advantage of linear mappings mapped till max_pfn
and avoid ioremap().

> +
> +static size_t copy_oldmem_vaddr(void *vaddr, char *buf, size_t csize,
> +				unsigned long offset, int userbuf)
> +{
> +	if (userbuf) {
> +		if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {
> +			return -EFAULT;
> +		}

Probably you can get rid of above pair of braces as there is only single
statement under if.

>  
> -	if (userbuf) {
> -		if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {
> -			iounmap(vaddr);
> -			return -EFAULT;
> -		}
> -	} else
> -		memcpy(buf, (vaddr + offset), csize);
> +	if (pfn < max_pfn) {

Should this be (pfn <= max_pfn) ?

-Vivek

Re: [PATCH] kdump: don't call __ioremap() for pfn = 0

[Sachin P. Sant]

Michael Ellerman wrote:
>> Attached is a patch to fix this problem. During such rare cases don't call
>> __ioremap() to do the address translation, instead use __va() .
>>     
>
> It's not really rare, it's just when we're reading /dev/oldmem directly.
>   
That's true. Since we don't try to copy raw dump from /dev/oldmem very
often, we haven't come across this problem. Hence rare .. but as
michael said always recreatable while using dd command with /dev/oldmem.

> We can actually use the __va() trick for the whole linear mapping rather
> than just pfn 0, which saves the ioremap. We also shouldn't really be
> trying to iounmap(__va(0)).
>   
Yes. Makes sense. Agreed.

> So perhaps something more like this? Although it's a bit ugly because of
> the need to conditionally call iounmap().
>   
< snip > 

Thanks
-Sachin

Re: [Fastboot] [PATCH] kdump: don't call __ioremap() for pfn = 0

[Michael Ellerman]

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

On Thu, 2006-09-21 at 10:10 -0400, Vivek Goyal wrote:
> On Thu, Sep 21, 2006 at 09:02:37PM +1000, Michael Ellerman wrote:
> > +
> > +static size_t copy_oldmem_vaddr(void *vaddr, char *buf, size_t csize,
> > +				unsigned long offset, int userbuf)
> > +{
> > +	if (userbuf) {
> > +		if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {
> > +			return -EFAULT;
> > +		}
> 
> Probably you can get rid of above pair of braces as there is only single
> statement under if.

Yep.

> >  
> > -	if (userbuf) {
> > -		if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {
> > -			iounmap(vaddr);
> > -			return -EFAULT;
> > -		}
> > -	} else
> > -		memcpy(buf, (vaddr + offset), csize);
> > +	if (pfn < max_pfn) {
> 
> Should this be (pfn <= max_pfn) ?

No, max_pfn is badly named. It seems to actually be the total number of
pages == the first pfn past the end of the linear mapping.

But it'd be cleaner to use page_is_ram(), I'll do a new patch.

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: 191 bytes --]