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

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

[Dennis Chen]

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@arm.com>
Tested-by: Sudeep Holla <sudeep.holla@arm.com>

Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Sudeep Holla <sudeep.holla@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);
--
1.9.1

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.

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

[Ard Biesheuvel]

On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@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@arm.com>
> Tested-by: Sudeep Holla <sudeep.holla@arm.com>
>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@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);


Hello all,

Apologies for not spotting this before sending it out.

The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did
not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC,
since the series this is part of primarily concerns ARM not arm64.

Anyway, shuffling init ordering is my least favorite way of fixing
bugs. Since only ARM requires pgd_alloc() (for the kernel mappings),
we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and
define it to mean '__get_free_page(PGALLOC_GFP)' on arm64.

-- 
Ard.

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

[Dennis Chen]

On Tue, Jan 05, 2016 at 08:36:39AM +0100, Ard Biesheuvel wrote:
> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@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@arm.com>
> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> >
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Sudeep Holla <sudeep.holla@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);
>
>
> Hello all,
>
> Apologies for not spotting this before sending it out.
>
> The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did
> not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC,
> since the series this is part of primarily concerns ARM not arm64.
>
> Anyway, shuffling init ordering is my least favorite way of fixing
> bugs. Since only ARM requires pgd_alloc() (for the kernel mappings),
> we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and
> define it to mean '__get_free_page(PGALLOC_GFP)' on arm64.
>
>
Ard,

Both arm and arm64 provide pgd_alloc(), I don't think it's a good idea to create
another efi_pgd_alloc() used by efi only, which looks more like a temp workaround.
Plus, for 64KB page granule when VA = 48bit, the PGD_SIZE = 0x200, it doesn't make
sense to get an entire page as the space for the efi.pgd. Changing the init order
sequence is a reasonable way to fix it unless you can see some potential issues there.

> --
> 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.

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

[Ard Biesheuvel]

On 5 January 2016 at 09:35, Dennis Chen <dennis.chen@arm.com> wrote:
> On Tue, Jan 05, 2016 at 08:36:39AM +0100, Ard Biesheuvel wrote:
>> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@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@arm.com>
>> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
>> >
>> > Cc: Will Deacon <will.deacon@arm.com>
>> > Cc: Catalin Marinas <catalin.marinas@arm.com>
>> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > Cc: Sudeep Holla <sudeep.holla@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);
>>
>>
>> Hello all,
>>
>> Apologies for not spotting this before sending it out.
>>
>> The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did
>> not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC,
>> since the series this is part of primarily concerns ARM not arm64.
>>
>> Anyway, shuffling init ordering is my least favorite way of fixing
>> bugs. Since only ARM requires pgd_alloc() (for the kernel mappings),
>> we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and
>> define it to mean '__get_free_page(PGALLOC_GFP)' on arm64.
>>
>>
> Ard,
>
> Both arm and arm64 provide pgd_alloc(), I don't think it's a good idea to create
> another efi_pgd_alloc() used by efi only, which looks more like a temp workaround.
> Plus, for 64KB page granule when VA = 48bit, the PGD_SIZE = 0x200, it doesn't make
> sense to get an entire page as the space for the efi.pgd. Changing the init order
> sequence is a reasonable way to fix it unless you can see some potential issues there.
>

Well, since arm_enable_runtime_services() is an early_initcall()
itself, how are you guaranteeing the ordering between the two? Link
order?

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

[Dennis Chen]

