[linux-next PATCH] arm64: fix kernel crash with 48-bit VA and 64KB granule

[Dennis Chen]

On Wed, Jan 06, 2016 at 08:42:20AM +0100, Ard Biesheuvel wrote:
> On 6 January 2016 at 08:38, Dennis Chen <dennis.chen at arm.com> wrote:
> > On Wed, Jan 06, 2016 at 08:13:24AM +0100, Ard Biesheuvel wrote:
> >> On 6 January 2016 at 07:14, Dennis Chen <dennis.chen at arm.com> wrote:
> >> > On Tue, Jan 05, 2016 at 09:56:03AM +0000, Catalin Marinas wrote:
> >> >> On Tue, Jan 05, 2016 at 04:40:44PM +0800, Dennis Chen wrote:
> >> >> > On Tue, Jan 05, 2016 at 09:38:11AM +0100, Ard Biesheuvel wrote:
> >> >> > > >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen at arm.com> wrote:
> >> >> > > >> > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime
> >> >> > > >> > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for
> >> >> > > >> > efi_mm.pgd while not the static efi_pgd[], since this function will be
> >> >> > > >> > called with early_initcall, which results in the pgd_cache used by
> >> >> > > >> > pgd_alloc() has not been initialized yet, kernel will hang in this
> >> >> > > >> > case. This patch is trying to make the pgd_cache_init() called before
> >> >> > > >> > arm_enable_runtime_services() by changing its core_initcall to
> >> >> > > >> > early_initcall.
> >> >> > > >> >
> >> >> > > >> > Signed-off-by: Dennis Chen <dennis.chen at arm.com>
> >> >> > > >> > Tested-by: Sudeep Holla <sudeep.holla at arm.com>
> >> >> > > >> >
> >> >> > > >> > Cc: Will Deacon <will.deacon at arm.com>
> >> >> > > >> > Cc: Catalin Marinas <catalin.marinas at arm.com>
> >> >> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> >> >> > > >> > Cc: Sudeep Holla <sudeep.holla at arm.com>
> >> >> > > >> > ---
> >> >> > > >> >  arch/arm64/mm/pgd.c | 2 +-
> >> >> > > >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >> > > >> >
> >> >> > > >> > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c
> >> >> > > >> > index cb3ba1b..859a788 100644
> >> >> > > >> > --- a/arch/arm64/mm/pgd.c
> >> >> > > >> > +++ b/arch/arm64/mm/pgd.c
> >> >> > > >> > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void)
> >> >> > > >> >                                               SLAB_PANIC, NULL);
> >> >> > > >> >         return 0;
> >> >> > > >> >  }
> >> >> > > >> > -core_initcall(pgd_cache_init);
> >> >> > > >> > +early_initcall(pgd_cache_init);
> >> >> [...]
> >> >> > > Well, since arm_enable_runtime_services() is an early_initcall()
> >> >> > > itself, how are you guaranteeing the ordering between the two? Link
> >> >> > > order?
> >> >> >
> >> >> > Link order.
> >> >>
> >> >> And can you explain how this works, what guarantees it gives?
> >> >>
> >> > You can take a look at include/asm-generic/vmlinux.ldx.h: INIT_CALLS macro,
> >> > for the init call section, early_initcall is the first chuck in the section,
> >> > followed by LEVEL[0-7]. For the same level, the layout order is determined
> >> > by the link order, ARCH is always precedence over the drivers. Catalin, are
> >> > you giving me a kernel examination? :)
> >> >
> >>
> >> We all know how initcalls are implemented. The question is how you are
> >> going to ensure that the early_initcall() that initializes the PGD
> >> cache is always invoked before the early_initcall() that creates the
> >> UEFI runtime page tables.
> >>
> >> And saying that the currently observed link order happens to be
> >> correct is not good enough. We need to be sure that, even if we change
> >> the link order, or switch to LTO at some point, things don't suddenly
> >> break again.
> >>
> > Well, if the build system changes the link order, it can't make sure it will break something
> > unexpectedly, needless to say the pgd_cache_init here at all.
>
> That is no excuse to introduce yet another failure mode.
>
> > Do you think the kernel
> > will change its currently link order policy? If the answer is yes, what's the benefit?
> >
>
> It does not matter what I think. You seem to think that the link order
> is set in stone, so it is you who should argue why that is a
> reasonable assumption.
>
OK, If you think the link order is volatile, how can you guarantee the arm_enable_runtime_services with early_initcall
always work in a volatile link order environment?

> --
> Ard.
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.