2009-11-04 06:48:38

by Xiaotian Feng

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

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 <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Venkatesh Pallipadi <[email protected]>
Cc: Suresh Siddha <[email protected]>
---
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

2009-11-04 10:52:12

by Ingo Molnar

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

* Xiaotian Feng <[email protected]> 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 <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: H. Peter Anvin <[email protected]>
> Cc: Venkatesh Pallipadi <[email protected]>
> Cc: Suresh Siddha <[email protected]>
> ---
> 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

2009-11-05 02:44:27

by Xiaotian Feng

Subject: [PATCH V2] 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 cleanup the repetitive error
return sequence.

Signed-off-by: Xiaotian Feng <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Venkatesh Pallipadi <[email protected]>
Cc: Suresh Siddha <[email protected]>
---
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

2009-11-06 00:24:41

by Suresh Siddha

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

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 <[email protected]>

Thanks Xiaotian.

2009-11-08 11:15:22

by Ingo Molnar

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

* Suresh Siddha <[email protected]> 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 <[email protected]>
>
> Thanks Xiaotian.

Thanks guys - queued it up for Linus.

Ingo

2009-11-08 11:19:09

by Xiaotian Feng

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

Commit-ID: 3b83171ef258627a91f60419f503b9b10e25f3fe
Gitweb: http://git.kernel.org/tip/3b83171ef258627a91f60419f503b9b10e25f3fe
Author: Xiaotian Feng <[email protected]>
AuthorDate: Thu, 5 Nov 2009 10:43:51 +0800
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Acked-by: Suresh Siddha <[email protected]>
Cc: Venkatesh Pallipadi <[email protected]>
Cc: H. Peter Anvin <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
}

/**

2009-11-08 11:51:57

by Xiaotian Feng

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

Commit-ID: de2a47cf2b3f59ef9664b277f4021b91af13598e
Gitweb: http://git.kernel.org/tip/de2a47cf2b3f59ef9664b277f4021b91af13598e
Author: Xiaotian Feng <[email protected]>
AuthorDate: Thu, 5 Nov 2009 10:43:51 +0800
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Acked-by: Suresh Siddha <[email protected]>
Cc: Venkatesh Pallipadi <[email protected]>
Cc: H. Peter Anvin <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
}

/**