On Tue, Jan 05, 2016 at 09:38:11AM +0100, Ard Biesheuvel wrote:
> On 5 January 2016 at 09:35, Dennis Chen <dennis.chen@arm.com> wrote:
> > On Tue, Jan 05, 2016 at 08:36:39AM +0100, Ard Biesheuvel wrote:
> >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@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@arm.com>
> >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> >> >
> >> > Cc: Will Deacon <will.deacon@arm.com>
> >> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> > Cc: Sudeep Holla <sudeep.holla@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);
> >>
> >>
> >> Hello all,
> >>
> >> Apologies for not spotting this before sending it out.
> >>
> >> The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did
> >> not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC,
> >> since the series this is part of primarily concerns ARM not arm64.
> >>
> >> Anyway, shuffling init ordering is my least favorite way of fixing
> >> bugs. Since only ARM requires pgd_alloc() (for the kernel mappings),
> >> we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and
> >> define it to mean '__get_free_page(PGALLOC_GFP)' on arm64.
> >>
> >>
> > Ard,
> >
> > Both arm and arm64 provide pgd_alloc(), I don't think it's a good idea to create
> > another efi_pgd_alloc() used by efi only, which looks more like a temp workaround.
> > Plus, for 64KB page granule when VA = 48bit, the PGD_SIZE = 0x200, it doesn't make
> > sense to get an entire page as the space for the efi.pgd. Changing the init order
> > sequence is a reasonable way to fix it unless you can see some potential issues there.
> >
>
> Well, since arm_enable_runtime_services() is an early_initcall()
> itself, how are you guaranteeing the ordering between the two? Link
> order?
>

Link order.
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.

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

[Catalin Marinas]

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@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@arm.com>
> > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> > >> >
> > >> > Cc: Will Deacon <will.deacon@arm.com>
> > >> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > >> > Cc: Sudeep Holla <sudeep.holla@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?

> 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.

BTW, please sort out the legal disclaimer (raise an IT ticket).

-- 
Catalin

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

[Dennis Chen]

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@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@arm.com>
> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> > > >> >
> > > >> > Cc: Will Deacon <will.deacon@arm.com>
> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > >> > Cc: Sudeep Holla <sudeep.holla@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? :)

>
> > 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.
>
> BTW, please sort out the legal disclaimer (raise an IT ticket).
>
Hmm, the ticket has been raised.
> --
> Catalin
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
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.

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

[Ard Biesheuvel]

On 6 January 2016 at 07:14, Dennis Chen <dennis.chen@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@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@arm.com>
>> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
>> > > >> >
>> > > >> > Cc: Will Deacon <will.deacon@arm.com>
>> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com>
>> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > > >> > Cc: Sudeep Holla <sudeep.holla@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.

-- 
Ard.

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

[Dennis Chen]

On Wed, Jan 06, 2016 at 08:13:24AM +0100, Ard Biesheuvel wrote:
> On 6 January 2016 at 07:14, Dennis Chen <dennis.chen@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@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@arm.com>
> >> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> >> > > >> >
> >> > > >> > Cc: Will Deacon <will.deacon@arm.com>
> >> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> > > >> > Cc: Sudeep Holla <sudeep.holla@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. Do you think the kernel
will change its currently link order policy? If the answer is yes, what's the benefit?

> --
> 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.

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

[Ard Biesheuvel]

On 6 January 2016 at 08:38, Dennis Chen <dennis.chen@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@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@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@arm.com>
>> >> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
>> >> > > >> >
>> >> > > >> > Cc: Will Deacon <will.deacon@arm.com>
>> >> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com>
>> >> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> > > >> > Cc: Sudeep Holla <sudeep.holla@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.

-- 
Ard.

