Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CADDKRnANovPM5Xvme7Ywg8KEMUyP-gB0M-ufxKD8pw0gNwtFag@mail.gmail.com>
 <CAHk-=wjOXE4cqFOdtSymYnMMayZq8Lv7qDy-6BzCs=2=8HcoBA@mail.gmail.com>
 <20200131064327.GB130017@gmail.com> <CADDKRnATVt9JjgV+dAZDH9C3=goJ5=TzdZ8EJMjT8tKP+Uhezw@mail.gmail.com>
 <20200131183658.GA71555@gmail.com> <20200201094545.GC71555@gmail.com>
In-Reply-To: <20200201094545.GC71555@gmail.com>
From:   Dan Williams <dan.j.williams@intel.com>
Date:   Sat, 1 Feb 2020 08:23:42 -0800
Message-ID: <CAPcyv4iH6BA4Oo7ZedWMG_28cp1KauQET58qOpR7012aaHWzbQ@mail.gmail.com>
Subject: Re: [TEST PATCH RFC] Revert the EFI leak fixes for now (was: Re: EFI
 boot crash regression (was: Re: 5.6-### doesn't boot))
To:     Ingo Molnar <mingo@kernel.org>
Cc:     =?UTF-8?Q?J=C3=B6rg_Otte?= <jrg.otte@gmail.com>,
        Ard Biesheuvel <ardb@kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "the arch/x86 maintainers" <x86@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Feb 1, 2020 at 1:45 AM Ingo Molnar <mingo@kernel.org> wrote:
>
>
>
> J=C3=B6rg 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:4=
8:
> > /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=
=3D 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=C3=B6rg, 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 =3D efi.memmap.desc_version,
                        .desc_size =3D efi.memmap.desc_size,
                        .size =3D data.desc_size * (efi.memmap.nr_map -
n_removal),
-                       .flags =3D 0,
+                       .flags =3D efi.memmap.flags & EFI_MEMMAP_LATE,
                };

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

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