From: Dave Hansen <dave.hansen@intel.com>
To: Matt Fleming <matt.fleming@intel.com>,
Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Alan Cox <alan@lxorguk.ukuu.org.uk>
Subject: Re: [PATCH] mm: bootmem: Check pfn_valid() before accessing struct page
Date: Tue, 27 May 2014 11:45:59 -0700 [thread overview]
Message-ID: <5384DD67.3010408@intel.com> (raw)
In-Reply-To: <1401199802-10212-1-git-send-email-matt.fleming@intel.com>
On 05/27/2014 07:10 AM, Matt Fleming wrote:
> We need to check that a pfn is valid before handing it to pfn_to_page()
> since on low memory systems with CONFIG_HIGHMEM=n it's possible that a
> pfn may not have a corresponding struct page.
>
> This is in fact the case for one of Alan's machines where some of the
> EFI boot services pages live in highmem, and running a kernel without
> CONFIG_HIGHMEM enabled results in the following oops
...
> diff --git a/mm/bootmem.c b/mm/bootmem.c
> index 90bd3507b413..406e9cb1d58c 100644
> --- a/mm/bootmem.c
> +++ b/mm/bootmem.c
> @@ -164,6 +164,9 @@ void __init free_bootmem_late(unsigned long physaddr, unsigned long size)
> end = PFN_DOWN(physaddr + size);
>
> for (; cursor < end; cursor++) {
> + if (!pfn_valid(cursor))
> + continue;
> +
> __free_pages_bootmem(pfn_to_page(cursor), 0);
> totalram_pages++;
> }
I don't think this is quite right. pfn_valid() tells us whether we have
a 'struct page' there or not. *BUT*, it does not tell us whether it is
RAM that we can actually address and than can be freed in to the buddy
allocator.
I think sparsemem is where this matters. Let's say mem= caused lowmem
to end in the middle of a section (or that 896MB wasn't
section-aligned). Then someone calls free_bootmem_late() on an area
that is in the last section, but _above_ max_mapnr. It'll be
pfn_valid(), we'll free it in to the buddy allocator, and we'll blam the
first time we try to write to a bogus vaddr after a phys_to_virt().
At a higher level, I don't like the idea of the bootmem code papering
over bugs when somebody calls in to it trying to _free_ stuff that's not
memory (as far as the kernel is concerned).
I think the right thing to do is to call in to the e820 code and see if
the range is E820_RAM before trying to bootmem-free it.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
WARNING: multiple messages have this Message-ID (diff)
From: Dave Hansen <dave.hansen@intel.com>
To: Matt Fleming <matt.fleming@intel.com>,
Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Alan Cox <alan@lxorguk.ukuu.org.uk>
Subject: Re: [PATCH] mm: bootmem: Check pfn_valid() before accessing struct page
Date: Tue, 27 May 2014 11:45:59 -0700 [thread overview]
Message-ID: <5384DD67.3010408@intel.com> (raw)
In-Reply-To: <1401199802-10212-1-git-send-email-matt.fleming@intel.com>
On 05/27/2014 07:10 AM, Matt Fleming wrote:
> We need to check that a pfn is valid before handing it to pfn_to_page()
> since on low memory systems with CONFIG_HIGHMEM=n it's possible that a
> pfn may not have a corresponding struct page.
>
> This is in fact the case for one of Alan's machines where some of the
> EFI boot services pages live in highmem, and running a kernel without
> CONFIG_HIGHMEM enabled results in the following oops
...
> diff --git a/mm/bootmem.c b/mm/bootmem.c
> index 90bd3507b413..406e9cb1d58c 100644
> --- a/mm/bootmem.c
> +++ b/mm/bootmem.c
> @@ -164,6 +164,9 @@ void __init free_bootmem_late(unsigned long physaddr, unsigned long size)
> end = PFN_DOWN(physaddr + size);
>
> for (; cursor < end; cursor++) {
> + if (!pfn_valid(cursor))
> + continue;
> +
> __free_pages_bootmem(pfn_to_page(cursor), 0);
> totalram_pages++;
> }
I don't think this is quite right. pfn_valid() tells us whether we have
a 'struct page' there or not. *BUT*, it does not tell us whether it is
RAM that we can actually address and than can be freed in to the buddy
allocator.
I think sparsemem is where this matters. Let's say mem= caused lowmem
to end in the middle of a section (or that 896MB wasn't
section-aligned). Then someone calls free_bootmem_late() on an area
that is in the last section, but _above_ max_mapnr. It'll be
pfn_valid(), we'll free it in to the buddy allocator, and we'll blam the
first time we try to write to a bogus vaddr after a phys_to_virt().
At a higher level, I don't like the idea of the bootmem code papering
over bugs when somebody calls in to it trying to _free_ stuff that's not
memory (as far as the kernel is concerned).
I think the right thing to do is to call in to the e820 code and see if
the range is E820_RAM before trying to bootmem-free it.
next prev parent reply other threads:[~2014-05-27 18:46 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-27 14:10 [PATCH] mm: bootmem: Check pfn_valid() before accessing struct page Matt Fleming
2014-05-27 14:10 ` Matt Fleming
2014-05-27 18:45 ` Dave Hansen [this message]
2014-05-27 18:45 ` Dave Hansen
2014-06-03 15:17 ` Fleming, Matt
2014-06-03 15:17 ` Fleming, Matt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5384DD67.3010408@intel.com \
--to=dave.hansen@intel.com \
--cc=akpm@linux-foundation.org \
--cc=alan@lxorguk.ukuu.org.uk \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=matt.fleming@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.