[PATCH v2 0/2] x86: Check return values for early memory/IO remap calls

[PATCH v2 0/2] x86: Check return values for early memory/IO remap calls

[Ross Philipson]

While sending an earlier patch set it was discovered that there are a
number of places in early x86 code were the functions early_memremap()
and early_ioremap() are called but the returned pointer is not checked
for NULL. Since NULL can be returned for a couple of reasons, the return
value should be checked for NULL.

This set fixes the places where the checks were missing. It was not always
clear what the best failure mode should be when NULL is detected. In modules
where other places tended to pr_warn or panic e.g., the same was done for
the checks. In other places it was based on how significantly fatal the
failure would end up being. The review process may point out places where
this should be changed.

Changes in v2:
 - Added notes in comments about why panic() was used in some cases and
the fact that maintainers approved the usage.
 - Added pr_fmt macros in changed files to allow proper usage of pr_*
printing macros.

Ross Philipson (2):
  x86: Check return values from early_memremap calls
  x86: Check return values from early_ioremap calls

 arch/x86/kernel/apic/x2apic_uv_x.c |  2 ++
 arch/x86/kernel/devicetree.c       | 13 ++++++++++
 arch/x86/kernel/e820.c             | 12 +++++++--
 arch/x86/kernel/early_printk.c     |  2 ++
 arch/x86/kernel/jailhouse.c        |  6 +++++
 arch/x86/kernel/mpparse.c          | 51 ++++++++++++++++++++++++++++----------
 arch/x86/kernel/setup.c            | 19 +++++++++++---
 arch/x86/kernel/vsmp_64.c          |  3 +++
 arch/x86/xen/enlighten_hvm.c       |  2 ++
 arch/x86/xen/mmu_pv.c              |  8 ++++++
 arch/x86/xen/setup.c               |  2 ++
 11 files changed, 102 insertions(+), 18 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/2] x86: Check return values from early_memremap calls

[Ross Philipson]

There are a number of places where early_memremap is called
but the return pointer is not checked for NULL. The call
can result in a NULL being returned so the checks must
be added.

Note that the maintainers for both the Jailhouse and Xen code
approved of using panic() to handle allocation failures.

In addition to checking the return values, a bit of extra
cleanup of pr_* usages was done since the pr_fmt macro was
introduced in the modules touched by this patch.

Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
---
 arch/x86/kernel/devicetree.c | 13 +++++++++++
 arch/x86/kernel/e820.c       | 12 +++++++++--
 arch/x86/kernel/jailhouse.c  |  6 ++++++
 arch/x86/kernel/mpparse.c    | 51 +++++++++++++++++++++++++++++++++-----------
 arch/x86/kernel/setup.c      | 19 ++++++++++++++---
 arch/x86/xen/enlighten_hvm.c |  2 ++
 arch/x86/xen/mmu_pv.c        |  8 +++++++
 arch/x86/xen/setup.c         |  2 ++
 8 files changed, 95 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index 5cd51f2..4a5ca9a 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -2,6 +2,9 @@
 /*
  * Architecture specific OF callbacks.
  */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/export.h>
 #include <linux/io.h>
 #include <linux/interrupt.h>
@@ -292,10 +295,20 @@ static void __init x86_flattree_get_config(void)
 	map_len = max(PAGE_SIZE - (initial_dtb & ~PAGE_MASK), (u64)128);
 
 	dt = early_memremap(initial_dtb, map_len);
+	if (!dt) {
+		pr_warn("failed to memremap initial dtb\n");
+		return;
+	}
+
 	size = fdt_totalsize(dt);
 	if (map_len < size) {
 		early_memunmap(dt, map_len);
 		dt = early_memremap(initial_dtb, size);
+		if (!dt) {
+			pr_warn("failed to memremap initial dtb\n");
+			return;
+		}
+
 		map_len = size;
 	}
 
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 9dac246..9cbc724 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -9,6 +9,9 @@
  * quirks and other tweaks, and feeds that into the generic Linux memory
  * allocation code routines via a platform independent interface (memblock, etc.).
  */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/crash_dump.h>
 #include <linux/memblock.h>
 #include <linux/suspend.h>
