[PATCH] x86: add missing free_memtype if get_vm_area_caller failed

[PATCH] x86: add missing free_memtype if get_vm_area_caller failed

[Xiaotian Feng]

When __ioremap_caller goes into get_vm_area, kernel already reserved
memtype. so if get_vm_area fails, we need to free it before return.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
---
 arch/x86/mm/ioremap.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 334e63c..7859f77 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -196,8 +196,10 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 	 * Ok, go for it..
 	 */
 	area = get_vm_area_caller(size, VM_IOREMAP, caller);
-	if (!area)
+	if (!area) {
+		free_memtype(phys_addr, phys_addr + size);
 		return NULL;
+	}
 	area->phys_addr = phys_addr;
 	vaddr = (unsigned long) area->addr;
 
-- 
1.6.2.5

Re: [PATCH] x86: add missing free_memtype if get_vm_area_caller failed

[Ingo Molnar]

* Xiaotian Feng <dfeng@redhat.com> wrote:

> When __ioremap_caller goes into get_vm_area, kernel already reserved
> memtype. so if get_vm_area fails, we need to free it before return.
> 
> Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> Cc: Suresh Siddha <suresh.b.siddha@intel.com>
> ---
>  arch/x86/mm/ioremap.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 334e63c..7859f77 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -196,8 +196,10 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
>  	 * Ok, go for it..
>  	 */
>  	area = get_vm_area_caller(size, VM_IOREMAP, caller);
> -	if (!area)
> +	if (!area) {
> +		free_memtype(phys_addr, phys_addr + size);
>  		return NULL;
> +	}
>  	area->phys_addr = phys_addr;
>  	vaddr = (unsigned long) area->addr;

Nice one!

Mind structuring this fix in a bit different way please?

The main cause of this bug is the following unclean resource-teardown 
pattern in __ioremap_caller():

	area = get_vm_area_caller(size, VM_IOREMAP, caller);
	if (!area)
		return NULL;
	area->phys_addr = phys_addr;
	vaddr = (unsigned long) area->addr;

	if (kernel_map_sync_memtype(phys_addr, size, prot_val)) {
		free_memtype(phys_addr, phys_addr + size);
		free_vm_area(area);
		return NULL;
	}

	if (ioremap_page_range(vaddr, vaddr + size, phys_addr, prot)) {
		free_memtype(phys_addr, phys_addr + size);
		free_vm_area(area);
		return NULL;
	}

see how repetitive it is, and how the return sequence is duplicated?

The way we do this is to add an error path to the tail of the function:

err_free_area:
	free_vm_area(area);
err_free_memtype:
	free_memtype(phys_addr, phys_addr + size);
	return NULL;

and changing the failure branches to:

	if (!area)
		goto err_free_memtype;
	...

	if (ioremap_page_range(vaddr, vaddr + size, phys_addr, prot))
		goto err_free_area;

etc.

Ok?
	
	Ingo

[PATCH V2] x86: fix error return sequence in __ioremap_caller

[Xiaotian Feng]

kernel missed to free memtype if get_vm_area_caller failed in __ioremap_caller.
This patch introduces error path to fix this and cleanup the repetitive error
return sequence.  

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
---
 arch/x86/mm/ioremap.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 334e63c..7574575 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -170,8 +170,7 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 				(unsigned long long)phys_addr,
 				(unsigned long long)(phys_addr + size),
 				prot_val, new_prot_val);
-			free_memtype(phys_addr, phys_addr + size);
-			return NULL;
+			goto err_free_memtype;
 		}
 		prot_val = new_prot_val;
 	}
@@ -197,26 +196,25 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 	 */
 	area = get_vm_area_caller(size, VM_IOREMAP, caller);
 	if (!area)
-		return NULL;
+		goto err_free_memtype;
 	area->phys_addr = phys_addr;
 	vaddr = (unsigned long) area->addr;
 
-	if (kernel_map_sync_memtype(phys_addr, size, prot_val)) {
-		free_memtype(phys_addr, phys_addr + size);
-		free_vm_area(area);
-		return NULL;
-	}
+	if (kernel_map_sync_memtype(phys_addr, size, prot_val)) 
+		goto err_free_area;
 
-	if (ioremap_page_range(vaddr, vaddr + size, phys_addr, prot)) {
-		free_memtype(phys_addr, phys_addr + size);
-		free_vm_area(area);
-		return NULL;
-	}
+	if (ioremap_page_range(vaddr, vaddr + size, phys_addr, prot))
+		goto err_free_area;
 
 	ret_addr = (void __iomem *) (vaddr + offset);
 	mmiotrace_ioremap(unaligned_phys_addr, unaligned_size, ret_addr);
 
 	return ret_addr;