[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@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@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@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@arm.com>
> >> >> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> >> >> > > >> >
> >> >> > > >> > Cc: Will Deacon <will.deacon@arm.com>
> >> >> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> >> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> >> > > >> > Cc: Sudeep Holla <sudeep.holla@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.

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

[Ard Biesheuvel]

On 6 January 2016 at 09:52, Dennis Chen <dennis.chen@arm.com> wrote:
> On Wed, Jan 06, 2016 at 08:42:20AM +0100, Ard Biesheuvel wrote:
>> On 6 January 2016 at 08:38, Dennis Chen <dennis.chen@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@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@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@arm.com>
>> >> >> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
>> >> >> > > >> >
>> >> >> > > >> > Cc: Will Deacon <will.deacon@arm.com>
>> >> >> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com>
>> >> >> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> >> > > >> > Cc: Sudeep Holla <sudeep.holla@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?
>

By not relying on other early_initcalls

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

[Dennis Chen]

On Wed, Jan 06, 2016 at 09:54:42AM +0100, Ard Biesheuvel wrote:
> On 6 January 2016 at 09:52, Dennis Chen <dennis.chen@arm.com> wrote:
> > On Wed, Jan 06, 2016 at 08:42:20AM +0100, Ard Biesheuvel wrote:
> >> On 6 January 2016 at 08:38, Dennis Chen <dennis.chen@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@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@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@arm.com>
> >> >> >> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> >> >> >> > > >> >
> >> >> >> > > >> > Cc: Will Deacon <will.deacon@arm.com>
> >> >> >> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> >> >> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> >> >> > > >> > Cc: Sudeep Holla <sudeep.holla@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?
> >
>
> By not relying on other early_initcalls
>
You're limiting the scope of the link order
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.

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

[Will Deacon]

Dennis,

On Wed, Jan 06, 2016 at 04:59:30PM +0800, Dennis Chen wrote:
> On Wed, Jan 06, 2016 at 09:54:42AM +0100, Ard Biesheuvel wrote:
> > On 6 January 2016 at 09:52, Dennis Chen <dennis.chen@arm.com> wrote:
> > > On Wed, Jan 06, 2016 at 08:42:20AM +0100, Ard Biesheuvel wrote:
> > >> On 6 January 2016 at 08:38, Dennis Chen <dennis.chen@arm.com> wrote:
> > >> > 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?
> > >
> > 
> > By not relying on other early_initcalls
> > 
> You're limiting the scope of the link order

Please stop this mindless bickering. It's a waste of everybody's time and
we have a kernel to test.

Will

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

[Will Deacon]

On Tue, Jan 05, 2016 at 08:36:39AM +0100, Ard Biesheuvel wrote:
> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@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@arm.com>
> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> >
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Sudeep Holla <sudeep.holla@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);
> 
> 
> Hello all,
> 
> Apologies for not spotting this before sending it out.
> 
> The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did
> not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC,
> since the series this is part of primarily concerns ARM not arm64.
> 
> Anyway, shuffling init ordering is my least favorite way of fixing
> bugs. Since only ARM requires pgd_alloc() (for the kernel mappings),
> we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and
> define it to mean '__get_free_page(PGALLOC_GFP)' on arm64.

Or we could put the pgd_cache initialisation in pgtable_cache_init, which
sounds like the right place for it anyway.

Curious, but why are our EFI runtime services initialised off the back of
initcalls, whereas on x86 the initialisation is driven directly from
init/main.c? I'm not saying it should be changed, it just looks odd having
the two paths.

Will

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

[Ard Biesheuvel]

On 5 January 2016 at 13:31, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jan 05, 2016 at 08:36:39AM +0100, Ard Biesheuvel wrote:
>> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@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@arm.com>
>> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
>> >
>> > Cc: Will Deacon <will.deacon@arm.com>
>> > Cc: Catalin Marinas <catalin.marinas@arm.com>
>> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > Cc: Sudeep Holla <sudeep.holla@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);
>>
>>
>> Hello all,
>>
>> Apologies for not spotting this before sending it out.
>>
>> The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did
>> not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC,
>> since the series this is part of primarily concerns ARM not arm64.
>>
>> Anyway, shuffling init ordering is my least favorite way of fixing
>> bugs. Since only ARM requires pgd_alloc() (for the kernel mappings),
>> we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and
>> define it to mean '__get_free_page(PGALLOC_GFP)' on arm64.
>
> Or we could put the pgd_cache initialisation in pgtable_cache_init, which
> sounds like the right place for it anyway.
>

Indeed.

> Curious, but why are our EFI runtime services initialised off the back of
> initcalls, whereas on x86 the initialisation is driven directly from
> init/main.c? I'm not saying it should be changed, it just looks odd having
> the two paths.
>

Simply because we tried not to pollute all core files with explicit
EFI calls. The EFI init bits are called in line by setup_arch(),
because they are needed so early, so there we had no choice. The
runtime services bits are only needed much later, so there we used an
initcall because we could.

-- 
Ard.

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

[Will Deacon]

