* [PATCH 0/2] mm/memblock.c: fix potential bug and code refine
@ 2016-12-11 12:59 Wei Yang
2016-12-11 12:59 ` [PATCH 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() Wei Yang
2016-12-11 12:59 ` [PATCH 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() Wei Yang
0 siblings, 2 replies; 6+ messages in thread
From: Wei Yang @ 2016-12-11 12:59 UTC (permalink / raw)
To: trivial, akpm; +Cc: linux-mm, linux-kernel, Wei Yang
Here are two patch of mm/memblock.c.
[1]. A trivial code refine in memblock_is_region_memory(), which removes an
unnecessary check on base address.
[2]. The original code forgets to check the return value of
memblock_reserve(), which may lead to potential problem. The patch fix this.
Wei Yang (2):
mm/memblock.c: trivial code refine in memblock_is_region_memory()
mm/memblock.c: check return value of memblock_reserve() in
memblock_virt_alloc_internal()
include/linux/memblock.h | 5 ++---
mm/memblock.c | 8 +++-----
2 files changed, 5 insertions(+), 8 deletions(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory()
2016-12-11 12:59 [PATCH 0/2] mm/memblock.c: fix potential bug and code refine Wei Yang
@ 2016-12-11 12:59 ` Wei Yang
2016-12-11 12:59 ` [PATCH 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() Wei Yang
1 sibling, 0 replies; 6+ messages in thread
From: Wei Yang @ 2016-12-11 12:59 UTC (permalink / raw)
To: trivial, akpm; +Cc: linux-mm, linux-kernel, Wei Yang
The base address is already guaranteed to be in the region by
memblock_search().
This patch removes the check on base, also a little refine in a macro.
Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
include/linux/memblock.h | 5 ++---
mm/memblock.c | 2 +-
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 3106ac1..e611819 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -408,9 +408,8 @@ static inline unsigned long memblock_region_reserved_end_pfn(const struct memblo
region++)
#define for_each_memblock_type(memblock_type, rgn) \
- idx = 0; \
- rgn = &memblock_type->regions[idx]; \
- for (idx = 0; idx < memblock_type->cnt; \
+ for (idx = 0, rgn = &memblock_type->regions[idx]; \
+ idx < memblock_type->cnt; \
idx++,rgn = &memblock_type->regions[idx])
#ifdef CONFIG_MEMTEST
diff --git a/mm/memblock.c b/mm/memblock.c
index ac12489..9d402d05 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1557,7 +1557,7 @@ int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size
if (idx == -1)
return 0;
- return memblock.memory.regions[idx].base <= base &&
+ return /* memblock.memory.regions[idx].base <= base && */
(memblock.memory.regions[idx].base +
memblock.memory.regions[idx].size) >= end;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal()
2016-12-11 12:59 [PATCH 0/2] mm/memblock.c: fix potential bug and code refine Wei Yang
2016-12-11 12:59 ` [PATCH 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() Wei Yang
@ 2016-12-11 12:59 ` Wei Yang
1 sibling, 0 replies; 6+ messages in thread
From: Wei Yang @ 2016-12-11 12:59 UTC (permalink / raw)
To: trivial, akpm; +Cc: linux-mm, linux-kernel, Wei Yang
memblock_reserve() may fail in case there is not enough regions.
This patch checks the return value.
Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
mm/memblock.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/mm/memblock.c b/mm/memblock.c
index 9d402d05..83ad703 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1268,18 +1268,17 @@ static void * __init memblock_virt_alloc_internal(
if (max_addr > memblock.current_limit)
max_addr = memblock.current_limit;
-
again:
alloc = memblock_find_in_range_node(size, align, min_addr, max_addr,
nid, flags);
- if (alloc)
+ if (alloc && !memblock_reserve(alloc, size))
goto done;
if (nid != NUMA_NO_NODE) {
alloc = memblock_find_in_range_node(size, align, min_addr,
max_addr, NUMA_NO_NODE,
flags);
- if (alloc)
+ if (alloc && !memblock_reserve(alloc, size))
goto done;
}
@@ -1297,7 +1296,6 @@ again:
return NULL;
done:
- memblock_reserve(alloc, size);
ptr = phys_to_virt(alloc);
memset(ptr, 0, size);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal()
2016-12-22 9:15 ` Michal Hocko
@ 2016-12-22 22:37 ` Wei Yang
0 siblings, 0 replies; 6+ messages in thread
From: Wei Yang @ 2016-12-22 22:37 UTC (permalink / raw)
To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel
On Thu, Dec 22, 2016 at 10:15:20AM +0100, Michal Hocko wrote:
>On Wed 21-12-16 23:30:33, Wei Yang wrote:
>> memblock_reserve() would add a new range to memblock.reserved in case the
>> new range is not totally covered by any of the current memblock.reserved
>> range. If the memblock.reserved is full and can't resize,
>> memblock_reserve() would fail.
>>
>> This doesn't happen in real world now, I observed this during code review.
>> While theoretically, it has the chance to happen. And if it happens, others
>> would think this range of memory is still available and may corrupt the
>> memory.
>
>OK, this explains it much better than the previous version! The silent
>memory corruption is indeed too hard to debug to have this open even
>when the issue is theoretical.
>
Thanks~ Have a nice day:-)
>> This patch checks the return value and goto "done" after it succeeds.
>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>
>Acked-by: Michal Hocko <mhocko@suse.com>
>
>Thanks!
>
>> ---
>> mm/memblock.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/memblock.c b/mm/memblock.c
>> index 4929e06..d0f2c96 100644
>> --- a/mm/memblock.c
>> +++ b/mm/memblock.c
>> @@ -1274,18 +1274,17 @@ static void * __init memblock_virt_alloc_internal(
>>
>> if (max_addr > memblock.current_limit)
>> max_addr = memblock.current_limit;
>> -
>> again:
>> alloc = memblock_find_in_range_node(size, align, min_addr, max_addr,
>> nid, flags);
>> - if (alloc)
>> + if (alloc && !memblock_reserve(alloc, size))
>> goto done;
>>
>> if (nid != NUMA_NO_NODE) {
>> alloc = memblock_find_in_range_node(size, align, min_addr,
>> max_addr, NUMA_NO_NODE,
>> flags);
>> - if (alloc)
>> + if (alloc && !memblock_reserve(alloc, size))
>> goto done;
>> }
>>
>> @@ -1303,7 +1302,6 @@ static void * __init memblock_virt_alloc_internal(
>>
>> return NULL;
>> done:
>> - memblock_reserve(alloc, size);
>> ptr = phys_to_virt(alloc);
>> memset(ptr, 0, size);
>>
>> --
>> 2.5.0
>
>--
>Michal Hocko
>SUSE Labs
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal()
2016-12-21 23:30 ` [PATCH 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() Wei Yang
@ 2016-12-22 9:15 ` Michal Hocko
2016-12-22 22:37 ` Wei Yang
0 siblings, 1 reply; 6+ messages in thread
From: Michal Hocko @ 2016-12-22 9:15 UTC (permalink / raw)
To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel
On Wed 21-12-16 23:30:33, Wei Yang wrote:
> memblock_reserve() would add a new range to memblock.reserved in case the
> new range is not totally covered by any of the current memblock.reserved
> range. If the memblock.reserved is full and can't resize,
> memblock_reserve() would fail.
>
> This doesn't happen in real world now, I observed this during code review.
> While theoretically, it has the chance to happen. And if it happens, others
> would think this range of memory is still available and may corrupt the
> memory.
OK, this explains it much better than the previous version! The silent
memory corruption is indeed too hard to debug to have this open even
when the issue is theoretical.
> This patch checks the return value and goto "done" after it succeeds.
>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Thanks!
> ---
> mm/memblock.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 4929e06..d0f2c96 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1274,18 +1274,17 @@ static void * __init memblock_virt_alloc_internal(
>
> if (max_addr > memblock.current_limit)
> max_addr = memblock.current_limit;
> -
> again:
> alloc = memblock_find_in_range_node(size, align, min_addr, max_addr,
> nid, flags);
> - if (alloc)
> + if (alloc && !memblock_reserve(alloc, size))
> goto done;
>
> if (nid != NUMA_NO_NODE) {
> alloc = memblock_find_in_range_node(size, align, min_addr,
> max_addr, NUMA_NO_NODE,
> flags);
> - if (alloc)
> + if (alloc && !memblock_reserve(alloc, size))
> goto done;
> }
>
> @@ -1303,7 +1302,6 @@ static void * __init memblock_virt_alloc_internal(
>
> return NULL;
> done:
> - memblock_reserve(alloc, size);
> ptr = phys_to_virt(alloc);
> memset(ptr, 0, size);
>
> --
> 2.5.0
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal()
2016-12-21 23:30 [PATCH V3 0/2] mm/memblock.c: fix potential bug and code refine Wei Yang
@ 2016-12-21 23:30 ` Wei Yang
2016-12-22 9:15 ` Michal Hocko
0 siblings, 1 reply; 6+ messages in thread
From: Wei Yang @ 2016-12-21 23:30 UTC (permalink / raw)
To: trivial, akpm, mhocko; +Cc: linux-mm, linux-kernel, Wei Yang
memblock_reserve() would add a new range to memblock.reserved in case the
new range is not totally covered by any of the current memblock.reserved
range. If the memblock.reserved is full and can't resize,
memblock_reserve() would fail.
This doesn't happen in real world now, I observed this during code review.
While theoretically, it has the chance to happen. And if it happens, others
would think this range of memory is still available and may corrupt the
memory.
This patch checks the return value and goto "done" after it succeeds.
Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
mm/memblock.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/mm/memblock.c b/mm/memblock.c
index 4929e06..d0f2c96 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1274,18 +1274,17 @@ static void * __init memblock_virt_alloc_internal(
if (max_addr > memblock.current_limit)
max_addr = memblock.current_limit;
-
again:
alloc = memblock_find_in_range_node(size, align, min_addr, max_addr,
nid, flags);
- if (alloc)
+ if (alloc && !memblock_reserve(alloc, size))
goto done;
if (nid != NUMA_NO_NODE) {
alloc = memblock_find_in_range_node(size, align, min_addr,
max_addr, NUMA_NO_NODE,
flags);
- if (alloc)
+ if (alloc && !memblock_reserve(alloc, size))
goto done;
}
@@ -1303,7 +1302,6 @@ static void * __init memblock_virt_alloc_internal(
return NULL;
done:
- memblock_reserve(alloc, size);
ptr = phys_to_virt(alloc);
memset(ptr, 0, size);
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-22 22:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-11 12:59 [PATCH 0/2] mm/memblock.c: fix potential bug and code refine Wei Yang
2016-12-11 12:59 ` [PATCH 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() Wei Yang
2016-12-11 12:59 ` [PATCH 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() Wei Yang
2016-12-21 23:30 [PATCH V3 0/2] mm/memblock.c: fix potential bug and code refine Wei Yang
2016-12-21 23:30 ` [PATCH 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() Wei Yang
2016-12-22 9:15 ` Michal Hocko
2016-12-22 22:37 ` Wei Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).