[PATCH] x86: add missing free_memtype if get_vm_area

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH] x86: add missing free_memtype if get_vm_area_caller failed
@ 2009-11-04  6:48 Xiaotian Feng
  2009-11-04 10:51 ` Ingo Molnar
  0 siblings, 1 reply; 7+ messages in thread
From: Xiaotian Feng @ 2009-11-04  6:48 UTC (permalink / raw)
  To: tglx, mingo, hpa, x86, venkatesh.pallipadi, suresh.b.siddha
  Cc: linux-kernel, 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] x86: add missing free_memtype if get_vm_area_caller failed
  2009-11-04  6:48 [PATCH] x86: add missing free_memtype if get_vm_area_caller failed Xiaotian Feng
@ 2009-11-04 10:51 ` Ingo Molnar
  2009-11-05  2:43   ` [PATCH V2] x86: fix error return sequence in __ioremap_caller Xiaotian Feng
  0 siblings, 1 reply; 7+ messages in thread
From: Ingo Molnar @ 2009-11-04 10:51 UTC (permalink / raw)
  To: Xiaotian Feng
  Cc: tglx, mingo, hpa, x86, venkatesh.pallipadi, suresh.b.siddha,
	linux-kernel


* 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH V2] x86: fix error return sequence in __ioremap_caller
  2009-11-04 10:51 ` Ingo Molnar
@ 2009-11-05  2:43   ` Xiaotian Feng
  2009-11-06  0:23     ` Suresh Siddha
                       ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Xiaotian Feng @ 2009-11-05  2:43 UTC (permalink / raw)
  To: tglx, mingo, hpa, x86, venkatesh.pallipadi, suresh.b.siddha
  Cc: linux-kernel, 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH V2] x86: fix error return sequence in __ioremap_caller
  2009-11-05  2:43   ` [PATCH V2] x86: fix error return sequence in __ioremap_caller Xiaotian Feng
@ 2009-11-06  0:23     ` Suresh Siddha
  2009-11-08 11:15       ` Ingo Molnar
  2009-11-08 11:18     ` [tip:x86/urgent] x86: Fix error return sequence in __ioremap_caller() tip-bot for Xiaotian Feng
  2009-11-08 11:51     ` tip-bot for Xiaotian Feng
  2 siblings, 1 reply; 7+ messages in thread
From: Suresh Siddha @ 2009-11-06  0:23 UTC (permalink / raw)
  To: Xiaotian Feng
  Cc: tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com,
	x86@kernel.org, Pallipadi, Venkatesh,
	linux-kernel@vger.kernel.org

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2] x86: fix error return sequence in __ioremap_caller
  2009-11-06  0:23     ` Suresh Siddha
@ 2009-11-08 11:15       ` Ingo Molnar
  0 siblings, 0 replies; 7+ messages in thread
From: Ingo Molnar @ 2009-11-08 11:15 UTC (permalink / raw)
  To: Suresh Siddha
  Cc: Xiaotian Feng, tglx@linutronix.de, mingo@redhat.com,
	hpa@zytor.com, x86@kernel.org, Pallipadi, Venkatesh,
	linux-kernel@vger.kernel.org


* 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [tip:x86/urgent] x86: Fix error return sequence in __ioremap_caller()
  2009-11-05  2:43   ` [PATCH V2] x86: fix error return sequence in __ioremap_caller Xiaotian Feng
  2009-11-06  0:23     ` Suresh Siddha
@ 2009-11-08 11:18     ` tip-bot for Xiaotian Feng
  2009-11-08 11:51     ` tip-bot for Xiaotian Feng
  2 siblings, 0 replies; 7+ messages in thread
From: tip-bot for Xiaotian Feng @ 2009-11-08 11:18 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, hpa, mingo, venkatesh.pallipadi, suresh.b.siddha,
	tglx, dfeng, mingo

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;
 }
 
 /**

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [tip:x86/urgent] x86: Fix error return sequence in __ioremap_caller()
  2009-11-05  2:43   ` [PATCH V2] x86: fix error return sequence in __ioremap_caller Xiaotian Feng
  2009-11-06  0:23     ` Suresh Siddha
  2009-11-08 11:18     ` [tip:x86/urgent] x86: Fix error return sequence in __ioremap_caller() tip-bot for Xiaotian Feng
@ 2009-11-08 11:51     ` tip-bot for Xiaotian Feng
  2 siblings, 0 replies; 7+ messages in thread
From: tip-bot for Xiaotian Feng @ 2009-11-08 11:51 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, hpa, mingo, venkatesh.pallipadi, suresh.b.siddha,
	tglx, dfeng, mingo

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;
 }
 
 /**

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2009-11-08 11:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-11-04  6:48 [PATCH] x86: add missing free_memtype if get_vm_area_caller failed Xiaotian Feng
2009-11-04 10:51 ` Ingo Molnar
2009-11-05  2:43   ` [PATCH V2] x86: fix error return sequence in __ioremap_caller Xiaotian Feng
2009-11-06  0:23     ` Suresh Siddha
2009-11-08 11:15       ` Ingo Molnar
2009-11-08 11:18     ` [tip:x86/urgent] x86: Fix error return sequence in __ioremap_caller() tip-bot for Xiaotian Feng
2009-11-08 11:51     ` tip-bot for Xiaotian Feng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox