sanitize_e820_map returns two possible values:
-1: Returned when either the provided memory map has length 1 (ok) or
when the provided memory map is invalid (not ok).
0: Returned when the memory map was correctly sanitized.
In addition, most code ignores the returned value, and none actually
handles it (except possibly by panicking).
This patch changes the behavior so that sanitize_e820_map is a void
function. When the provided memory map has length 1 or it is sanitized
(both ok cases), it returns nothing. If the provided memory map is
invalid, then it panics.
Signed-off-by: Martin Kelly <[email protected]>
---
Changes in v2:
- Fixed compiler warnings:
* return 0 from void function
* early_panic declared but not used
---
---
arch/x86/include/asm/e820.h | 2 +-
arch/x86/kernel/e820.c | 103 ++++++++++++++++++++------------------------
2 files changed, 47 insertions(+), 58 deletions(-)
diff --git a/arch/x86/include/asm/e820.h b/arch/x86/include/asm/e820.h
index 779c2ef..739f8db 100644
--- a/arch/x86/include/asm/e820.h
+++ b/arch/x86/include/asm/e820.h
@@ -18,7 +18,7 @@ extern int e820_any_mapped(u64 start, u64 end, unsigned type);
extern int e820_all_mapped(u64 start, u64 end, unsigned type);
extern void e820_add_region(u64 start, u64 size, int type);
extern void e820_print_map(char *who);
-extern int
+extern void
sanitize_e820_map(struct e820entry *biosmap, int max_nr_map, u32 *pnr_map);
extern u64 e820_update_range(u64 start, u64 size, unsigned old_type,
unsigned new_type);
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 49f8864..7ca23fb 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -188,47 +188,48 @@ void __init e820_print_map(char *who)
* be updated on return, with the new number of valid entries
* (something no more than max_nr_map.)
*
- * The return value from sanitize_e820_map() is zero if it
- * successfully 'sanitized' the map entries passed in, and is -1
- * if it did nothing, which can happen if either of (1) it was
- * only passed one map entry, or (2) any of the input map entries
- * were invalid (start + size < start, meaning that the size was
- * so big the described memory range wrapped around through zero.)
+ * There are three possible actions that sanitize_e820_map() can take:
+ * (1) If the map entry count is 1, do nothing and return.
+ * (2) If any of the input map entries were invalid
+ * (start + size < start), then the size was so big that the described
+ * memory range wrapped around through zero. In this case, panic.
+ * (3) If the map entry count is greater than 1 and the map is valid,
+ * sanitize the map and return.
*
- * Visually we're performing the following
- * (1,2,3,4 = memory types)...
+ * Visually we're performing the following
+ * (1,2,3,4 = memory types)...
*
- * Sample memory map (w/overlaps):
- * ____22__________________
- * ______________________4_
- * ____1111________________
- * _44_____________________
- * 11111111________________
- * ____________________33__
- * ___________44___________
- * __________33333_________
- * ______________22________
- * ___________________2222_
- * _________111111111______
- * _____________________11_
- * _________________4______
+ * Sample memory map (w/overlaps):
+ * ____22__________________
+ * ______________________4_
+ * ____1111________________
+ * _44_____________________
+ * 11111111________________
+ * ____________________33__
+ * ___________44___________
+ * __________33333_________
+ * ______________22________
+ * ___________________2222_
+ * _________111111111______
+ * _____________________11_
+ * _________________4______
*
- * Sanitized equivalent (no overlap):
- * 1_______________________
- * _44_____________________
- * ___1____________________
- * ____22__________________
- * ______11________________
- * _________1______________
- * __________3_____________
- * ___________44___________
- * _____________33_________
- * _______________2________
- * ________________1_______
- * _________________4______
- * ___________________2____
- * ____________________33__
- * ______________________4_
+ * Sanitized equivalent (no overlap):
+ * 1_______________________
+ * _44_____________________
+ * ___1____________________
+ * ____22__________________
+ * ______11________________
+ * _________1______________
+ * __________3_____________
+ * ___________44___________
+ * _____________33_________
+ * _______________2________
+ * ________________1_______
+ * _________________4______
+ * ___________________2____
+ * ____________________33__
+ * ______________________4_
*/
struct change_member {
struct e820entry *pbios; /* pointer to original bios entry */
@@ -252,7 +253,7 @@ static int __init cpcompare(const void *a, const void *b)
return (ap->addr != ap->pbios->addr) - (bp->addr != bp->pbios->addr);
}
-int __init sanitize_e820_map(struct e820entry *biosmap, int max_nr_map,
+void __init sanitize_e820_map(struct e820entry *biosmap, int max_nr_map,
u32 *pnr_map)
{
static struct change_member change_point_list[2*E820_X_MAX] __initdata;
@@ -269,15 +270,14 @@ int __init sanitize_e820_map(struct e820entry *biosmap, int max_nr_map,
/* if there's only one memory region, don't bother */
if (*pnr_map < 2)
- return -1;
+ return;
old_nr = *pnr_map;
BUG_ON(old_nr > max_nr_map);
- /* bail out if we find any unreasonable addresses in bios map */
+ /* panic if we find any unreasonable addresses in bios map */
for (i = 0; i < old_nr; i++)
- if (biosmap[i].addr + biosmap[i].size < biosmap[i].addr)
- return -1;
+ BUG_ON(biosmap[i].addr + biosmap[i].size < biosmap[i].addr);
/* create pointers for initial change-point information (for sorting) */
for (i = 0; i < 2 * old_nr; i++)
@@ -374,8 +374,6 @@ int __init sanitize_e820_map(struct e820entry *biosmap, int max_nr_map,
/* copy new bios mapping into original location */
memcpy(biosmap, new_bios, new_nr * sizeof(struct e820entry));
*pnr_map = new_nr;
-
- return 0;
}
static int __init __append_e820_map(struct e820entry *biosmap, int nr_map)
@@ -564,8 +562,7 @@ void __init update_e820(void)
u32 nr_map;
nr_map = e820.nr_map;
- if (sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr_map))
- return;
+ sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr_map);
e820.nr_map = nr_map;
printk(KERN_INFO "e820: modified physical RAM map:\n");
e820_print_map("modified");
@@ -575,8 +572,7 @@ static void __init update_e820_saved(void)
u32 nr_map;
nr_map = e820_saved.nr_map;
- if (sanitize_e820_map(e820_saved.map, ARRAY_SIZE(e820_saved.map), &nr_map))
- return;
+ sanitize_e820_map(e820_saved.map, ARRAY_SIZE(e820_saved.map), &nr_map);
e820_saved.nr_map = nr_map;
}
#define MAX_GAP_END 0x100000000ull
@@ -800,12 +796,6 @@ unsigned long __init e820_end_of_low_ram_pfn(void)
return e820_end_pfn(1UL<<(32 - PAGE_SHIFT), E820_RAM);
}
-static void early_panic(char *msg)
-{
- early_printk(msg);
- panic(msg);
-}
-
static int userdef __initdata;
/* "mem=nopentium" disables the 4MB page tables. */
@@ -900,8 +890,7 @@ void __init finish_e820_parsing(void)
if (userdef) {
u32 nr = e820.nr_map;
- if (sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr) < 0)
- early_panic("Invalid user supplied memory map");
+ sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr);
e820.nr_map = nr;
printk(KERN_INFO "e820: user-defined physical RAM map:\n");
--
2.1.1
On Mon, 20 Oct 2014, Martin Kelly wrote:
> sanitize_e820_map returns two possible values:
> -1: Returned when either the provided memory map has length 1 (ok) or
> when the provided memory map is invalid (not ok).
> 0: Returned when the memory map was correctly sanitized.
>
> In addition, most code ignores the returned value, and none actually
> handles it (except possibly by panicking).
There are reasons WHY some code does ignore it.
> This patch changes the behavior so that sanitize_e820_map is a void
> function. When the provided memory map has length 1 or it is sanitized
> (both ok cases), it returns nothing. If the provided memory map is
> invalid, then it panics.
So you break wilfully default_machine_specific_memory_setup() and
probably some other places. Are you sure about that?
Thanks,
tglx
On 10/21/2014 01:56 AM, Thomas Gleixner wrote:
>> This patch changes the behavior so that sanitize_e820_map is a void
>> function. When the provided memory map has length 1 or it is sanitized
>> (both ok cases), it returns nothing. If the provided memory map is
>> invalid, then it panics.
>
> So you break wilfully default_machine_specific_memory_setup() and
> probably some other places. Are you sure about that?
>
I was concerned about exactly that kind of breakage, so my first patch
merely separated out the return values and added some appropriate error
checking:
https://lkml.org/lkml/2014/10/13/514
I then asked whether there are valid cases for ignoring an invalid map
and continuing on, but I didn't receive a reply, so I took my best
guess. It appears I missed some fallback code
(default_machine_specific_memory_setup). That said, most cases don't
appear to have fallback code and will hit issues later on if the BIOS
map is invalid (e.g. Xen).
Thomas, do you see any issues with a revision that separates out the
return values (0 for a map with 1 entry, -1 for a map with invalid
entries) and adds checks in the callers, where appropriate?
Thanks,
Martin
On Tue, 21 Oct 2014, Martin Kelly wrote:
> On 10/21/2014 01:56 AM, Thomas Gleixner wrote:
> >> This patch changes the behavior so that sanitize_e820_map is a void
> >> function. When the provided memory map has length 1 or it is sanitized
> >> (both ok cases), it returns nothing. If the provided memory map is
> >> invalid, then it panics.
> >
> > So you break wilfully default_machine_specific_memory_setup() and
> > probably some other places. Are you sure about that?
> >
> I was concerned about exactly that kind of breakage, so my first patch
> merely separated out the return values and added some appropriate error
> checking:
> https://lkml.org/lkml/2014/10/13/514
>
> I then asked whether there are valid cases for ignoring an invalid map
> and continuing on, but I didn't receive a reply, so I took my best
> guess. It appears I missed some fallback code
> (default_machine_specific_memory_setup). That said, most cases don't
We probably could and should also spend some time on investigating the
validity of that fallback stuff. AFAICT, this has some rather obscure
history from voyager, but I had no time to do more archeology on that.
> appear to have fallback code and will hit issues later on if the BIOS
> map is invalid (e.g. Xen).
Right.
> Thomas, do you see any issues with a revision that separates out the
> return values (0 for a map with 1 entry, -1 for a map with invalid
> entries) and adds checks in the callers, where appropriate?
I think that's a sane approach.
Thanks,
tglx
On 10/21/2014 12:44 PM, Thomas Gleixner wrote:
> On Tue, 21 Oct 2014, Martin Kelly wrote:
>> On 10/21/2014 01:56 AM, Thomas Gleixner wrote:
>> Thomas, do you see any issues with a revision that separates out the
>> return values (0 for a map with 1 entry, -1 for a map with invalid
>> entries) and adds checks in the callers, where appropriate?
>
> I think that's a sane approach.
>
Alright, I will work on a new patch doing that, separating out each call
site (and relevant error handling) into its own patch.