| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1234139464.31963.79.camel@pasglop>
Date: Mon, 09 Feb 2009 11:31:04 +1100
From: Benjamin Herrenschmidt <benh@...nel.crashing.org>
To: Dave Hansen <dave@...ux.vnet.ibm.com>
Cc: linux-kernel@...r.kernel.org, mm-commits@...r.kernel.org,
chandru@...ibm.com, chandru@...ux.vnet.ibm.com, paulus@...ba.org,
akpm@...ux-foundation.org, Johannes Weiner <hannes@...xchg.org>
Subject: Re: +
powerpc-fix-code-for-reserved-memory-spanning-across-nodes.patch added to
-mm tree
Dave, you've had your hands in that code more than I did lately, do that
look ok to you ?
(BTW. On another note, do you still have pending bug fixes that didn't
got in .29 ?)
Cheers,
Ben.
On Sun, 2009-02-08 at 21:47 +0100, Johannes Weiner wrote:
> On Thu, Dec 25, 2008 at 12:06:21AM -0800, akpm@...ux-foundation.org wrote:
> >
> > The patch titled
> > powerpc: fix code for reserved memory spanning across nodes
> > has been added to the -mm tree. Its filename is
> > powerpc-fix-code-for-reserved-memory-spanning-across-nodes.patch
> >
> > Before you just go and hit "reply", please:
> > a) Consider who else should be cc'ed
> > b) Prefer to cc a suitable mailing list as well
> > c) Ideally: find the original patch on the mailing list and do a
> > reply-to-all to that, adding suitable additional cc's
> >
> > *** Remember to use Documentation/SubmitChecklist when testing your code ***
> >
> > See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
> > out what to do about this
> >
> > The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
> >
> > ------------------------------------------------------
> > Subject: powerpc: fix code for reserved memory spanning across nodes
> > From: Chandru <chandru@...ibm.com>
> >
> > When booted with crashkernel=224M@32M or any memory size less than this,
> > the system boots properly. The following was the observation.. The
> > system comes up with two nodes (0-256M and 256M-4GB). The crashkernel
> > memory reservation spans across these two nodes. The
> > mark_reserved_regions_for_nid() in arch/powerpc/mm/numa.c resizes the
> > reserved part of the memory within it as:
> >
> > if (end_pfn > node_ar.end_pfn)
> > reserve_size = (node_ar.end_pfn << PAGE_SHIFT)
> > - (start_pfn << PAGE_SHIFT);
> >
> >
> > but the reserve_bootmem_node() in mm/bootmem.c raises the pfn value of end
> >
> > end = PFN_UP(physaddr + size);
> >
> > This causes end to get a value past the last page in the 0-256M node.
> > Again when reserve_bootmem_node() returns, mark_reserved_regions_for_nid()
> > loops around to set the rest of the crashkernel memory in the next node as
> > reserved. It references NODE_DATA(node_ar.nid) and this causes another
> > 'Oops: kernel access of bad area' problem. The following changes made the
> > system to boot with any amount of crashkernel memory size.
> >
> > Signed-off-by: Chandru S <chandru@...ux.vnet.ibm.com>
> > Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> > Cc: Paul Mackerras <paulus@...ba.org>
> > Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
> > ---
> >
> > arch/powerpc/mm/numa.c | 7 ++++---
> > mm/bootmem.c | 4 ++++
> > 2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff -puN arch/powerpc/mm/numa.c~powerpc-fix-code-for-reserved-memory-spanning-across-nodes arch/powerpc/mm/numa.c
> > --- a/arch/powerpc/mm/numa.c~powerpc-fix-code-for-reserved-memory-spanning-across-nodes
> > +++ a/arch/powerpc/mm/numa.c
> > @@ -995,10 +995,11 @@ void __init do_init_bootmem(void)
> > start_pfn, end_pfn);
> >
> > free_bootmem_with_active_regions(nid, end_pfn);
> > + }
> > +
> > + for_each_online_node(nid) {
> > /*
> > - * Be very careful about moving this around. Future
> > - * calls to careful_allocation() depend on this getting
> > - * done correctly.
> > + * Be very careful about moving this around.
> > */
> > mark_reserved_regions_for_nid(nid);
> > sparse_memory_present_with_active_regions(nid);
> > diff -puN mm/bootmem.c~powerpc-fix-code-for-reserved-memory-spanning-across-nodes mm/bootmem.c
> > --- a/mm/bootmem.c~powerpc-fix-code-for-reserved-memory-spanning-across-nodes
> > +++ a/mm/bootmem.c
> > @@ -375,10 +375,14 @@ int __init reserve_bootmem_node(pg_data_
> > unsigned long size, int flags)
> > {
> > unsigned long start, end;
> > + bootmem_data_t *bdata = pgdat->bdata;
> >
> > start = PFN_DOWN(physaddr);
> > end = PFN_UP(physaddr + size);
> >
> > + if (end > bdata->node_low_pfn)
> > + end = bdata->node_low_pfn;
>
> Chandru,
>
> from your patch descriptions I read two reasons why your bootup fails:
>
> 1. Rounding a contained range so that it exceeds the node
>
> This is impossible. Either your range includes that page already or
> it doesn't. bootmem doesn't reserve more pages than requested, it
> just rounds up to full pages if your range includes partials. Your
> size must already overflow the node boundary if the end pfn is too
> high.
>
> 2. Node-spanning range
>
> If you have ranges that span nodes you are using the wrong interface.
> By passing a node descriptor to reserve_bootmem_node(), you expect the
> specified range to be on that node. If it is not, please use the
> node-agnostic reserve_bootmem() interface.
>
> The above patch brings a kludge back to bootmem that silently cuts
> bogus ranges. I don't agree, these bugs should be fixed, not worked
> around.
>
> What I can spot in the callsite is the following:
>
> unsigned long physbase = lmb.reserved.region[i].base;
> unsigned long size = lmb.reserved.region[i].size;
> unsigned long start_pfn = physbase >> PAGE_SHIFT;
> unsigned long end_pfn = ((physbase + size) >> PAGE_SHIFT);
>
> When physbase + size is not a multiple of PAGE_SIZE, the overlap is
> cut off and end_pfn is too low.
>
> if (end_pfn > node_ar.end_pfn)
> reserve_size = (node_ar.end_pfn << PAGE_SHIFT)
> - (start_pfn << PAGE_SHIFT);
>
> The test could then fail even if the size should have been trimmed.
> Since the overlap must be less than PAGE_SIZE for this to happen, this
> would explain what you noticed: the bootmem range exceeds the node
> boundary by exactly one PFN because of the rounding (if I got you
> right).
>
> Patch attached, can not test, sorry, eat carefully.
>
> Hannes
>
> PS: Andrew, I wasn't CC'd but this patch was listed in this `patches
> that might be from hannes@...ppp'-list from another mm-commit message.
> It's magical!
>
> ---
> From: Johannes Weiner <hannes@...xchg.org>
> Subject: powerpc: fix rounding error in teaching bootmem about LMB
>
> If the reserved LMB does not exactly span complete pages, treating
> (start + size) >> PAGE_SHIFT as the ending PFN is an off by one error.
>
> The subsequent check for whether the region needs to be trimmed to fit
> the underlying node can now fail if the range exceeds the node by 1 to
> PAGE_SIZE - 1 bytes. The excessive range is then passed to bootmem
> which BUG()s out on it correctly.
>
> Fix up the rounding to include all pages the LMB spans, even partial
> ones.
>
> Signed-off-by: Johannes Weiner <hannes@...xchg.org>
> ---
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 7393bd7..38016a2 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -12,6 +12,7 @@
> #include <linux/bootmem.h>
> #include <linux/init.h>
> #include <linux/mm.h>
> +#include <linux/pfn.h>
> #include <linux/mmzone.h>
> #include <linux/module.h>
> #include <linux/nodemask.h>
> @@ -881,8 +882,8 @@ static void mark_reserved_regions_for_nid(int nid)
> for (i = 0; i < lmb.reserved.cnt; i++) {
> unsigned long physbase = lmb.reserved.region[i].base;
> unsigned long size = lmb.reserved.region[i].size;
> - unsigned long start_pfn = physbase >> PAGE_SHIFT;
> - unsigned long end_pfn = ((physbase + size) >> PAGE_SHIFT);
> + unsigned long start_pfn = PFN_DOWN(physbase);
> + unsigned long end_pfn = PFN_UP(physbase + size);
> struct node_active_region node_ar;
> unsigned long node_end_pfn = node->node_start_pfn +
> node->node_spanned_pages;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists