Re: [PATCH v2 19/21] treewide: add checks for the return value of memblock_alloc*()
On Thu, Jan 31, 2019 at 08:07:29AM +0100, Christophe Leroy wrote:
>
>
> Le 31/01/2019 à 07:44, Christophe Leroy a écrit :
> >
> >
> >Le 31/01/2019 à 07:41, Mike Rapoport a écrit :
> >>On Thu, Jan 31, 2019 at 07:07:46AM +0100, Christophe Leroy wrote:
> >>>
> >>>
> >>>Le 21/01/2019 à 09:04, Mike Rapoport a écrit :
> Add check for the return value of memblock_alloc*() functions and call
> panic() in case of error.
> The panic message repeats the one used by panicing memblock
> allocators with
> adjustment of parameters to include only relevant ones.
>
> The replacement was mostly automated with semantic patches like the one
> below with manual massaging of format strings.
>
> @@
> expression ptr, size, align;
> @@
> ptr = memblock_alloc(size, align);
> + if (!ptr)
> + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
> size, align);
>
> Signed-off-by: Mike Rapoport
> Reviewed-by: Guo Ren # c-sky
> Acked-by: Paul Burton # MIPS
> Acked-by: Heiko Carstens # s390
> Reviewed-by: Juergen Gross # Xen
> ---
> >>>
> >>>[...]
> >>>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 7ea5dc6..ad94242 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> >>>
> >>>[...]
> >>>
> @@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned
> long size, int nid)
> memblock_alloc_try_nid_raw(size, PAGE_SIZE,
> __pa(MAX_DMA_ADDRESS),
> MEMBLOCK_ALLOC_ACCESSIBLE, nid);
> + if (!sparsemap_buf)
> + panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d
> from=%lx\n",
> + __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS));
> +
> >>>
> >>>memblock_alloc_try_nid_raw() does not panic (help explicitly says:
> >>>Does not
> >>>zero allocated memory, does not panic if request cannot be satisfied.).
> >>
> >>"Does not panic" does not mean it always succeeds.
> >
> >I agree, but at least here you are changing the behaviour by making it
> >panic explicitly. Are we sure there are not cases where the system could
> >just continue functionning ? Maybe a WARN_ON() would be enough there ?
>
> Looking more in details, it looks like everything is done to live with
> sparsemap_buf NULL, all functions using it check it so having it NULL
> shouldn't imply a panic I believe, see code below.
You are right, I'm preparing the fix right now.
> static void *sparsemap_buf __meminitdata;
> static void *sparsemap_buf_end __meminitdata;
>
> static void __init sparse_buffer_init(unsigned long size, int nid)
> {
> WARN_ON(sparsemap_buf); /* forgot to call sparse_buffer_fini()? */
> sparsemap_buf =
> memblock_alloc_try_nid_raw(size, PAGE_SIZE,
> __pa(MAX_DMA_ADDRESS),
> MEMBLOCK_ALLOC_ACCESSIBLE, nid);
> sparsemap_buf_end = sparsemap_buf + size;
> }
>
> static void __init sparse_buffer_fini(void)
> {
> unsigned long size = sparsemap_buf_end - sparsemap_buf;
>
> if (sparsemap_buf && size > 0)
> memblock_free_early(__pa(sparsemap_buf), size);
> sparsemap_buf = NULL;
> }
>
> void * __meminit sparse_buffer_alloc(unsigned long size)
> {
> void *ptr = NULL;
>
> if (sparsemap_buf) {
> ptr = PTR_ALIGN(sparsemap_buf, size);
> if (ptr + size > sparsemap_buf_end)
> ptr = NULL;
> else
> sparsemap_buf = ptr + size;
> }
> return ptr;
> }
>
>
> Christophe
>
--
Sincerely yours,
Mike.
Re: [PATCH v2 19/21] treewide: add checks for the return value of memblock_alloc*()
Le 31/01/2019 à 07:44, Christophe Leroy a écrit :
Le 31/01/2019 à 07:41, Mike Rapoport a écrit :
On Thu, Jan 31, 2019 at 07:07:46AM +0100, Christophe Leroy wrote:
Le 21/01/2019 à 09:04, Mike Rapoport a écrit :
Add check for the return value of memblock_alloc*() functions and call
panic() in case of error.
The panic message repeats the one used by panicing memblock
allocators with
adjustment of parameters to include only relevant ones.
The replacement was mostly automated with semantic patches like the one
below with manual massaging of format strings.
@@
expression ptr, size, align;
@@
ptr = memblock_alloc(size, align);
+ if (!ptr)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
size, align);
Signed-off-by: Mike Rapoport
Reviewed-by: Guo Ren # c-sky
Acked-by: Paul Burton # MIPS
Acked-by: Heiko Carstens # s390
Reviewed-by: Juergen Gross # Xen
---
[...]
diff --git a/mm/sparse.c b/mm/sparse.c
index 7ea5dc6..ad94242 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
[...]
@@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned
long size, int nid)
memblock_alloc_try_nid_raw(size, PAGE_SIZE,
__pa(MAX_DMA_ADDRESS),
MEMBLOCK_ALLOC_ACCESSIBLE, nid);
+ if (!sparsemap_buf)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d
from=%lx\n",
+ __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS));
+
memblock_alloc_try_nid_raw() does not panic (help explicitly says:
Does not
zero allocated memory, does not panic if request cannot be satisfied.).
"Does not panic" does not mean it always succeeds.
I agree, but at least here you are changing the behaviour by making it
panic explicitly. Are we sure there are not cases where the system could
just continue functionning ? Maybe a WARN_ON() would be enough there ?
Looking more in details, it looks like everything is done to live with
sparsemap_buf NULL, all functions using it check it so having it NULL
shouldn't imply a panic I believe, see code below.
static void *sparsemap_buf __meminitdata;
static void *sparsemap_buf_end __meminitdata;
static void __init sparse_buffer_init(unsigned long size, int nid)
{
WARN_ON(sparsemap_buf); /* forgot to call sparse_buffer_fini()? */
sparsemap_buf =
memblock_alloc_try_nid_raw(size, PAGE_SIZE,
__pa(MAX_DMA_ADDRESS),
MEMBLOCK_ALLOC_ACCESSIBLE, nid);
sparsemap_buf_end = sparsemap_buf + size;
}
static void __init sparse_buffer_fini(void)
{
unsigned long size = sparsemap_buf_end - sparsemap_buf;
if (sparsemap_buf && size > 0)
memblock_free_early(__pa(sparsemap_buf), size);
sparsemap_buf = NULL;
}
void * __meminit sparse_buffer_alloc(unsigned long size)
{
void *ptr = NULL;
if (sparsemap_buf) {
ptr = PTR_ALIGN(sparsemap_buf, size);
if (ptr + size > sparsemap_buf_end)
ptr = NULL;
else
sparsemap_buf = ptr + size;
}
return ptr;
}
Christophe
Re: [PATCH v2 19/21] treewide: add checks for the return value of memblock_alloc*()
Le 31/01/2019 à 07:41, Mike Rapoport a écrit :
On Thu, Jan 31, 2019 at 07:07:46AM +0100, Christophe Leroy wrote:
Le 21/01/2019 à 09:04, Mike Rapoport a écrit :
Add check for the return value of memblock_alloc*() functions and call
panic() in case of error.
The panic message repeats the one used by panicing memblock allocators with
adjustment of parameters to include only relevant ones.
The replacement was mostly automated with semantic patches like the one
below with manual massaging of format strings.
@@
expression ptr, size, align;
@@
ptr = memblock_alloc(size, align);
+ if (!ptr)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
size, align);
Signed-off-by: Mike Rapoport
Reviewed-by: Guo Ren # c-sky
Acked-by: Paul Burton# MIPS
Acked-by: Heiko Carstens # s390
Reviewed-by: Juergen Gross # Xen
---
[...]
diff --git a/mm/sparse.c b/mm/sparse.c
index 7ea5dc6..ad94242 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
[...]
@@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned long size,
int nid)
memblock_alloc_try_nid_raw(size, PAGE_SIZE,
__pa(MAX_DMA_ADDRESS),
MEMBLOCK_ALLOC_ACCESSIBLE, nid);
+ if (!sparsemap_buf)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d
from=%lx\n",
+ __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS));
+
memblock_alloc_try_nid_raw() does not panic (help explicitly says: Does not
zero allocated memory, does not panic if request cannot be satisfied.).
"Does not panic" does not mean it always succeeds.
I agree, but at least here you are changing the behaviour by making it
panic explicitly. Are we sure there are not cases where the system could
just continue functionning ? Maybe a WARN_ON() would be enough there ?
Christophe
Stephen Rothwell reports a boot failure due to this change.
Please see my reply on that thread.
Christophe
sparsemap_buf_end = sparsemap_buf + size;
}
Re: [PATCH v2 19/21] treewide: add checks for the return value of memblock_alloc*()
On Thu, Jan 31, 2019 at 07:07:46AM +0100, Christophe Leroy wrote:
>
>
> Le 21/01/2019 à 09:04, Mike Rapoport a écrit :
> >Add check for the return value of memblock_alloc*() functions and call
> >panic() in case of error.
> >The panic message repeats the one used by panicing memblock allocators with
> >adjustment of parameters to include only relevant ones.
> >
> >The replacement was mostly automated with semantic patches like the one
> >below with manual massaging of format strings.
> >
> >@@
> >expression ptr, size, align;
> >@@
> >ptr = memblock_alloc(size, align);
> >+ if (!ptr)
> >+panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
> >size, align);
> >
> >Signed-off-by: Mike Rapoport
> >Reviewed-by: Guo Ren # c-sky
> >Acked-by: Paul Burton # MIPS
> >Acked-by: Heiko Carstens # s390
> >Reviewed-by: Juergen Gross # Xen
> >---
>
> [...]
>
> >diff --git a/mm/sparse.c b/mm/sparse.c
> >index 7ea5dc6..ad94242 100644
> >--- a/mm/sparse.c
> >+++ b/mm/sparse.c
>
> [...]
>
> >@@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned long
> >size, int nid)
> > memblock_alloc_try_nid_raw(size, PAGE_SIZE,
> > __pa(MAX_DMA_ADDRESS),
> > MEMBLOCK_ALLOC_ACCESSIBLE, nid);
> >+if (!sparsemap_buf)
> >+panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d
> >from=%lx\n",
> >+ __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS));
> >+
>
> memblock_alloc_try_nid_raw() does not panic (help explicitly says: Does not
> zero allocated memory, does not panic if request cannot be satisfied.).
"Does not panic" does not mean it always succeeds.
> Stephen Rothwell reports a boot failure due to this change.
Please see my reply on that thread.
> Christophe
>
> > sparsemap_buf_end = sparsemap_buf + size;
> > }
> >
>
--
Sincerely yours,
Mike.
Re: [PATCH v2 19/21] treewide: add checks for the return value of memblock_alloc*()
Le 21/01/2019 à 09:04, Mike Rapoport a écrit :
Add check for the return value of memblock_alloc*() functions and call
panic() in case of error.
The panic message repeats the one used by panicing memblock allocators with
adjustment of parameters to include only relevant ones.
The replacement was mostly automated with semantic patches like the one
below with manual massaging of format strings.
@@
expression ptr, size, align;
@@
ptr = memblock_alloc(size, align);
+ if (!ptr)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
size, align);
Signed-off-by: Mike Rapoport
Reviewed-by: Guo Ren # c-sky
Acked-by: Paul Burton# MIPS
Acked-by: Heiko Carstens # s390
Reviewed-by: Juergen Gross # Xen
---
[...]
diff --git a/mm/sparse.c b/mm/sparse.c
index 7ea5dc6..ad94242 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
[...]
@@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned long size,
int nid)
memblock_alloc_try_nid_raw(size, PAGE_SIZE,
__pa(MAX_DMA_ADDRESS),
MEMBLOCK_ALLOC_ACCESSIBLE, nid);
+ if (!sparsemap_buf)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d
from=%lx\n",
+ __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS));
+
memblock_alloc_try_nid_raw() does not panic (help explicitly says: Does
not zero allocated memory, does not panic if request cannot be satisfied.).
Stephen Rothwell reports a boot failure due to this change.
Christophe
sparsemap_buf_end = sparsemap_buf + size;
}
Re: [PATCH v2 19/21] treewide: add checks for the return value of memblock_alloc*()
On Mon, Jan 21, 2019 at 10:04:06AM +0200, Mike Rapoport wrote:
> Add check for the return value of memblock_alloc*() functions and call
> panic() in case of error.
> The panic message repeats the one used by panicing memblock allocators with
> adjustment of parameters to include only relevant ones.
>
> The replacement was mostly automated with semantic patches like the one
> below with manual massaging of format strings.
>
> @@
> expression ptr, size, align;
> @@
> ptr = memblock_alloc(size, align);
> + if (!ptr)
> + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
> size, align);
>
> Signed-off-by: Mike Rapoport
> Reviewed-by: Guo Ren # c-sky
> Acked-by: Paul Burton# MIPS
> Acked-by: Heiko Carstens # s390
> Reviewed-by: Juergen Gross # Xen
> ---
> arch/alpha/kernel/core_cia.c | 3 +++
> arch/alpha/kernel/core_marvel.c | 6 ++
> arch/alpha/kernel/pci-noop.c | 13 +++--
> arch/alpha/kernel/pci.c | 11 ++-
> arch/alpha/kernel/pci_iommu.c | 12
> arch/arc/mm/highmem.c | 4
> arch/arm/kernel/setup.c | 6 ++
> arch/arm/mm/mmu.c | 14 +-
> arch/arm64/kernel/setup.c | 8 +---
> arch/arm64/mm/kasan_init.c| 10 ++
> arch/c6x/mm/dma-coherent.c| 4
> arch/c6x/mm/init.c| 3 +++
> arch/csky/mm/highmem.c| 5 +
> arch/h8300/mm/init.c | 3 +++
> arch/m68k/atari/stram.c | 4
> arch/m68k/mm/init.c | 3 +++
> arch/m68k/mm/mcfmmu.c | 6 ++
> arch/m68k/mm/motorola.c | 9 +
> arch/m68k/mm/sun3mmu.c| 6 ++
> arch/m68k/sun3/sun3dvma.c | 3 +++
> arch/microblaze/mm/init.c | 8 ++--
> arch/mips/cavium-octeon/dma-octeon.c | 3 +++
> arch/mips/kernel/setup.c | 3 +++
> arch/mips/kernel/traps.c | 3 +++
> arch/mips/mm/init.c | 5 +
> arch/nds32/mm/init.c | 12
> arch/openrisc/mm/ioremap.c| 8 ++--
> arch/powerpc/kernel/dt_cpu_ftrs.c | 5 +
> arch/powerpc/kernel/pci_32.c | 3 +++
> arch/powerpc/kernel/setup-common.c| 3 +++
> arch/powerpc/kernel/setup_64.c| 4
> arch/powerpc/lib/alloc.c | 3 +++
> arch/powerpc/mm/hash_utils_64.c | 3 +++
> arch/powerpc/mm/mmu_context_nohash.c | 9 +
> arch/powerpc/mm/pgtable-book3e.c | 12 ++--
> arch/powerpc/mm/pgtable-book3s64.c| 3 +++
> arch/powerpc/mm/pgtable-radix.c | 9 -
> arch/powerpc/mm/ppc_mmu_32.c | 3 +++
> arch/powerpc/platforms/pasemi/iommu.c | 3 +++
> arch/powerpc/platforms/powermac/nvram.c | 3 +++
> arch/powerpc/platforms/powernv/opal.c | 3 +++
> arch/powerpc/platforms/powernv/pci-ioda.c | 8
> arch/powerpc/platforms/ps3/setup.c| 3 +++
> arch/powerpc/sysdev/msi_bitmap.c | 3 +++
> arch/s390/kernel/setup.c | 13 +
> arch/s390/kernel/smp.c| 5 -
> arch/s390/kernel/topology.c | 6 ++
> arch/s390/numa/mode_emu.c | 3 +++
> arch/s390/numa/numa.c | 6 +-
> arch/sh/mm/init.c | 6 ++
> arch/sh/mm/numa.c | 4
> arch/um/drivers/net_kern.c| 3 +++
> arch/um/drivers/vector_kern.c | 3 +++
> arch/um/kernel/initrd.c | 2 ++
> arch/um/kernel/mem.c | 16
> arch/unicore32/kernel/setup.c | 4
> arch/unicore32/mm/mmu.c | 15 +--
> arch/x86/kernel/acpi/boot.c | 3 +++
> arch/x86/kernel/apic/io_apic.c| 5 +
> arch/x86/kernel/e820.c| 3 +++
> arch/x86/platform/olpc/olpc_dt.c | 3 +++
> arch/x86/xen/p2m.c| 11 +--
> arch/xtensa/mm/kasan_init.c | 4
> arch/xtensa/mm/mmu.c | 3 +++
> drivers/clk/ti/clk.c | 3 +++
> drivers/macintosh/smu.c | 3 +++
> drivers/of/fdt.c | 8 +++-
> drivers/of/unittest.c | 8 +++-
Acked-by: Rob Herring
> drivers/xen/swiotlb-xen.c | 7 +--
> kernel/power/snapshot.c | 3 +++
> lib/cpumask.c | 3 +++
> mm/kasan/init.c | 10 --
> mm/sparse.c | 19 +--
[PATCH v2 19/21] treewide: add checks for the return value of memblock_alloc*()
Add check for the return value of memblock_alloc*() functions and call
panic() in case of error.
The panic message repeats the one used by panicing memblock allocators with
adjustment of parameters to include only relevant ones.
The replacement was mostly automated with semantic patches like the one
below with manual massaging of format strings.
@@
expression ptr, size, align;
@@
ptr = memblock_alloc(size, align);
+ if (!ptr)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
size, align);
Signed-off-by: Mike Rapoport
Reviewed-by: Guo Ren # c-sky
Acked-by: Paul Burton # MIPS
Acked-by: Heiko Carstens # s390
Reviewed-by: Juergen Gross # Xen
---
arch/alpha/kernel/core_cia.c | 3 +++
arch/alpha/kernel/core_marvel.c | 6 ++
arch/alpha/kernel/pci-noop.c | 13 +++--
arch/alpha/kernel/pci.c | 11 ++-
arch/alpha/kernel/pci_iommu.c | 12
arch/arc/mm/highmem.c | 4
arch/arm/kernel/setup.c | 6 ++
arch/arm/mm/mmu.c | 14 +-
arch/arm64/kernel/setup.c | 8 +---
arch/arm64/mm/kasan_init.c| 10 ++
arch/c6x/mm/dma-coherent.c| 4
arch/c6x/mm/init.c| 3 +++
arch/csky/mm/highmem.c| 5 +
arch/h8300/mm/init.c | 3 +++
arch/m68k/atari/stram.c | 4
arch/m68k/mm/init.c | 3 +++
arch/m68k/mm/mcfmmu.c | 6 ++
arch/m68k/mm/motorola.c | 9 +
arch/m68k/mm/sun3mmu.c| 6 ++
arch/m68k/sun3/sun3dvma.c | 3 +++
arch/microblaze/mm/init.c | 8 ++--
arch/mips/cavium-octeon/dma-octeon.c | 3 +++
arch/mips/kernel/setup.c | 3 +++
arch/mips/kernel/traps.c | 3 +++
arch/mips/mm/init.c | 5 +
arch/nds32/mm/init.c | 12
arch/openrisc/mm/ioremap.c| 8 ++--
arch/powerpc/kernel/dt_cpu_ftrs.c | 5 +
arch/powerpc/kernel/pci_32.c | 3 +++
arch/powerpc/kernel/setup-common.c| 3 +++
arch/powerpc/kernel/setup_64.c| 4
arch/powerpc/lib/alloc.c | 3 +++
arch/powerpc/mm/hash_utils_64.c | 3 +++
arch/powerpc/mm/mmu_context_nohash.c | 9 +
arch/powerpc/mm/pgtable-book3e.c | 12 ++--
arch/powerpc/mm/pgtable-book3s64.c| 3 +++
arch/powerpc/mm/pgtable-radix.c | 9 -
arch/powerpc/mm/ppc_mmu_32.c | 3 +++
arch/powerpc/platforms/pasemi/iommu.c | 3 +++
arch/powerpc/platforms/powermac/nvram.c | 3 +++
arch/powerpc/platforms/powernv/opal.c | 3 +++
arch/powerpc/platforms/powernv/pci-ioda.c | 8
arch/powerpc/platforms/ps3/setup.c| 3 +++
arch/powerpc/sysdev/msi_bitmap.c | 3 +++
arch/s390/kernel/setup.c | 13 +
arch/s390/kernel/smp.c| 5 -
arch/s390/kernel/topology.c | 6 ++
arch/s390/numa/mode_emu.c | 3 +++
arch/s390/numa/numa.c | 6 +-
arch/sh/mm/init.c | 6 ++
arch/sh/mm/numa.c | 4
arch/um/drivers/net_kern.c| 3 +++
arch/um/drivers/vector_kern.c | 3 +++
arch/um/kernel/initrd.c | 2 ++
arch/um/kernel/mem.c | 16
arch/unicore32/kernel/setup.c | 4
arch/unicore32/mm/mmu.c | 15 +--
arch/x86/kernel/acpi/boot.c | 3 +++
arch/x86/kernel/apic/io_apic.c| 5 +
arch/x86/kernel/e820.c| 3 +++
arch/x86/platform/olpc/olpc_dt.c | 3 +++
arch/x86/xen/p2m.c| 11 +--
arch/xtensa/mm/kasan_init.c | 4
arch/xtensa/mm/mmu.c | 3 +++
drivers/clk/ti/clk.c | 3 +++
drivers/macintosh/smu.c | 3 +++
drivers/of/fdt.c | 8 +++-
drivers/of/unittest.c | 8 +++-
drivers/xen/swiotlb-xen.c | 7 +--
kernel/power/snapshot.c | 3 +++
lib/cpumask.c | 3 +++
mm/kasan/init.c | 10 --
mm/sparse.c | 19 +--
73 files changed, 409 insertions(+), 28 deletions(-)
diff --git a/arch/alpha/kernel/core_cia.c b/arch/alpha/kernel/core_cia.c
index 466cd44..f489170 100644
--- a/arch/alpha/kernel/core_cia.c
+++ b/arch/alpha/kernel/core_cia.c
@@ -332,6 +332,9 @@
