2015-07-24 20:53:58

by Chris Metcalf

Subject: [PATCH] bootmem: avoid freeing to bootmem after bootmem is done

Bootmem isn't popular any more, but some architectures still use
it, and freeing to bootmem after calling free_all_bootmem_core()
can end up scribbling over random memory. Instead, make sure the
kernel panics by ensuring the node_bootmem_map field is non-NULL
when are freeing or marking bootmem.

An instance of this bug was just fixed in the tile architecture
("tile: use free_bootmem_late() for initrd") and catching this case
more widely seems like a good thing.

Signed-off-by: Chris Metcalf <[email protected]>
---
mm/bootmem.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/mm/bootmem.c b/mm/bootmem.c
index a23dd1934654..178748259736 100644
--- a/mm/bootmem.c
+++ b/mm/bootmem.c
@@ -236,6 +236,7 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
count += pages;
while (pages--)
__free_pages_bootmem(page++, cur++, 0);
+ bdata->node_bootmem_map = NULL;

bdebug("nid=%td released=%lx\n", bdata - bootmem_node_data, count);

@@ -294,6 +295,8 @@ static void __init __free(bootmem_data_t *bdata,
sidx + bdata->node_min_pfn,
eidx + bdata->node_min_pfn);

+ BUG_ON(bdata->node_bootmem_map == NULL);
+
if (bdata->hint_idx > sidx)
bdata->hint_idx = sidx;

@@ -314,6 +317,8 @@ static int __init __reserve(bootmem_data_t *bdata, unsigned long sidx,
eidx + bdata->node_min_pfn,
flags);

+ BUG_ON(bdata->node_bootmem_map == NULL);
+
for (idx = sidx; idx < eidx; idx++)
if (test_and_set_bit(idx, bdata->node_bootmem_map)) {
if (exclusive) {
--
2.1.2

2015-07-27 10:59:58

by Mel Gorman

Subject: Re: [PATCH] bootmem: avoid freeing to bootmem after bootmem is done

On Fri, Jul 24, 2015 at 04:53:46PM -0400, Chris Metcalf wrote:
> Bootmem isn't popular any more, but some architectures still use
> it, and freeing to bootmem after calling free_all_bootmem_core()
> can end up scribbling over random memory. Instead, make sure the
> kernel panics by ensuring the node_bootmem_map field is non-NULL
> when are freeing or marking bootmem.
>
> An instance of this bug was just fixed in the tile architecture
> ("tile: use free_bootmem_late() for initrd") and catching this case
> more widely seems like a good thing.
>
> Signed-off-by: Chris Metcalf <[email protected]>

In general it looks fine but you could just WARN_ON, return and still
boot the kernel too. Obviously it would need to be fixed but Linus will
push back if he spots a BUG_ON when there was a recovery option.

--
Mel Gorman
SUSE Labs

2015-07-27 15:36:42

by Chris Metcalf

Subject: [PATCH v2] bootmem: avoid freeing to bootmem after bootmem is done

Bootmem isn't popular any more, but some architectures still use it,
and freeing to bootmem after calling free_all_bootmem_core() can end
up scribbling over random memory. Instead, make sure the kernel
generates a warning in this case by ensuring the node_bootmem_map
field is non-NULL when are freeing or marking bootmem.

An instance of this bug was just fixed in the tile architecture
("tile: use free_bootmem_late() for initrd") and catching this case
more widely seems like a good thing.

Signed-off-by: Chris Metcalf <[email protected]>
---
v2: use WARN_ON() instead of BUG_ON() [Mel Gorman]

mm/bootmem.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/mm/bootmem.c b/mm/bootmem.c
index a23dd1934654..3b6380784c28 100644
--- a/mm/bootmem.c
+++ b/mm/bootmem.c
@@ -236,6 +236,7 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
count += pages;
while (pages--)
__free_pages_bootmem(page++, cur++, 0);
+ bdata->node_bootmem_map = NULL;

bdebug("nid=%td released=%lx\n", bdata - bootmem_node_data, count);

@@ -294,6 +295,9 @@ static void __init __free(bootmem_data_t *bdata,
sidx + bdata->node_min_pfn,
eidx + bdata->node_min_pfn);

+ if (WARN_ON(bdata->node_bootmem_map == NULL))
+ return;
+
if (bdata->hint_idx > sidx)
bdata->hint_idx = sidx;

@@ -314,6 +318,9 @@ static int __init __reserve(bootmem_data_t *bdata, unsigned long sidx,
eidx + bdata->node_min_pfn,
flags);

+ if (WARN_ON(bdata->node_bootmem_map == NULL))
+ return 0;
+
for (idx = sidx; idx < eidx; idx++)
if (test_and_set_bit(idx, bdata->node_bootmem_map)) {
if (exclusive) {
--
2.1.2

2015-07-27 16:05:32

by Mel Gorman

Subject: Re: [PATCH v2] bootmem: avoid freeing to bootmem after bootmem is done

On Mon, Jul 27, 2015 at 11:36:06AM -0400, Chris Metcalf wrote:
> Bootmem isn't popular any more, but some architectures still use it,
> and freeing to bootmem after calling free_all_bootmem_core() can end
> up scribbling over random memory. Instead, make sure the kernel
> generates a warning in this case by ensuring the node_bootmem_map
> field is non-NULL when are freeing or marking bootmem.
>
> An instance of this bug was just fixed in the tile architecture
> ("tile: use free_bootmem_late() for initrd") and catching this case
> more widely seems like a good thing.
>
> Signed-off-by: Chris Metcalf <[email protected]>

Acked-by: Mel Gorman <[email protected]>

--
Mel Gorman
SUSE Labs