[PATCH] arm64: Enable MRS emulation early

[PATCH] arm64: Enable MRS emulation early

[Suzuki K Poulose]

Make sure the MRS emulation is enabled early enough, such that the
early userspace applications (e.g, those run from initrd) could
use the facility without crashing them.

Fixes: commit 77c97b4ee2129 ("arm64: cpufeature: Expose CPUID registers by emulation")
Reported-by: Matwey V. Kornilov <matwey.kornilov@gmail.com>
Cc: James Morse <james.morse@arm.com>
Cc: Dave Martin <Dave.martin@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: stable at vger.kernel.org
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/cpufeature.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 9f9e0064c8c1..276eecab6cea 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1294,4 +1294,4 @@ static int __init enable_mrs_emulation(void)
 	return 0;
 }
 
-late_initcall(enable_mrs_emulation);
+core_initcall(enable_mrs_emulation);
-- 
2.13.5

[PATCH] arm64: Enable MRS emulation early

[Mark Rutland]

On Wed, Oct 04, 2017 at 10:48:05AM +0100, Suzuki K Poulose wrote:
> Make sure the MRS emulation is enabled early enough, such that the
> early userspace applications (e.g, those run from initrd) could
> use the facility without crashing them.
> 
> Fixes: commit 77c97b4ee2129 ("arm64: cpufeature: Expose CPUID registers by emulation")
> Reported-by: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Dave Martin <Dave.martin@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: stable at vger.kernel.org
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

This looks sensible, but shouldn't we do the same for other
late_inicalls can affect initrd userspace?

e.g. armv8_deprecated_init, fpsimd_init, sys_reg_genericv8_init?

Thanks,
Mark.

> ---
>  arch/arm64/kernel/cpufeature.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 9f9e0064c8c1..276eecab6cea 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1294,4 +1294,4 @@ static int __init enable_mrs_emulation(void)
>  	return 0;
>  }
>  
> -late_initcall(enable_mrs_emulation);
> +core_initcall(enable_mrs_emulation);
> -- 
> 2.13.5
> 

[PATCH] arm64: Enable MRS emulation early

[Catalin Marinas]

On Wed, Oct 04, 2017 at 11:14:26AM +0100, Mark Rutland wrote:
> On Wed, Oct 04, 2017 at 10:48:05AM +0100, Suzuki K Poulose wrote:
> > Make sure the MRS emulation is enabled early enough, such that the
> > early userspace applications (e.g, those run from initrd) could
> > use the facility without crashing them.
> > 
> > Fixes: commit 77c97b4ee2129 ("arm64: cpufeature: Expose CPUID registers by emulation")
> > Reported-by: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> > Cc: James Morse <james.morse@arm.com>
> > Cc: Dave Martin <Dave.martin@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: stable at vger.kernel.org
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> This looks sensible, but shouldn't we do the same for other
> late_inicalls can affect initrd userspace?
> 
> e.g. armv8_deprecated_init, fpsimd_init, sys_reg_genericv8_init?

I think we should, though not all of them are concerned with the user
code. For example, fpsimd_init() takes care of the pm/hotplug aspect and
nothing to do with user space. That said, making it core_initcall() is
probably not a bad thing (just a statement that it is concerned with the
core initialisation), as long as all the other infrastructure it
registers with is up.

For Suzuki's patch, I was thinking of enabling emulation before we
register the HWCAP_CPUID bit (setup_elf_hwcaps). However, that means we
have to bring it before smp_cpus_done(). It's not really worth it as we
don't expect any user space at that point.

-- 
Catalin

[PATCH] arm64: Enable MRS emulation early

[Dave Martin]