@@ -728,6 +731,11 @@ void __init e820__memory_setup_extended(u64 phys_addr, u32 data_len)
 	struct setup_data *sdata;
 
 	sdata = early_memremap(phys_addr, data_len);
+	if (!sdata) {
+		pr_warn("failed to memremap extended\n");
+		return;
+	}
+
 	entries = sdata->len / sizeof(*extmap);
 	extmap = (struct boot_e820_entry *)(sdata->data);
 
@@ -1007,7 +1015,7 @@ void __init e820__reserve_setup_data(void)
 	while (pa_data) {
 		data = early_memremap(pa_data, sizeof(*data));
 		if (!data) {
-			pr_warn("e820: failed to memremap setup_data entry\n");
+			pr_warn("failed to memremap setup_data entry\n");
 			return;
 		}
 
@@ -1030,7 +1038,7 @@ void __init e820__reserve_setup_data(void)
 			early_memunmap(data, sizeof(*data));
 			data = early_memremap(pa_data, len);
 			if (!data) {
-				pr_warn("e820: failed to memremap indirect setup_data\n");
+				pr_warn("failed to memremap indirect setup_data\n");
 				return;
 			}
 
diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
index 4eb8f2d..80db0c2 100644
--- a/arch/x86/kernel/jailhouse.c
+++ b/arch/x86/kernel/jailhouse.c
@@ -221,6 +221,9 @@ static void __init jailhouse_init_platform(void)
 
 	while (pa_data) {
 		mapping = early_memremap(pa_data, sizeof(header));
+		if (!mapping)
+			panic("Jailhouse: failed to memremap setup_data header\n");
+
 		memcpy(&header, mapping, sizeof(header));
 		early_memunmap(mapping, sizeof(header));
 
@@ -241,6 +244,9 @@ static void __init jailhouse_init_platform(void)
 	setup_data_len = min_t(unsigned long, sizeof(setup_data),
 			       (unsigned long)header.len);
 	mapping = early_memremap(pa_data, setup_data_len);
+	if (!mapping)
+		panic("Jailhouse: failed to memremap setup_data\n");
+
 	memcpy(&setup_data, mapping, setup_data_len);
 	early_memunmap(mapping, setup_data_len);
 
diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
index fed721f..4254163 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -8,6 +8,8 @@
  *      (c) 2008 Alexey Starikovskiy <astarikovskiy@suse.de>
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/mm.h>
 #include <linux/init.h>
 #include <linux/delay.h>
@@ -145,33 +147,33 @@ static int __init smp_check_mpc(struct mpc_table *mpc, char *oem, char *str)
 {
 
 	if (memcmp(mpc->signature, MPC_SIGNATURE, 4)) {
-		pr_err("MPTABLE: bad signature [%c%c%c%c]!\n",
+		pr_err("bad signature [%c%c%c%c]!\n",
 		       mpc->signature[0], mpc->signature[1],
 		       mpc->signature[2], mpc->signature[3]);
 		return 0;
 	}
 	if (mpf_checksum((unsigned char *)mpc, mpc->length)) {
-		pr_err("MPTABLE: checksum error!\n");
+		pr_err("checksum error!\n");
 		return 0;
 	}
 	if (mpc->spec != 0x01 && mpc->spec != 0x04) {
-		pr_err("MPTABLE: bad table version (%d)!!\n", mpc->spec);
+		pr_err("bad table version (%d)!!\n", mpc->spec);
 		return 0;
 	}
 	if (!mpc->lapic) {
-		pr_err("MPTABLE: null local APIC address!\n");
+		pr_err("null local APIC address!\n");
 		return 0;
 	}
 	memcpy(oem, mpc->oem, 8);
 	oem[8] = 0;
-	pr_info("MPTABLE: OEM ID: %s\n", oem);
+	pr_info("OEM ID: %s\n", oem);
 
 	memcpy(str, mpc->productid, 12);
 	str[12] = 0;
 
-	pr_info("MPTABLE: Product ID: %s\n", str);
+	pr_info("Product ID: %s\n", str);
 
-	pr_info("MPTABLE: APIC at: 0x%X\n", mpc->lapic);
+	pr_info("APIC at: 0x%X\n", mpc->lapic);
 
 	return 1;
 }
@@ -242,7 +244,7 @@ static int __init smp_read_mpc(struct mpc_table *mpc, unsigned early)
 	}
 
 	if (!num_processors)
-		pr_err("MPTABLE: no processors registered!\n");
+		pr_err("no processors registered!\n");
 	return num_processors;
 }
 
@@ -424,6 +426,9 @@ static unsigned long __init get_mpc_size(unsigned long physptr)
 	unsigned long size;
 
 	mpc = early_memremap(physptr, PAGE_SIZE);
+	if (!mpc)
+		return 0;
+
 	size = mpc->length;
 	early_memunmap(mpc, PAGE_SIZE);
 	apic_printk(APIC_VERBOSE, "  mpc: %lx-%lx\n", physptr, physptr + size);
@@ -437,7 +442,16 @@ static int __init check_physptr(struct mpf_intel *mpf, unsigned int early)
 	unsigned long size;
 
 	size = get_mpc_size(mpf->physptr);
+	if (!size) {
+		pr_err("error getting MP table size\n");
+		return -1;
+	}
+
 	mpc = early_memremap(mpf->physptr, size);
+	if (!mpc) {
+		pr_err("error mapping MP table physptr\n");
+		return -1;
+	}
 
 	/*
 	 * Read the physical hardware table.  Anything here will
@@ -505,7 +519,7 @@ void __init default_get_smp_config(unsigned int early)
 
 	mpf = early_memremap(mpf_base, sizeof(*mpf));
 	if (!mpf) {
-		pr_err("MPTABLE: error mapping MP table\n");
+		pr_err("error mapping MP table\n");
 		return;
 	}
 
@@ -552,6 +566,7 @@ void __init default_get_smp_config(unsigned int early)
 
 static void __init smp_reserve_memory(struct mpf_intel *mpf)
 {
+	/* If get_mpc_size() is 0, memblock_reserve() will just do nothing */
 	memblock_reserve(mpf->physptr, get_mpc_size(mpf->physptr));
 }
 
