Re: [TEST PATCH RFC] Revert the EFI leak fixes for now (was: Re: EFI boot crash regression (was: Re: 5.6-### doesn't boot))

[Dan Williams]

On Sat, Feb 1, 2020 at 1:45 AM Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>
>
> JÃrg Otte wrote:
>
> > It's bisected.
> > The first bad commit is :
> > 1db91035d01aa8bfa2350c00ccb63d629b4041ad
> > efi: Add tracking for dynamically allocated memmaps
>
> > Unfortunately I can not revert because of compile errors!
> >
> > In file included from /media/jojo/deftoshiba/kernel/linux/init/main.c:48:
> > /media/jojo/deftoshiba/kernel/linux/include/linux/efi.h:975:1: error:
> > version control conflict marker in file
> > <<<<<<< HEAD
>
> So 1db91035d0 doesn't revert cleanly, because 484a418d0754 depends on it,
> plus there a third commit (f0ef6523475f) that has a semantic dependency
> on 1db91035d01a.
>
> But you can revert them all, if done in reverse chronological order:
>
>   git revert f0ef6523475f # ("efi: Fix efi_memmap_alloc() leaks")
>   git revert 484a418d0754 # ("efi: Fix handling of multiple efi_fake_mem= entries")
>   git revert 1db91035d01a # ("efi: Add tracking for dynamically allocated memmaps")
>
> You can paste those three lines into a shell as-is, or you can apply the
> patch below which is a combination of these three reverts.
>
> JÃrg, can you confirm that doing these reverts fixes booting on your
> system? If it does then a full dmesg from that kernel would be useful.
>
> FWIW I reviewed the bisected commit and didn't find the bug but I also
> couldn't convince myself it's a 1:1 identity transformation and cleanup
> of the existing logic.
>
> The size arithmethics transformation looks correct at first sight, but
> the data->flags handling in particular looks rather interwoven.

Agreed, but the only flags change that I couldn't convince myself was
correct is this:

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 59f7f6d60cf6..314b36ac2a08 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -309,7 +309,7 @@ static void __init efi_clean_memmap(void)
                        .desc_version = efi.memmap.desc_version,
                        .desc_size = efi.memmap.desc_size,
                        .size = data.desc_size * (efi.memmap.nr_map -
n_removal),
-                       .flags = 0,
+                       .flags = efi.memmap.flags & EFI_MEMMAP_LATE,
                };

                pr_warn("Removing %d invalid memory map entries.\n", n_removal);

...but efi_clean_memmap() should "early".