On Wed, Oct 04, 2017 at 12:10:40PM +0100, Catalin Marinas wrote:
> On Wed, Oct 04, 2017 at 11:14:26AM +0100, Mark Rutland wrote:
> > On Wed, Oct 04, 2017 at 10:48:05AM +0100, Suzuki K Poulose wrote:
> > > Make sure the MRS emulation is enabled early enough, such that the
> > > early userspace applications (e.g, those run from initrd) could
> > > use the facility without crashing them.
> > > 
> > > Fixes: commit 77c97b4ee2129 ("arm64: cpufeature: Expose CPUID registers by emulation")
> > > Reported-by: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> > > Cc: James Morse <james.morse@arm.com>
> > > Cc: Dave Martin <Dave.martin@arm.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will.deacon@arm.com>
> > > Cc: stable at vger.kernel.org
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > 
> > This looks sensible, but shouldn't we do the same for other
> > late_inicalls can affect initrd userspace?
> > 
> > e.g. armv8_deprecated_init, fpsimd_init, sys_reg_genericv8_init?
> 
> I think we should, though not all of them are concerned with the user
> code. For example, fpsimd_init() takes care of the pm/hotplug aspect and
> nothing to do with user space. That said, making it core_initcall() is
> probably not a bad thing (just a statement that it is concerned with the
> core initialisation), as long as all the other infrastructure it
> registers with is up.
> 
> For Suzuki's patch, I was thinking of enabling emulation before we
> register the HWCAP_CPUID bit (setup_elf_hwcaps). However, that means we
> have to bring it before smp_cpus_done(). It's not really worth it as we
> don't expect any user space at that point.

I don't think the hwcaps shouldn't change after entry to userspace,
so it really doesn't matter whether HWCAP_CPUID is set before or
after registration: for userspace it should all already have happened.


It looks to me like all initcalls are called in the same kernel thread
that execs the initramfs init process, before the exec.

So I still don't see how a built-in late initcall can not have been
called before entry to userspace.

The patch seems to demonstrate that I'm wrong though.
What am I missing?

Cheers
---Dave

[PATCH] arm64: Enable MRS emulation early

[Catalin Marinas]

On Wed, Oct 04, 2017 at 12:32:07PM +0100, Dave P Martin wrote:
> On Wed, Oct 04, 2017 at 12:10:40PM +0100, Catalin Marinas wrote:
> > On Wed, Oct 04, 2017 at 11:14:26AM +0100, Mark Rutland wrote:
> > > On Wed, Oct 04, 2017 at 10:48:05AM +0100, Suzuki K Poulose wrote:
> > > > Make sure the MRS emulation is enabled early enough, such that the
> > > > early userspace applications (e.g, those run from initrd) could
> > > > use the facility without crashing them.
> > > > 
> > > > Fixes: commit 77c97b4ee2129 ("arm64: cpufeature: Expose CPUID registers by emulation")
> > > > Reported-by: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> > > > Cc: James Morse <james.morse@arm.com>
> > > > Cc: Dave Martin <Dave.martin@arm.com>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: Will Deacon <will.deacon@arm.com>
> > > > Cc: stable at vger.kernel.org
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > 
> > > This looks sensible, but shouldn't we do the same for other
> > > late_inicalls can affect initrd userspace?
> > > 
> > > e.g. armv8_deprecated_init, fpsimd_init, sys_reg_genericv8_init?
> > 
> > I think we should, though not all of them are concerned with the user
> > code. For example, fpsimd_init() takes care of the pm/hotplug aspect and
> > nothing to do with user space. That said, making it core_initcall() is
> > probably not a bad thing (just a statement that it is concerned with the
> > core initialisation), as long as all the other infrastructure it
> > registers with is up.
> > 
> > For Suzuki's patch, I was thinking of enabling emulation before we
> > register the HWCAP_CPUID bit (setup_elf_hwcaps). However, that means we
> > have to bring it before smp_cpus_done(). It's not really worth it as we
> > don't expect any user space at that point.
> 
> I don't think the hwcaps shouldn't change after entry to userspace,
> so it really doesn't matter whether HWCAP_CPUID is set before or
> after registration: for userspace it should all already have happened.

Good point, I forgot about this.

> It looks to me like all initcalls are called in the same kernel thread
> that execs the initramfs init process, before the exec.
> 
> So I still don't see how a built-in late initcall can not have been
> called before entry to userspace.
> 
> The patch seems to demonstrate that I'm wrong though.
> What am I missing?

I also wondered about this. I think is the kernel invoking modprobe
before the actual init/linuxrc in an initrd.

-- 
Catalin

[PATCH] arm64: Enable MRS emulation early

[Dave Martin]

On Wed, Oct 04, 2017 at 12:36:29PM +0100, Catalin Marinas wrote:
> On Wed, Oct 04, 2017 at 12:32:07PM +0100, Dave P Martin wrote:

[...]

> > I don't think the hwcaps shouldn't change after entry to userspace,
> > so it really doesn't matter whether HWCAP_CPUID is set before or
> > after registration: for userspace it should all already have happened.
> 
> Good point, I forgot about this.
> 
> > It looks to me like all initcalls are called in the same kernel thread
> > that execs the initramfs init process, before the exec.
> > 
> > So I still don't see how a built-in late initcall can not have been
> > called before entry to userspace.
> > 
> > The patch seems to demonstrate that I'm wrong though.
> > What am I missing?
> 
> I also wondered about this. I think is the kernel invoking modprobe
> before the actual init/linuxrc in an initrd.

Ah, right.  Could that be a bug, do you think?

I wonder whether it's even well-defined how early that can happen.
i.e., which initcall level is guaranteed early enough to prevent this?

Cheers
---Dave

[PATCH] arm64: Enable MRS emulation early

[Catalin Marinas]

On Wed, Oct 04, 2017 at 01:48:19PM +0100, Dave P Martin wrote:
> On Wed, Oct 04, 2017 at 12:36:29PM +0100, Catalin Marinas wrote:
> > On Wed, Oct 04, 2017 at 12:32:07PM +0100, Dave P Martin wrote:
> 
> [...]
> 
> > > I don't think the hwcaps shouldn't change after entry to userspace,
> > > so it really doesn't matter whether HWCAP_CPUID is set before or
> > > after registration: for userspace it should all already have happened.
> > 
> > Good point, I forgot about this.
> > 
> > > It looks to me like all initcalls are called in the same kernel thread
> > > that execs the initramfs init process, before the exec.
> > > 
> > > So I still don't see how a built-in late initcall can not have been
> > > called before entry to userspace.
> > > 
> > > The patch seems to demonstrate that I'm wrong though.
> > > What am I missing?
> > 
> > I also wondered about this. I think is the kernel invoking modprobe
> > before the actual init/linuxrc in an initrd.
> 
> Ah, right.  Could that be a bug, do you think?
> 
> I wonder whether it's even well-defined how early that can happen.
> i.e., which initcall level is guaranteed early enough to prevent this?

I don't think it's a bug, probably just an undocumented convention. I
don't know which module triggered this specific issue but, for example,
I see the possibility of request_module in the ipv4 code only after
fs_initcall(). Let's hope nothing executes user space prior to
arch_initcall().

usermodehlper_enable() is called in do_basic_setup() prior to
do_initcalls(), so there is always a chance of someone calling user
space early. If we want some guarantee, maybe something like below:

diff --git a/init/main.c b/init/main.c
index 0ee9c6866ada..67040e570533 100644
--- a/init/main.c
+++ b/init/main.c
@@ -914,10 +914,16 @@ static void __init do_basic_setup(void)
 	driver_init();
 	init_irq_proc();
 	do_ctors();
-	usermodehelper_enable();
 	do_initcalls();
 }
 
+static int usermodehelper_init(void)
+{
+	usermodehelper_enable();
+	return 0;
+}
+arch_initcall_sync(usermodehelper_init);
+
 static void __init do_pre_smp_initcalls(void)
 {
 	initcall_t *fn;

[PATCH] arm64: Enable MRS emulation early

[Mark Rutland]

On Wed, Oct 04, 2017 at 12:10:40PM +0100, Catalin Marinas wrote:
> On Wed, Oct 04, 2017 at 11:14:26AM +0100, Mark Rutland wrote:
> > On Wed, Oct 04, 2017 at 10:48:05AM +0100, Suzuki K Poulose wrote:
> > > Make sure the MRS emulation is enabled early enough, such that the
> > > early userspace applications (e.g, those run from initrd) could
> > > use the facility without crashing them.
> > > 
> > > Fixes: commit 77c97b4ee2129 ("arm64: cpufeature: Expose CPUID registers by emulation")
> > > Reported-by: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> > > Cc: James Morse <james.morse@arm.com>
> > > Cc: Dave Martin <Dave.martin@arm.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will.deacon@arm.com>
> > > Cc: stable at vger.kernel.org
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > 
> > This looks sensible, but shouldn't we do the same for other
> > late_inicalls can affect initrd userspace?
> > 
> > e.g. armv8_deprecated_init, fpsimd_init, sys_reg_genericv8_init?
> 
> I think we should, though not all of them are concerned with the user
> code. For example, fpsimd_init() takes care of the pm/hotplug aspect and
> nothing to do with user space.

My worry was that without the pm/hotplug notifiers, things could go
wrong during the initrd. e.g. we could lose userspace fp state without
the pm notifier, or userspace could trigger hotplpug that we wouldn't
manage correctly

So even if it's not directly userspace related, it can affect (or can be
affected by) initrd userspace.

> That said, making it core_initcall() is probably not a bad thing (just
> a statement that it is concerned with the core initialisation), as
> long as all the other infrastructure it registers with is up.

Sure.

> For Suzuki's patch, I was thinking of enabling emulation before we
> register the HWCAP_CPUID bit (setup_elf_hwcaps). However, that means we
> have to bring it before smp_cpus_done(). It's not really worth it as we
> don't expect any user space at that point.

I think so long as we bring it up before *any* userspace can run, it
doesn't matter if we set the hwcaps earlier.

Thanks,
Mark.

[PATCH] arm64: Enable MRS emulation early

[Suzuki K Poulose]

On 04/10/17 12:32, Mark Rutland wrote:
> On Wed, Oct 04, 2017 at 12:10:40PM +0100, Catalin Marinas wrote:
>> On Wed, Oct 04, 2017 at 11:14:26AM +0100, Mark Rutland wrote:
>>> On Wed, Oct 04, 2017 at 10:48:05AM +0100, Suzuki K Poulose wrote:
>>>> Make sure the MRS emulation is enabled early enough, such that the
>>>> early userspace applications (e.g, those run from initrd) could
>>>> use the facility without crashing them.
>>>>
>>>> Fixes: commit 77c97b4ee2129 ("arm64: cpufeature: Expose CPUID registers by emulation")
>>>> Reported-by: Matwey V. Kornilov <matwey.kornilov@gmail.com>
>>>> Cc: James Morse <james.morse@arm.com>
>>>> Cc: Dave Martin <Dave.martin@arm.com>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>> Cc: stable at vger.kernel.org
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>
>>> This looks sensible, but shouldn't we do the same for other
>>> late_inicalls can affect initrd userspace?
>>>
>>> e.g. armv8_deprecated_init, fpsimd_init, sys_reg_genericv8_init?
>>
>> I think we should, though not all of them are concerned with the user
>> code. For example, fpsimd_init() takes care of the pm/hotplug aspect and
>> nothing to do with user space.
> 
> My worry was that without the pm/hotplug notifiers, things could go
> wrong during the initrd. e.g. we could lose userspace fp state without
> the pm notifier, or userspace could trigger hotplpug that we wouldn't
> manage correctly
> 
> So even if it's not directly userspace related, it can affect (or can be
> affected by) initrd userspace.

You're right. In fact, I had a version of the patch which enables the emulation
as soon as we have initialised the ELF_HWCAPs from setup_cpu_features(), rather
than depending on an initcall. But that requires moving the setup_cpu_features()
to the bottom, which makes the hunk look a bit more complex than it is.

And similarly, we should be able to do the fpsimd_init from setup_cpu_features(),
as we have finalised the HWCAPs and pm_register_notifier any adds the entry to
a static list of notifiers ( even though the cpu_pm callbacks are registered as
a core_initcall() ).

Similarly for sys_reg_genericv8_init & armv8_deprecated_init could be made a core
initcall.

I think it would be good to separate them out.

i.e, enable_mrs_emulation & fpsimd_init from setup_cpu_features()
and the other two promoted as core_initcalls.

Thoughts ?

Suzuki