+err_free_area:
+	free_vm_area(area);
+err_free_memtype:
+	free_memtype(phys_addr, phys_addr + size);
+	return NULL;
 }
 
 /**
-- 
1.6.2.5

Re: [PATCH V2] x86: fix error return sequence in __ioremap_caller

[Suresh Siddha]

On Wed, 2009-11-04 at 18:43 -0800, Xiaotian Feng wrote:
> kernel missed to free memtype if get_vm_area_caller failed in __ioremap_caller.
> This patch introduces error path to fix this and cleanup the repetitive error
> return sequence.  

Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

Thanks Xiaotian.

Re: [PATCH V2] x86: fix error return sequence in __ioremap_caller

[Ingo Molnar]

* Suresh Siddha <suresh.b.siddha@intel.com> wrote:

> On Wed, 2009-11-04 at 18:43 -0800, Xiaotian Feng wrote:
> > kernel missed to free memtype if get_vm_area_caller failed in __ioremap_caller.
> > This patch introduces error path to fix this and cleanup the repetitive error
> > return sequence.  
> 
> Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
> 
> Thanks Xiaotian.

Thanks guys - queued it up for Linus.

	Ingo

[tip:x86/urgent] x86: Fix error return sequence in __ioremap_caller()

[tip-bot for Xiaotian Feng]

Commit-ID:  3b83171ef258627a91f60419f503b9b10e25f3fe
Gitweb:     http://git.kernel.org/tip/3b83171ef258627a91f60419f503b9b10e25f3fe
Author:     Xiaotian Feng <dfeng@redhat.com>
AuthorDate: Thu, 5 Nov 2009 10:43:51 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 8 Nov 2009 12:14:50 +0100

x86: Fix error return sequence in __ioremap_caller()

kernel missed to free memtype if get_vm_area_caller failed in
__ioremap_caller.

This patch introduces error path to fix this and cleans up the
repetitive error return sequences that contributed to the
creation of the bug.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
LKML-Reference: <1257389031-20429-1-git-send-email-dfeng@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/mm/ioremap.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 334e63c..2feb9bd 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -170,8 +170,7 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 				(unsigned long long)phys_addr,
 				(unsigned long long)(phys_addr + size),
 				prot_val, new_prot_val);
-			free_memtype(phys_addr, phys_addr + size);
-			return NULL;
+			goto err_free_memtype;
 		}
 		prot_val = new_prot_val;
 	}
@@ -197,26 +196,25 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 	 */
 	area = get_vm_area_caller(size, VM_IOREMAP, caller);
 	if (!area)
-		return NULL;
+		goto err_free_memtype;
 	area->phys_addr = phys_addr;
 	vaddr = (unsigned long) area->addr;
 
-	if (kernel_map_sync_memtype(phys_addr, size, prot_val)) {
-		free_memtype(phys_addr, phys_addr + size);
-		free_vm_area(area);
-		return NULL;
-	}
+	if (kernel_map_sync_memtype(phys_addr, size, prot_val))
+		goto err_free_area;
 
-	if (ioremap_page_range(vaddr, vaddr + size, phys_addr, prot)) {
-		free_memtype(phys_addr, phys_addr + size);
-		free_vm_area(area);
-		return NULL;
-	}
+	if (ioremap_page_range(vaddr, vaddr + size, phys_addr, prot))
+		goto err_free_area;
 
 	ret_addr = (void __iomem *) (vaddr + offset);
 	mmiotrace_ioremap(unaligned_phys_addr, unaligned_size, ret_addr);
 
 	return ret_addr;
+err_free_area:
+	free_vm_area(area);
+err_free_memtype:
+	free_memtype(phys_addr, phys_addr + size);
+	return NULL;
 }
 
 /**

[tip:x86/urgent] x86: Fix error return sequence in __ioremap_caller()

[tip-bot for Xiaotian Feng]

Commit-ID:  de2a47cf2b3f59ef9664b277f4021b91af13598e
Gitweb:     http://git.kernel.org/tip/de2a47cf2b3f59ef9664b277f4021b91af13598e
Author:     Xiaotian Feng <dfeng@redhat.com>
AuthorDate: Thu, 5 Nov 2009 10:43:51 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 8 Nov 2009 12:48:58 +0100

x86: Fix error return sequence in __ioremap_caller()

kernel missed to free memtype if get_vm_area_caller failed in
__ioremap_caller.

This patch introduces error path to fix this and cleans up the
repetitive error return sequences that contributed to the
creation of the bug.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
LKML-Reference: <1257389031-20429-1-git-send-email-dfeng@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/mm/ioremap.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 334e63c..2feb9bd 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -170,8 +170,7 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 				(unsigned long long)phys_addr,
 				(unsigned long long)(phys_addr + size),
 				prot_val, new_prot_val);
-			free_memtype(phys_addr, phys_addr + size);
-			return NULL;
+			goto err_free_memtype;
 		}
 		prot_val = new_prot_val;
 	}
@@ -197,26 +196,25 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 	 */
 	area = get_vm_area_caller(size, VM_IOREMAP, caller);
 	if (!area)
-		return NULL;
+		goto err_free_memtype;
 	area->phys_addr = phys_addr;
 	vaddr = (unsigned long) area->addr;
 
-	if (kernel_map_sync_memtype(phys_addr, size, prot_val)) {
-		free_memtype(phys_addr, phys_addr + size);
-		free_vm_area(area);
-		return NULL;
-	}
+	if (kernel_map_sync_memtype(phys_addr, size, prot_val))
+		goto err_free_area;
 
-	if (ioremap_page_range(vaddr, vaddr + size, phys_addr, prot)) {
-		free_memtype(phys_addr, phys_addr + size);
-		free_vm_area(area);
-		return NULL;
-	}
+	if (ioremap_page_range(vaddr, vaddr + size, phys_addr, prot))
+		goto err_free_area;
 
 	ret_addr = (void __iomem *) (vaddr + offset);
 	mmiotrace_ioremap(unaligned_phys_addr, unaligned_size, ret_addr);
 
 	return ret_addr;
+err_free_area:
+	free_vm_area(area);
+err_free_memtype:
+	free_memtype(phys_addr, phys_addr + size);
+	return NULL;
 }
 
 /**