On Tue, Jan 05, 2016 at 01:47:11PM +0100, Ard Biesheuvel wrote:
> On 5 January 2016 at 13:31, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jan 05, 2016 at 08:36:39AM +0100, Ard Biesheuvel wrote:
> >> The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did
> >> not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC,
> >> since the series this is part of primarily concerns ARM not arm64.
> >>
> >> Anyway, shuffling init ordering is my least favorite way of fixing
> >> bugs. Since only ARM requires pgd_alloc() (for the kernel mappings),
> >> we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and
> >> define it to mean '__get_free_page(PGALLOC_GFP)' on arm64.
> >
> > Or we could put the pgd_cache initialisation in pgtable_cache_init, which
> > sounds like the right place for it anyway.
> >
> 
> Indeed.

Patch below. Seems to do the job on my Juno.

Will

--->8

>From 39b5be9b4233a9f212b98242bddf008f379b5122 Mon Sep 17 00:00:00 2001
From: Will Deacon <will.deacon@arm.com>
Date: Tue, 5 Jan 2016 15:36:59 +0000
Subject: [PATCH] arm64: mm: move pgd_cache initialisation to
 pgtable_cache_init

Initialising the suppport for EFI runtime services requires us to
allocate a pgd off the back of an early_initcall. On systems where the
PGD_SIZE is smaller than PAGE_SIZE (e.g. 64k pages and 48-bit VA), the
pgd_cache isn't initialised at this stage, and we panic with a NULL
dereference during boot:

  Unable to handle kernel NULL pointer dereference at virtual address 00000000

  __create_mapping.isra.5+0x84/0x350
  create_pgd_mapping+0x20/0x28
  efi_create_mapping+0x5c/0x6c
  arm_enable_runtime_services+0x154/0x1e4
  do_one_initcall+0x8c/0x190
  kernel_init_freeable+0x84/0x1ec
  kernel_init+0x10/0xe0
  ret_from_fork+0x10/0x50

This patch fixes the problem by initialising the pgd_cache earlier, in
the pgtable_cache_init callback, which sounds suspiciously like what it
was intended for.

Reported-by: Dennis Chen <dennis.chen@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/pgtable.h |  3 ++-
 arch/arm64/mm/pgd.c              | 12 ++++++------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 35a318c2fd87..a87e964d2791 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -676,7 +676,8 @@ extern int kern_addr_valid(unsigned long addr);
 
 #include <asm-generic/pgtable.h>
 
-#define pgtable_cache_init() do { } while (0)
+void pgd_cache_init(void);
+#define pgtable_cache_init	pgd_cache_init
 
 /*
  * On AArch64, the cache coherency is handled via the set_pte_at() function.
diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c
index cb3ba1b812e7..ae11d4e03d0e 100644
--- a/arch/arm64/mm/pgd.c
+++ b/arch/arm64/mm/pgd.c
@@ -46,14 +46,14 @@ void pgd_free(struct mm_struct *mm, pgd_t *pgd)
 		kmem_cache_free(pgd_cache, pgd);
 }
 
-static int __init pgd_cache_init(void)
+void __init pgd_cache_init(void)
 {
+	if (PGD_SIZE == PAGE_SIZE)
+		return;
+
 	/*
 	 * Naturally aligned pgds required by the architecture.
 	 */
-	if (PGD_SIZE != PAGE_SIZE)
-		pgd_cache = kmem_cache_create("pgd_cache", PGD_SIZE, PGD_SIZE,
-					      SLAB_PANIC, NULL);
-	return 0;
+	pgd_cache = kmem_cache_create("pgd_cache", PGD_SIZE, PGD_SIZE,
+				      SLAB_PANIC, NULL);
 }
-core_initcall(pgd_cache_init);
-- 
2.1.4

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

[Dennis Chen]

