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.
---
v3:
* remove the check for base instead of comment out
* Reform the changelog
v2:
* remove a trivial code refine, which is already fixed in upstream
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
memblock_is_region_memory() invoke memblock_search() to see whether the
base address is in the memory region. If it fails, idx would be -1. Then,
it returns 0.
If the memblock_search() returns a valid index, it means the base address
is guaranteed to be in the range memblock.memory.regions[idx]. Because of
this, it is not necessary to check the base again.
This patch removes the check on "base".
Signed-off-by: Wei Yang <[email protected]>
---
mm/memblock.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/mm/memblock.c b/mm/memblock.c
index 7608bc3..4929e06 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1615,8 +1615,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 &&
- (memblock.memory.regions[idx].base +
+ return (memblock.memory.regions[idx].base +
memblock.memory.regions[idx].size) >= end;
}
--
2.5.0
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 <[email protected]>
---
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
On Wed 21-12-16 23:30:32, Wei Yang wrote:
> memblock_is_region_memory() invoke memblock_search() to see whether the
> base address is in the memory region. If it fails, idx would be -1. Then,
> it returns 0.
>
> If the memblock_search() returns a valid index, it means the base address
> is guaranteed to be in the range memblock.memory.regions[idx]. Because of
> this, it is not necessary to check the base again.
>
> This patch removes the check on "base".
OK, the patch looks correct. I doubt it makes any real difference but I
do not see it being harmful.
> Signed-off-by: Wei Yang <[email protected]>
Acked-by: Michal Hocko <[email protected]>
> ---
> mm/memblock.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 7608bc3..4929e06 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1615,8 +1615,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 &&
> - (memblock.memory.regions[idx].base +
> + return (memblock.memory.regions[idx].base +
> memblock.memory.regions[idx].size) >= end;
> }
>
> --
> 2.5.0
--
Michal Hocko
SUSE Labs
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 <[email protected]>
Acked-by: Michal Hocko <[email protected]>
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
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 <[email protected]>
>
>Acked-by: Michal Hocko <[email protected]>
>
>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