| 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: <206656f9-917e-4478-b731-ab7e05bc8a74@suse.com>
Date: Mon, 2 Jun 2025 15:50:24 +0300
From: Nikolay Borisov <nik.borisov@...e.com>
To: Ingo Molnar <mingo@...nel.org>, linux-kernel@...r.kernel.org
Cc: Andy Shevchenko <andy@...nel.org>, Arnd Bergmann <arnd@...nel.org>,
Borislav Petkov <bp@...en8.de>, Juergen Gross <jgross@...e.com>,
"H . Peter Anvin" <hpa@...or.com>, Kees Cook <keescook@...omium.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Mike Rapoport <rppt@...nel.org>, Paul Menzel <pmenzel@...gen.mpg.de>,
Peter Zijlstra <peterz@...radead.org>, Thomas Gleixner <tglx@...utronix.de>,
David Woodhouse <dwmw@...zon.co.uk>
Subject: Re: [PATCH 25/32] x86/boot/e820: Simplify append_e820_table() and
remove restriction on single-entry tables
On 5/15/25 15:05, Ingo Molnar wrote:
> So append_e820_table() begins with this weird condition that checks 'nr_entries':
>
> static int __init append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
> {
> /* Only one memory region (or negative)? Ignore it */
> if (nr_entries < 2)
> return -1;
>
> Firstly, 'nr_entries' has been an u32 since 2017 and cannot be negative.
>
> Secondly, there's nothing inherently wrong with single-entry E820 maps,
> especially in virtualized environments.
>
> So remove this restriction and remove the __append_e820_table()
> indirection.
>
> Also:
>
> - fix/update comments
> - remove obsolete comments
>
> This shrinks the generated code a bit as well:
>
> text data bss dec hex filename
> 7549 44072 0 51621 c9a5 e820.o.before
> 7533 44072 0 51605 c995 e820.o.after
>
> Signed-off-by: Ingo Molnar <mingo@...nel.org>
> Cc: Andy Shevchenko <andy@...nel.org>
> Cc: Arnd Bergmann <arnd@...nel.org>
> Cc: David Woodhouse <dwmw@...zon.co.uk>
> Cc: H. Peter Anvin <hpa@...or.com>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Mike Rapoport (Microsoft) <rppt@...nel.org>
> ---
> arch/x86/kernel/e820.c | 35 +++++++++++------------------------
> 1 file changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 447f8bbb77b8..22bfcad7b723 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -483,17 +483,22 @@ __init int e820__update_table(struct e820_table *table)
> return 0;
> }
>
> -__init static int __append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
> +/*
> + * Copy the BIOS E820 map into the kernel's e820_table.
> + *
> + * Sanity-check it while we're at it..
> + */
> +__init static int append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
> {
> struct boot_e820_entry *entry = entries;
>
> while (nr_entries) {
> u64 start = entry->addr;
> - u64 size = entry->size;
> - u64 end = start + size - 1;
> - u32 type = entry->type;
> + u64 size = entry->size;
> + u64 end = start + size-1;
> + u32 type = entry->type;
>
> - /* Ignore the entry on 64-bit overflow: */
> + /* Ignore the remaining entries on 64-bit overflow: */
> if (start > end && likely(size))
> return -1;
nit: All this function wants to do is simply iterate an array, I'd say
the standard way to iterate an array is to use a 'for' loop.
How about doing this instead (on top of this patch):
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 22bfcad7b723..41aa26eae594 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -490,9 +490,9 @@ __init int e820__update_table(struct e820_table *table)
*/
__init static int append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
{
- struct boot_e820_entry *entry = entries;
- while (nr_entries) {
+ for (int i = 0; i < nr_entries; i++) {
+ struct boot_e820_entry *entry = entries + i;
u64 start = entry->addr;
u64 size = entry->size;
u64 end = start + size-1;
@@ -503,9 +503,6 @@ __init static int append_e820_table(struct boot_e820_entry *entries, u32 nr_entr
return -1;
e820__range_add(start, size, type);
-
- entry++;
- nr_entries--;
}
return 0;
}
>
> @@ -505,24 +510,6 @@ __init static int __append_e820_table(struct boot_e820_entry *entries, u32 nr_en
> return 0;
> }
>
> -/*
> - * Copy the BIOS E820 map into a safe place.
> - *
> - * Sanity-check it while we're at it..
> - *
> - * If we're lucky and live on a modern system, the setup code
> - * will have given us a memory map that we can use to properly
> - * set up memory. If we aren't, we'll fake a memory map.
> - */
> -__init static int append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
> -{
> - /* Only one memory region (or negative)? Ignore it */
> - if (nr_entries < 2)
> - return -1;
> -
> - return __append_e820_table(entries, nr_entries);
> -}
> -
> __init static u64
> __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type)
> {
> @@ -796,7 +783,7 @@ __init void e820__memory_setup_extended(u64 phys_addr, u32 data_len)
> entries = sdata->len / sizeof(*extmap);
> extmap = (struct boot_e820_entry *)(sdata->data);
>
> - __append_e820_table(extmap, entries);
> + append_e820_table(extmap, entries);
> e820__update_table(e820_table);
>
> memcpy(e820_table_kexec, e820_table, sizeof(*e820_table_kexec));
Powered by blists - more mailing lists