Re: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*()
Hi Mike,
On Wed, Jan 16, 2019 at 03:44:19PM +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
> ---
>%
> diff --git a/arch/mips/cavium-octeon/dma-octeon.c
> b/arch/mips/cavium-octeon/dma-octeon.c
> index e8eb60e..db1deb2 100644
> --- a/arch/mips/cavium-octeon/dma-octeon.c
> +++ b/arch/mips/cavium-octeon/dma-octeon.c
> @@ -245,6 +245,9 @@ void __init plat_swiotlb_setup(void)
> swiotlbsize = swiotlb_nslabs << IO_TLB_SHIFT;
>
> octeon_swiotlb = memblock_alloc_low(swiotlbsize, PAGE_SIZE);
> + if (!octeon_swiotlb)
> + panic("%s: Failed to allocate %lu bytes align=%lx\n",
> + __func__, swiotlbsize, PAGE_SIZE);
>
> if (swiotlb_init_with_tbl(octeon_swiotlb, swiotlb_nslabs, 1) == -ENOMEM)
> panic("Cannot allocate SWIOTLB buffer");
That one should be %zu rather than %lu. The rest looks good, so with
that one tweak:
Acked-by: Paul Burton # MIPS parts
Thanks,
Paul
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*()
On Wed, Jan 16, 2019 at 03:44:19PM +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
...
> diff --git a/arch/s390/numa/toptree.c b/arch/s390/numa/toptree.c
> index 71a608c..0118c77 100644
> --- a/arch/s390/numa/toptree.c
> +++ b/arch/s390/numa/toptree.c
> @@ -31,10 +31,14 @@ struct toptree __ref *toptree_alloc(int level, int id)
> {
> struct toptree *res;
>
> - if (slab_is_available())
> + if (slab_is_available()) {
> res = kzalloc(sizeof(*res), GFP_KERNEL);
> - else
> + } else {
> res = memblock_alloc(sizeof(*res), 8);
> + if (!res)
> + panic("%s: Failed to allocate %zu bytes align=0x%x\n",
> + __func__, sizeof(*res), 8);
> + }
> if (!res)
> return res;
Please remove this hunk, since the code _should_ be able to handle
allocation failures anyway (see end of quoted code).
Otherwise for the s390 bits:
Acked-by: Heiko Carstens
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*()
On Wed, Jan 16, 2019 at 03:44:19PM +0200, Mike Rapoport wrote:
> arch/csky/mm/highmem.c| 5 +
...
> diff --git a/arch/csky/mm/highmem.c b/arch/csky/mm/highmem.c
> index 53b1bfa..3317b774 100644
> --- a/arch/csky/mm/highmem.c
> +++ b/arch/csky/mm/highmem.c
> @@ -141,6 +141,11 @@ static void __init fixrange_init(unsigned long start,
> unsigned long end,
> for (; (k < PTRS_PER_PMD) && (vaddr != end); pmd++,
> k++) {
> if (pmd_none(*pmd)) {
> pte = (pte_t *)
> memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
> + if (!pte)
> + panic("%s: Failed to allocate
> %lu bytes align=%lx\n",
> + __func__, PAGE_SIZE,
> + PAGE_SIZE);
> +
> set_pmd(pmd, __pmd(__pa(pte)));
> BUG_ON(pte != pte_offset_kernel(pmd,
> 0));
> }
Looks good for me and panic is ok.
Reviewed-by: Guo Ren
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*()
On Wed, Jan 16, 2019 at 03:27:35PM +0100, Geert Uytterhoeven wrote:
> Hi Mike,
>
> On Wed, Jan 16, 2019 at 2:46 PM 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__,
>
> In general, you want to use %zu for size_t
>
> > size, align);
> >
> > Signed-off-by: Mike Rapoport
>
> Thanks for your patch!
>
> > 74 files changed, 415 insertions(+), 29 deletions(-)
>
> I'm wondering if this is really an improvement?
>From memblock perspective it's definitely an improvement :)
git diff --stat mmotm/master include/linux/memblock.h mm/memblock.c
include/linux/memblock.h | 59 ++-
mm/memblock.c| 249 ---
2 files changed, 90 insertions(+), 218 deletions(-)
> For the normal memory allocator, the trend is to remove printing of errors
> from all callers, as the core takes care of that.
It's more about allocation errors handling than printing of the errors.
Indeed, there is not much that can be done if an early allocation fails,
but I believe having an explicit pattern
ptr = alloc();
if (!ptr)
do_something_about_it();
is clearer than relying on the allocator to panic().
Besides, the diversity of panic and nopanic variants creates a confusion
and I've caught several places that call nopanic variant and do not check
its return value.
> > --- a/arch/alpha/kernel/core_marvel.c
> > +++ b/arch/alpha/kernel/core_marvel.c
> > @@ -83,6 +83,9 @@ mk_resource_name(int pe, int port, char *str)
> >
> > sprintf(tmp, "PCI %s PE %d PORT %d", str, pe, port);
> > name = memblock_alloc(strlen(tmp) + 1, SMP_CACHE_BYTES);
> > + if (!name)
> > + panic("%s: Failed to allocate %lu bytes\n", __func__,
>
> %zu, as strlen() returns size_t.
Thanks for spotting it, will fix.
> > + strlen(tmp) + 1);
> > strcpy(name, tmp);
> >
> > return name;
> > @@ -118,6 +121,9 @@ alloc_io7(unsigned int pe)
> > }
> >
> > io7 = memblock_alloc(sizeof(*io7), SMP_CACHE_BYTES);
> > + if (!io7)
> > + panic("%s: Failed to allocate %lu bytes\n", __func__,
>
> %zu, as sizeof() returns size_t.
> Probably there are more. Yes, it's hard to get them right in all callers.
Yeah :)
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
>
--
Sincerely yours,
Mike.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*()
Hi Mike,
On Wed, Jan 16, 2019 at 2:46 PM 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__,
In general, you want to use %zu for size_t
> size, align);
>
> Signed-off-by: Mike Rapoport
Thanks for your patch!
> 74 files changed, 415 insertions(+), 29 deletions(-)
I'm wondering if this is really an improvement?
For the normal memory allocator, the trend is to remove printing of errors
from all callers, as the core takes care of that.
> --- a/arch/alpha/kernel/core_marvel.c
> +++ b/arch/alpha/kernel/core_marvel.c
> @@ -83,6 +83,9 @@ mk_resource_name(int pe, int port, char *str)
>
> sprintf(tmp, "PCI %s PE %d PORT %d", str, pe, port);
> name = memblock_alloc(strlen(tmp) + 1, SMP_CACHE_BYTES);
> + if (!name)
> + panic("%s: Failed to allocate %lu bytes\n", __func__,
%zu, as strlen() returns size_t.
> + strlen(tmp) + 1);
> strcpy(name, tmp);
>
> return name;
> @@ -118,6 +121,9 @@ alloc_io7(unsigned int pe)
> }
>
> io7 = memblock_alloc(sizeof(*io7), SMP_CACHE_BYTES);
> + if (!io7)
> + panic("%s: Failed to allocate %lu bytes\n", __func__,
%zu, as sizeof() returns size_t.
Probably there are more. Yes, it's hard to get them right in all callers.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH 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
---
arch/alpha/kernel/core_cia.c | 3 +++
arch/alpha/kernel/core_marvel.c | 6 ++
arch/alpha/kernel/pci-noop.c | 11 ++-
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 | 9 ++---
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/s390/numa/toptree.c | 8 ++--
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 +--
74 files changed, 415 insertions(+), 29 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 @@ cia_prepare_tbia_workaround(int window)
/* Use minimal 1K map. */
ppte =