On Tue, Jan 05, 2016 at 03:44:44PM +0000, Will Deacon wrote:
> On Tue, Jan 05, 2016 at 01:47:11PM +0100, Ard Biesheuvel wrote:
> > On 5 January 2016 at 13:31, Will Deacon <will.deacon@arm.com> wrote:
> > > On Tue, Jan 05, 2016 at 08:36:39AM +0100, Ard Biesheuvel wrote:
> > >> The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did
> > >> not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC,
> > >> since the series this is part of primarily concerns ARM not arm64.
> > >>
> > >> Anyway, shuffling init ordering is my least favorite way of fixing
> > >> bugs. Since only ARM requires pgd_alloc() (for the kernel mappings),
> > >> we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and
> > >> define it to mean '__get_free_page(PGALLOC_GFP)' on arm64.
> > >
> > > Or we could put the pgd_cache initialisation in pgtable_cache_init, which
> > > sounds like the right place for it anyway.
> > >
> >
> > Indeed.
>
> Patch below. Seems to do the job on my Juno.
>
> Will
>
> --->8
>
> From 39b5be9b4233a9f212b98242bddf008f379b5122 Mon Sep 17 00:00:00 2001
> From: Will Deacon <will.deacon@arm.com>
> Date: Tue, 5 Jan 2016 15:36:59 +0000
> Subject: [PATCH] arm64: mm: move pgd_cache initialisation to
>  pgtable_cache_init
>
> Initialising the suppport for EFI runtime services requires us to
> allocate a pgd off the back of an early_initcall. On systems where the
> PGD_SIZE is smaller than PAGE_SIZE (e.g. 64k pages and 48-bit VA), the
> pgd_cache isn't initialised at this stage, and we panic with a NULL
> dereference during boot:
>
>   Unable to handle kernel NULL pointer dereference at virtual address 00000000
>
>   __create_mapping.isra.5+0x84/0x350
>   create_pgd_mapping+0x20/0x28
>   efi_create_mapping+0x5c/0x6c
>   arm_enable_runtime_services+0x154/0x1e4
>   do_one_initcall+0x8c/0x190
>   kernel_init_freeable+0x84/0x1ec
>   kernel_init+0x10/0xe0
>   ret_from_fork+0x10/0x50
>
> This patch fixes the problem by initialising the pgd_cache earlier, in
> the pgtable_cache_init callback, which sounds suspiciously like what it
> was intended for.
>
> Reported-by: Dennis Chen <dennis.chen@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/include/asm/pgtable.h |  3 ++-
>  arch/arm64/mm/pgd.c              | 12 ++++++------
>  2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 35a318c2fd87..a87e964d2791 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -676,7 +676,8 @@ extern int kern_addr_valid(unsigned long addr);
>
>  #include <asm-generic/pgtable.h>
>
> -#define pgtable_cache_init() do { } while (0)
> +void pgd_cache_init(void);
> +#define pgtable_cache_init   pgd_cache_init
>
>  /*
>   * On AArch64, the cache coherency is handled via the set_pte_at() function.
> diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c
> index cb3ba1b812e7..ae11d4e03d0e 100644
> --- a/arch/arm64/mm/pgd.c
> +++ b/arch/arm64/mm/pgd.c
> @@ -46,14 +46,14 @@ void pgd_free(struct mm_struct *mm, pgd_t *pgd)
>               kmem_cache_free(pgd_cache, pgd);
>  }
>
> -static int __init pgd_cache_init(void)
> +void __init pgd_cache_init(void)
>  {
> +     if (PGD_SIZE == PAGE_SIZE)
> +             return;
> +
It doesn't need to add a new if statement here.
>       /*
>        * Naturally aligned pgds required by the architecture.
>        */
> -     if (PGD_SIZE != PAGE_SIZE)
> -             pgd_cache = kmem_cache_create("pgd_cache", PGD_SIZE, PGD_SIZE,
> -                                           SLAB_PANIC, NULL);
The deleted code here is more better, seems we don't need to change it here
> -     return 0;
Nice.
> +     pgd_cache = kmem_cache_create("pgd_cache", PGD_SIZE, PGD_SIZE,
> +                                   SLAB_PANIC, NULL);

>  }
> -core_initcall(pgd_cache_init);
The new patch is reasonable for me.

> --
> 2.1.4
>
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.