@@ -567,6 +582,11 @@ static int __init smp_scan_config(unsigned long base, unsigned long length)
 
 	while (length > 0) {
 		bp = early_memremap(base, length);
+		if (!bp) {
+			pr_err("error mapping SMP config\n");
+			return 0;
+		}
+
 		mpf = (struct mpf_intel *)bp;
 		if ((*bp == SMP_MAGIC_IDENT) &&
 		    (mpf->length == 1) &&
@@ -850,7 +870,7 @@ static int __init update_mp_table(void)
 
 	mpf = early_memremap(mpf_base, sizeof(*mpf));
 	if (!mpf) {
-		pr_err("MPTABLE: mpf early_memremap() failed\n");
+		pr_err("mpf early_memremap() failed\n");
 		return 0;
 	}
 
@@ -864,9 +884,14 @@ static int __init update_mp_table(void)
 		goto do_unmap_mpf;
 
 	size = get_mpc_size(mpf->physptr);
+	if (!size) {
+		pr_err("error getting MP table size\n");
+		goto do_unmap_mpf;
+	}
+
 	mpc = early_memremap(mpf->physptr, size);
 	if (!mpc) {
-		pr_err("MPTABLE: mpc early_memremap() failed\n");
+		pr_err("mpc early_memremap() failed\n");
 		goto do_unmap_mpf;
 	}
 
@@ -897,7 +922,7 @@ static int __init update_mp_table(void)
 	} else {
 		mpc_new = early_memremap(mpc_new_phys, mpc_new_length);
 		if (!mpc_new) {
-			pr_err("MPTABLE: new mpc early_memremap() failed\n");
+			pr_err("new mpc early_memremap() failed\n");
 			goto do_unmap_mpc;
 		}
 		mpf->physptr = mpc_new_phys;
@@ -911,7 +936,7 @@ static int __init update_mp_table(void)
 			/* steal 16 bytes from [0, 1k) */
 			mpf_new = early_memremap(0x400 - 16, sizeof(*mpf_new));
 			if (!mpf_new) {
-				pr_err("MPTABLE: new mpf early_memremap() failed\n");
+				pr_err("new mpf early_memremap() failed\n");
 				goto do_unmap_mpc;
 			}
 			pr_info("mpf new: %x\n", 0x400 - 16);
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 216fee7..621fc3e 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -5,6 +5,9 @@
  * This file contains the setup_arch() code, which handles the architecture-dependent
  * parts of early kernel initialization.
  */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/acpi.h>
 #include <linux/console.h>
 #include <linux/crash_dump.h>
@@ -344,7 +347,7 @@ static void __init add_early_ima_buffer(u64 phys_addr)
 
 	data = early_memremap(phys_addr + sizeof(struct setup_data), sizeof(*data));
 	if (!data) {
-		pr_warn("setup: failed to memremap ima_setup_data entry\n");
+		pr_warn("failed to memremap ima_setup_data entry\n");
 		return;
 	}
 
@@ -401,6 +404,11 @@ static void __init parse_setup_data(void)
 		u32 data_len, data_type;
 
 		data = early_memremap(pa_data, sizeof(*data));
+		if (!data) {
+			pr_warn("failed to memremap in parse_setup_data\n");
+			return;
+		}
+
 		data_len = data->len + sizeof(struct setup_data);
 		data_type = data->type;
 		pa_next = data->next;
@@ -421,6 +429,11 @@ static void __init parse_setup_data(void)
 			break;
 		case SETUP_RNG_SEED:
 			data = early_memremap(pa_data, data_len);
+			if (!data) {
+				pr_warn("failed to memremap RNG seed data\n");
+				return;
+			}
+
 			add_bootloader_randomness(data->data, data->len);
 			/* Zero seed for forward secrecy. */
 			memzero_explicit(data->data, data->len);
@@ -446,7 +459,7 @@ static void __init memblock_x86_reserve_range_setup_data(void)
 	while (pa_data) {
 		data = early_memremap(pa_data, sizeof(*data));
 		if (!data) {
-			pr_warn("setup: failed to memremap setup_data entry\n");
+			pr_warn("failed to memremap setup_data entry\n");
 			return;
 		}
 
@@ -460,7 +473,7 @@ static void __init memblock_x86_reserve_range_setup_data(void)
 			early_memunmap(data, sizeof(*data));
 			data = early_memremap(pa_data, len);
 			if (!data) {
-				pr_warn("setup: failed to memremap indirect setup_data\n");
+				pr_warn("failed to memremap indirect setup_data\n");
 				return;
 			}
 
diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index c1cd28e..2135bfe 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -70,6 +70,8 @@ static void __init reserve_shared_info(void)
 
 	memblock_reserve(pa, PAGE_SIZE);
 	HYPERVISOR_shared_info = early_memremap(pa, PAGE_SIZE);
+	if (!HYPERVISOR_shared_info)
+		panic("xen: failed to memmap hypervisor shared page: 0x%llx\n", pa);
 }
 
 static void __init xen_hvm_init_mem_mapping(void)
diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index ee29fb5..b164d8f 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -1824,6 +1824,8 @@ static unsigned long __init xen_read_phys_ulong(phys_addr_t addr)
 	unsigned long val;
 
 	vaddr = early_memremap_ro(addr, sizeof(val));
+	if (!vaddr)
+		panic("xen: failed to memmap physical address: 0x%llx\n", addr);
 	val = *vaddr;
 	early_memunmap(vaddr, sizeof(val));
 	return val;
@@ -1919,14 +1921,20 @@ void __init xen_relocate_p2m(void)
 	new_p2m = (unsigned long *)(2 * PGDIR_SIZE);
 	for (idx_pud = 0; idx_pud < n_pud; idx_pud++) {
 		pud = early_memremap(pud_phys, PAGE_SIZE);
+		if (!pud)
+			panic("xen: failed to memmap PUD physical address: 0x%llx\n", pud_phys);
 		clear_page(pud);
 		for (idx_pmd = 0; idx_pmd < min(n_pmd, PTRS_PER_PUD);
 				idx_pmd++) {
 			pmd = early_memremap(pmd_phys, PAGE_SIZE);
+			if (!pmd)
+				panic("xen: failed to memmap PMD physical address: 0x%llx\n", pmd_phys);
 			clear_page(pmd);
 			for (idx_pt = 0; idx_pt < min(n_pt, PTRS_PER_PMD);
 					idx_pt++) {
 				pt = early_memremap(pt_phys, PAGE_SIZE);
+				if (!pt)
+					panic("xen: failed to memmap PT physical address: 0x%llx\n", pt_phys);
 				clear_page(pt);
 				for (idx_pte = 0;
 				     idx_pte < min(n_pte, PTRS_PER_PTE);
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 4f43095..2f3cf6c 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -685,6 +685,8 @@ static void __init xen_phys_memcpy(phys_addr_t dest, phys_addr_t src,
 		len = min(dest_len, src_len);
 		to = early_memremap(dest - dest_off, dest_len + dest_off);
 		from = early_memremap(src - src_off, src_len + src_off);
+		if (!to || !from)
+			panic("xen: failed to memmap for physical address memcpy\n");
 		memcpy(to, from, len);
 		early_memunmap(to, dest_len + dest_off);
 		early_memunmap(from, src_len + src_off);
-- 
1.8.3.1

[PATCH v2 2/2] x86: Check return values from early_ioremap calls

[Ross Philipson]

There are a number of places where early_ioremap is called
but the return pointer is not checked for NULL. The call
can result in a NULL being returned so the checks must
be added.

On allocation failures, panic() was used since this seemed
to be the action taken on other failures in the modules
touched by this patch.

Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
---
 arch/x86/kernel/apic/x2apic_uv_x.c | 2 ++
 arch/x86/kernel/early_printk.c     | 2 ++
 arch/x86/kernel/vsmp_64.c          | 3 +++
 3 files changed, 7 insertions(+)

diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index 4828552..4ffdc27 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -75,6 +75,8 @@ static unsigned long __init uv_early_read_mmr(unsigned long addr)
 	unsigned long val, *mmr;
 
 	mmr = early_ioremap(UV_LOCAL_MMR_BASE | addr, sizeof(*mmr));
+	if (!mmr)
+		panic("UV: error: failed to ioremap MMR\n");
 	val = *mmr;
 	early_iounmap(mmr, sizeof(*mmr));
 
diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index 44f9370..1fe590d 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -290,6 +290,8 @@ static __init void early_pci_serial_init(char *s)
 		/* WARNING! assuming the address is always in the first 4G */
 		early_serial_base =
 			(unsigned long)early_ioremap(bar0 & PCI_BASE_ADDRESS_MEM_MASK, 0x10);
+		if (!early_serial_base)
+			panic("early_serial: failed to ioremap MMIO BAR\n");
 		write_pci_config(bus, slot, func, PCI_COMMAND,
 				 cmdreg|PCI_COMMAND_MEMORY);
 	}
diff --git a/arch/x86/kernel/vsmp_64.c b/arch/x86/kernel/vsmp_64.c
index 796cfaa..39769f4 100644
--- a/arch/x86/kernel/vsmp_64.c
+++ b/arch/x86/kernel/vsmp_64.c
@@ -32,6 +32,9 @@ static void __init set_vsmp_ctl(void)
 	/* set vSMP magic bits to indicate vSMP capable kernel */
 	cfg = read_pci_config(0, 0x1f, 0, PCI_BASE_ADDRESS_0);
 	address = early_ioremap(cfg, 8);
+	if (WARN_ON(!address))
+		return;
+
 	cap = readl(address);
 	ctl = readl(address + 4);
 	printk(KERN_INFO "vSMP CTL: capabilities:0x%08x  control:0x%08x\n",
-- 
1.8.3.1

Re: [PATCH v2 1/2] x86: Check return values from early_memremap calls

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1177 bytes --]

On 10.11.22 16:45, Ross Philipson wrote:
> There are a number of places where early_memremap is called
> but the return pointer is not checked for NULL. The call
> can result in a NULL being returned so the checks must
> be added.
> 
> Note that the maintainers for both the Jailhouse and Xen code
> approved of using panic() to handle allocation failures.
> 
> In addition to checking the return values, a bit of extra
> cleanup of pr_* usages was done since the pr_fmt macro was
> introduced in the modules touched by this patch.
> 
> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> ---
>   arch/x86/kernel/devicetree.c | 13 +++++++++++
>   arch/x86/kernel/e820.c       | 12 +++++++++--
>   arch/x86/kernel/jailhouse.c  |  6 ++++++
>   arch/x86/kernel/mpparse.c    | 51 +++++++++++++++++++++++++++++++++-----------
>   arch/x86/kernel/setup.c      | 19 ++++++++++++++---
>   arch/x86/xen/enlighten_hvm.c |  2 ++
>   arch/x86/xen/mmu_pv.c        |  8 +++++++
>   arch/x86/xen/setup.c         |  2 ++
>   8 files changed, 95 insertions(+), 18 deletions(-)

For the Xen parts:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 1/2] x86: Check return values from early_memremap calls

[Dave Hansen]

On 11/10/22 07:45, Ross Philipson wrote:
>  	dt = early_memremap(initial_dtb, map_len);
> +	if (!dt) {
> +		pr_warn("failed to memremap initial dtb\n");
> +		return;
> +	}

Are all of these new pr_warn/err()'s really adding much value?  They all
look pretty generic.  It makes me wonder if we should just spit out a
generic message in early_memremap() and save all the callers the trouble.

Oh, and don't we try to refer to functions() with parenthesis?

Re: [PATCH v2 2/2] x86: Check return values from early_ioremap calls

[Peter Zijlstra]

On Thu, Nov 10, 2022 at 03:45:21PM +0000, Ross Philipson wrote:
> On allocation failures, panic() was used since this seemed
> to be the action taken on other failures in the modules
> touched by this patch.

How is the panic() more useful than the obvious NULL deref that also
splats?

Re: [PATCH v2 1/2] x86: Check return values from early_memremap calls

[Ross Philipson]

On 11/10/22 11:07, Dave Hansen wrote:
> On 11/10/22 07:45, Ross Philipson wrote:
>>   	dt = early_memremap(initial_dtb, map_len);
>> +	if (!dt) {
>> +		pr_warn("failed to memremap initial dtb\n");
>> +		return;
>> +	}
> 
> Are all of these new pr_warn/err()'s really adding much value?  They all
> look pretty generic.  It makes me wonder if we should just spit out a
> generic message in early_memremap() and save all the callers the trouble.

These changes were prompted by some comments on an earlier patch set I 
sent. It was requested that I fix the other missing checks for NULL 
returns from these functions but I thought that was out of scope for 
that patch set. So I agreed to submit this set and add the checks making 
things consistent.

> 
> Oh, and don't we try to refer to functions() with parenthesis?

Yes I can fix that.

Thanks
Ross

> 

Re: [PATCH v2 2/2] x86: Check return values from early_ioremap calls

[Ross Philipson]

On 11/10/22 13:07, Peter Zijlstra wrote:
> On Thu, Nov 10, 2022 at 03:45:21PM +0000, Ross Philipson wrote:
>> On allocation failures, panic() was used since this seemed
>> to be the action taken on other failures in the modules
>> touched by this patch.
> 
> How is the panic() more useful than the obvious NULL deref that also
> splats?
> 

My answer here is basically the same as the answer in the reply to Dave 
Hansen I sent a moment ago. I think one of the primary motivation was to 
make things consistent.

Thanks
Ross

Re: [PATCH v2 0/2] x86: Check return values for early memory/IO remap calls

[Ross Philipson]

On 11/10/22 10:45, Ross Philipson wrote:
> While sending an earlier patch set it was discovered that there are a
> number of places in early x86 code were the functions early_memremap()
> and early_ioremap() are called but the returned pointer is not checked
> for NULL. Since NULL can be returned for a couple of reasons, the return
> value should be checked for NULL.
> 
> This set fixes the places where the checks were missing. It was not always
> clear what the best failure mode should be when NULL is detected. In modules
> where other places tended to pr_warn or panic e.g., the same was done for
> the checks. In other places it was based on how significantly fatal the
> failure would end up being. The review process may point out places where
> this should be changed.

Borislav,

I just wanted to get your thoughts here since it was by your prompting 
that I sent this second patch set to make checking of return values from 
early_memremap() and early_ioremap() consistent. I have gotten 
Reviewed-by's from some of the maintainers in specific areas that they 
approve of the return handling. I also got two replies basically 
questioning the underlying approach. I replied that I basically did what 
you asked me to do. I have not heard back. How would you like me to proceed?

Thanks,

Ross Philipson

> 
> Changes in v2:
>   - Added notes in comments about why panic() was used in some cases and
> the fact that maintainers approved the usage.
>   - Added pr_fmt macros in changed files to allow proper usage of pr_*
> printing macros.
> 
> Ross Philipson (2):
>    x86: Check return values from early_memremap calls
>    x86: Check return values from early_ioremap calls
> 
>   arch/x86/kernel/apic/x2apic_uv_x.c |  2 ++
>   arch/x86/kernel/devicetree.c       | 13 ++++++++++
>   arch/x86/kernel/e820.c             | 12 +++++++--
>   arch/x86/kernel/early_printk.c     |  2 ++
>   arch/x86/kernel/jailhouse.c        |  6 +++++
>   arch/x86/kernel/mpparse.c          | 51 ++++++++++++++++++++++++++++----------
>   arch/x86/kernel/setup.c            | 19 +++++++++++---
>   arch/x86/kernel/vsmp_64.c          |  3 +++
>   arch/x86/xen/enlighten_hvm.c       |  2 ++
>   arch/x86/xen/mmu_pv.c              |  8 ++++++
>   arch/x86/xen/setup.c               |  2 ++
>   11 files changed, 102 insertions(+), 18 deletions(-)
> 

Re: [PATCH v2 1/2] x86: Check return values from early_memremap calls

[Borislav Petkov]

On Thu, Nov 10, 2022 at 08:07:53AM -0800, Dave Hansen wrote:
> On 11/10/22 07:45, Ross Philipson wrote:
> >  	dt = early_memremap(initial_dtb, map_len);
> > +	if (!dt) {
> > +		pr_warn("failed to memremap initial dtb\n");
> > +		return;
> > +	}
> 
> Are all of these new pr_warn/err()'s really adding much value?  They all
> look pretty generic.  It makes me wonder if we should just spit out a
> generic message in early_memremap() and save all the callers the trouble.

Well, let's see.

early_memremap() calls __early_ioremap() and that one already warns befofe each
NULL return. So I guess we don't need the error messages as we will know where
it starts failing.

I guess we still need the error handling though.

I.e., this above should be:

    dt = early_memremap(initial_dtb, map_len);
+   if (!dt)
+           return;

so that we don't go off into the weeds with a NULL ptr.

Or?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette