* [PATCH v2] powerpc: setup_64: set up PACA earlier to avoid kcov problems
@ 2020-02-13 5:23 Daniel Axtens
2020-02-13 6:23 ` Christophe Leroy
2020-02-14 3:04 ` Andrew Donnellan
0 siblings, 2 replies; 5+ messages in thread
From: Daniel Axtens @ 2020-02-13 5:23 UTC (permalink / raw)
To: linuxppc-dev; +Cc: ajd, Daniel Axtens
kcov instrumentation is collected the __sanitizer_cov_trace_pc hook in
kernel/kcov.c. The compiler inserts these hooks into every basic block
unless kcov is disabled for that file.
We then have a deep call-chain:
- __sanitizer_cov_trace_pc calls to check_kcov_mode()
- check_kcov_mode() (kernel/kcov.c) calls in_task()
- in_task() (include/linux/preempt.h) calls preempt_count().
- preempt_count() (include/asm-generic/preempt.h) calls
current_thread_info()
- because powerpc has THREAD_INFO_IN_TASK, current_thread_info()
(include/linux/thread_info.h) is defined to 'current'
- current (arch/powerpc/include/asm/current.h) is defined to
get_current().
- get_current (same file) loads an offset of r13.
- arch/powerpc/include/asm/paca.h makes r13 a register variable
called local_paca - it is the PACA for the current CPU, so
this has the effect of loading the current task from PACA.
- get_current returns the current task from PACA,
- current_thread_info returns the task cast to a thread_info
- preempt_count dereferences the thread_info to load preempt_count
- that value is used by in_task and so on up the chain
The problem is:
- kcov instrumentation is enabled for arch/powerpc/kernel/dt_cpu_ftrs.c
- even if it were not, dt_cpu_ftrs_init calls generic dt parsing code
which should definitely have instrumentation enabled.
- setup_64.c calls dt_cpu_ftrs_init before it sets up a PACA.
- If we don't set up a paca, r13 will contain unpredictable data.
- In a zImage compiled with kcov and KASAN, we see r13 containing a value
that leads to dereferencing invalid memory (something like
912a72603d420015).
- Weirdly, the same kernel as a vmlinux loaded directly by qemu does not
crash. Investigating with gdb, it seems that in the vmlinux boot case,
r13 is near enough to zero that we just happen to be able to read that
part of memory (we're operating with translation off at this point) and
the current pointer also happens to land in readable memory and
everything just works.
- PACA setup refers to CPU features - setup_paca() looks at
early_cpu_has_feature(CPU_FTR_HVMODE)
There's no generic kill switch for kcov (as far as I can tell), and we
don't want to have to turn off instrumentation in the generic dt parsing
code (which lives outside arch/powerpc/) just because we don't have a real
paca or task yet.
So:
- change the test when setting up a PACA to consider the actual value of
the MSR rather than the CPU feature.
- move the PACA setup to before the cpu feature parsing.
Translations get switched on once we leave early_setup, so I think we'd
already catch any other cases where the PACA or task aren't set up.
Boot tested on a P9 guest and host.
Fixes: fb0b0a73b223 ("powerpc: Enable kcov")
Cc: Andrew Donnellan <ajd@linux.ibm.com>
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
Regarding moving the comment about printk()-safety:
I am about 75% sure that the thing that makes printk() safe is the PACA,
not the CPU features. That's what commit 24d9649574fb ("[POWERPC] Document
when printk is useable") seems to indicate, but as someone wise recently
told me, "bootstrapping is hard", so I may be totally wrong.
---
arch/powerpc/kernel/paca.c | 2 +-
arch/powerpc/kernel/setup_64.c | 17 ++++++++++++-----
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
index 949eceb254d8..347e947b9d4b 100644
--- a/arch/powerpc/kernel/paca.c
+++ b/arch/powerpc/kernel/paca.c
@@ -218,7 +218,7 @@ void setup_paca(struct paca_struct *new_paca)
* if we do a GET_PACA() before the feature fixups have been
* applied
*/
- if (early_cpu_has_feature(CPU_FTR_HVMODE))
+ if (mfmsr() & MSR_HV)
mtspr(SPRN_SPRG_HPACA, local_paca);
#endif
mtspr(SPRN_SPRG_PACA, local_paca);
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index e05e6dd67ae6..ef455da7efa3 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -285,18 +285,25 @@ void __init early_setup(unsigned long dt_ptr)
/* -------- printk is _NOT_ safe to use here ! ------- */
- /* Try new device tree based feature discovery ... */
- if (!dt_cpu_ftrs_init(__va(dt_ptr)))
- /* Otherwise use the old style CPU table */
- identify_cpu(0, mfspr(SPRN_PVR));
+ /* Assume we're on cpu 0 for now. Don't write to the paca yet!
+ *
+ * We need to load a PACA very early if we are using kcov. kcov will
+ * call in_task() in its instrumentation, which relies on the current
+ * task from the PACA. dt_cpu_ftrs_init is coveraged-enabled and also
+ * calls into the coverage-enabled generic dt library.
+ */
- /* Assume we're on cpu 0 for now. Don't write to the paca yet! */
initialise_paca(&boot_paca, 0);
setup_paca(&boot_paca);
fixup_boot_paca();
/* -------- printk is now safe to use ------- */
+ /* Try new device tree based feature discovery ... */
+ if (!dt_cpu_ftrs_init(__va(dt_ptr)))
+ /* Otherwise use the old style CPU table */
+ identify_cpu(0, mfspr(SPRN_PVR));
+
/* Enable early debugging if any specified (see udbg.h) */
udbg_early_init();
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] powerpc: setup_64: set up PACA earlier to avoid kcov problems
2020-02-13 5:23 [PATCH v2] powerpc: setup_64: set up PACA earlier to avoid kcov problems Daniel Axtens
@ 2020-02-13 6:23 ` Christophe Leroy
2020-02-14 3:04 ` Andrew Donnellan
1 sibling, 0 replies; 5+ messages in thread
From: Christophe Leroy @ 2020-02-13 6:23 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev; +Cc: ajd
Le 13/02/2020 à 06:23, Daniel Axtens a écrit :
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index e05e6dd67ae6..ef455da7efa3 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -285,18 +285,25 @@ void __init early_setup(unsigned long dt_ptr)
>
> /* -------- printk is _NOT_ safe to use here ! ------- */
>
> - /* Try new device tree based feature discovery ... */
> - if (!dt_cpu_ftrs_init(__va(dt_ptr)))
> - /* Otherwise use the old style CPU table */
> - identify_cpu(0, mfspr(SPRN_PVR));
> + /* Assume we're on cpu 0 for now. Don't write to the paca yet!
> + *
> + * We need to load a PACA very early if we are using kcov. kcov will
> + * call in_task() in its instrumentation, which relies on the current
> + * task from the PACA. dt_cpu_ftrs_init is coveraged-enabled and also
> + * calls into the coverage-enabled generic dt library.
> + */
Checkpatch won't be happy I guess. For multiline comments, there should
be no text on the first line starting with /*
Christophe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] powerpc: setup_64: set up PACA earlier to avoid kcov problems
2020-02-13 5:23 [PATCH v2] powerpc: setup_64: set up PACA earlier to avoid kcov problems Daniel Axtens
2020-02-13 6:23 ` Christophe Leroy
@ 2020-02-14 3:04 ` Andrew Donnellan
2020-03-06 4:40 ` Daniel Axtens
1 sibling, 1 reply; 5+ messages in thread
From: Andrew Donnellan @ 2020-02-14 3:04 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev
On 13/2/20 4:23 pm, Daniel Axtens wrote:
> kcov instrumentation is collected the __sanitizer_cov_trace_pc hook in
> kernel/kcov.c. The compiler inserts these hooks into every basic block
> unless kcov is disabled for that file.
>
> We then have a deep call-chain:
> - __sanitizer_cov_trace_pc calls to check_kcov_mode()
> - check_kcov_mode() (kernel/kcov.c) calls in_task()
> - in_task() (include/linux/preempt.h) calls preempt_count().
> - preempt_count() (include/asm-generic/preempt.h) calls
> current_thread_info()
> - because powerpc has THREAD_INFO_IN_TASK, current_thread_info()
> (include/linux/thread_info.h) is defined to 'current'
> - current (arch/powerpc/include/asm/current.h) is defined to
> get_current().
> - get_current (same file) loads an offset of r13.
> - arch/powerpc/include/asm/paca.h makes r13 a register variable
> called local_paca - it is the PACA for the current CPU, so
> this has the effect of loading the current task from PACA.
> - get_current returns the current task from PACA,
> - current_thread_info returns the task cast to a thread_info
> - preempt_count dereferences the thread_info to load preempt_count
> - that value is used by in_task and so on up the chain
>
> The problem is:
>
> - kcov instrumentation is enabled for arch/powerpc/kernel/dt_cpu_ftrs.c
>
> - even if it were not, dt_cpu_ftrs_init calls generic dt parsing code
> which should definitely have instrumentation enabled.
>
> - setup_64.c calls dt_cpu_ftrs_init before it sets up a PACA.
>
> - If we don't set up a paca, r13 will contain unpredictable data.
>
> - In a zImage compiled with kcov and KASAN, we see r13 containing a value
> that leads to dereferencing invalid memory (something like
> 912a72603d420015).
>
> - Weirdly, the same kernel as a vmlinux loaded directly by qemu does not
> crash. Investigating with gdb, it seems that in the vmlinux boot case,
> r13 is near enough to zero that we just happen to be able to read that
> part of memory (we're operating with translation off at this point) and
> the current pointer also happens to land in readable memory and
> everything just works.
>
> - PACA setup refers to CPU features - setup_paca() looks at
> early_cpu_has_feature(CPU_FTR_HVMODE)
>
> There's no generic kill switch for kcov (as far as I can tell), and we
> don't want to have to turn off instrumentation in the generic dt parsing
> code (which lives outside arch/powerpc/) just because we don't have a real
> paca or task yet.
>
> So:
> - change the test when setting up a PACA to consider the actual value of
> the MSR rather than the CPU feature.
>
> - move the PACA setup to before the cpu feature parsing.
>
> Translations get switched on once we leave early_setup, so I think we'd
> already catch any other cases where the PACA or task aren't set up.
>
> Boot tested on a P9 guest and host.
>
> Fixes: fb0b0a73b223 ("powerpc: Enable kcov")
> Cc: Andrew Donnellan <ajd@linux.ibm.com>
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
There's some special handling for CPU_FTR_HVMODE in
cpufeatures_setup_cpu() in kernel/dt_cpu_ftrs.c:
/* Initialize the base environment -- clear FSCR/HFSCR. */
hv_mode = !!(mfmsr() & MSR_HV);
if (hv_mode) {
/* CPU_FTR_HVMODE is used early in PACA setup */
cur_cpu_spec->cpu_features |= CPU_FTR_HVMODE;
mtspr(SPRN_HFSCR, 0);
}
With this patch, the comment about PACA setup I assume is no longer
true. It looks like we still rely on hv_mode being set to deal with
discrepancies between the device tree and the MSR.
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] powerpc: setup_64: set up PACA earlier to avoid kcov problems
2020-02-14 3:04 ` Andrew Donnellan
@ 2020-03-06 4:40 ` Daniel Axtens
2020-03-06 4:45 ` Andrew Donnellan
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Axtens @ 2020-03-06 4:40 UTC (permalink / raw)
To: Andrew Donnellan, linuxppc-dev
> There's some special handling for CPU_FTR_HVMODE in
> cpufeatures_setup_cpu() in kernel/dt_cpu_ftrs.c:
>
> /* Initialize the base environment -- clear FSCR/HFSCR. */
> hv_mode = !!(mfmsr() & MSR_HV);
> if (hv_mode) {
> /* CPU_FTR_HVMODE is used early in PACA setup */
> cur_cpu_spec->cpu_features |= CPU_FTR_HVMODE;
> mtspr(SPRN_HFSCR, 0);
> }
>
> With this patch, the comment about PACA setup I assume is no longer
> true. It looks like we still rely on hv_mode being set to deal with
> discrepancies between the device tree and the MSR.
This code confuses me. IIUC it sets the CPU feature if we're in HV mode,
which will catch the case where the HV bit is set in the MSR but for
some reason it's not listed in the DT. With my patch, we'll directly
test the MSR so we don't need the cpu feature set for that.
However, the CPU feature is tested elsewhere, so I think the correct
behaviour is to keep the code but drop the comment. Having said that
bootstrapping is hard so lmk if I've misunderstood.
Regards,
Daniel
>
> --
> Andrew Donnellan OzLabs, ADL Canberra
> ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] powerpc: setup_64: set up PACA earlier to avoid kcov problems
2020-03-06 4:40 ` Daniel Axtens
@ 2020-03-06 4:45 ` Andrew Donnellan
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Donnellan @ 2020-03-06 4:45 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev
On 6/3/20 3:40 pm, Daniel Axtens wrote:
>> There's some special handling for CPU_FTR_HVMODE in
>> cpufeatures_setup_cpu() in kernel/dt_cpu_ftrs.c:
>>
>> /* Initialize the base environment -- clear FSCR/HFSCR. */
>> hv_mode = !!(mfmsr() & MSR_HV);
>> if (hv_mode) {
>> /* CPU_FTR_HVMODE is used early in PACA setup */
>> cur_cpu_spec->cpu_features |= CPU_FTR_HVMODE;
>> mtspr(SPRN_HFSCR, 0);
>> }
>>
>> With this patch, the comment about PACA setup I assume is no longer
>> true. It looks like we still rely on hv_mode being set to deal with
>> discrepancies between the device tree and the MSR.
>
> This code confuses me. IIUC it sets the CPU feature if we're in HV mode,
> which will catch the case where the HV bit is set in the MSR but for
> some reason it's not listed in the DT. With my patch, we'll directly
> test the MSR so we don't need the cpu feature set for that.
>
> However, the CPU feature is tested elsewhere, so I think the correct
> behaviour is to keep the code but drop the comment. Having said that
> bootstrapping is hard so lmk if I've misunderstood.
That was my thinking too.
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-06 4:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13 5:23 [PATCH v2] powerpc: setup_64: set up PACA earlier to avoid kcov problems Daniel Axtens
2020-02-13 6:23 ` Christophe Leroy
2020-02-14 3:04 ` Andrew Donnellan
2020-03-06 4:40 ` Daniel Axtens
2020-03-06 4:45 ` Andrew Donnellan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).