Re: [PATCH v3] powerpc: setup_64: set up PACA earlier to avoid kcov problems
Daniel Axtens's on March 11, 2020 9:03 am:
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.
>>>
>>> Hmm. Problem is that equally we want PACA to be sane before we call too
>>> far into the rest of the kernel ("generic dt parsing code").
>>
>> But currently we call into that code with no paca at all. Or rather,
>> with r13 pointing somewhere random that will be interpreted as being a
>> paca.
>>
>> This took a while for Daniel to debug because depending on how you boot
>> r13 contains a different junk value. That junk value may not point to
>> memory at all, or if it does the memory it points to may or may not send
>> you down the wrong path, depending on which exact bit you're looking at
>> in some random location.
>>
>> So this is really not about kcov from my POV, that's just how we
>> discovered it.
>
> Ah, yes. I agree with mpe, and reading back over my commit message I
> think I did a pretty poor job of explaining it. How about this for a
> commit message:
Sorry yeah I'm not quite sure what I was thinking there, because
you actually are moving the paca setup earlier. Hmm. Anyway it seems
okay to me. I would suggest putting a comment in the mfmsr() & MSR_HV
test so it doesn't get used elsewhere. Maybe include CPU_FTR_HVMODE
in the comment so grep shows it up.
Thanks,
Nick
Re: [PATCH v3] powerpc: setup_64: set up PACA earlier to avoid kcov problems
>>> 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.
>>
>> Hmm. Problem is that equally we want PACA to be sane before we call too
>> far into the rest of the kernel ("generic dt parsing code").
>
> But currently we call into that code with no paca at all. Or rather,
> with r13 pointing somewhere random that will be interpreted as being a
> paca.
>
> This took a while for Daniel to debug because depending on how you boot
> r13 contains a different junk value. That junk value may not point to
> memory at all, or if it does the memory it points to may or may not send
> you down the wrong path, depending on which exact bit you're looking at
> in some random location.
>
> So this is really not about kcov from my POV, that's just how we
> discovered it.
Ah, yes. I agree with mpe, and reading back over my commit message I
think I did a pretty poor job of explaining it. How about this for a
commit message:
---
powerpc: setup_64: set up PACA before parsing device tree
Currently, we set up the PACA after parsing the device tree for CPU
features. Before that, r13 contains random data, which means there is
random data in r13 while we're running the generic dt parsing code.
This random data varies depending on whether we boot through a vmlinux or a
zImage: for the vmlinux case it's usually around zero, but for zImages we
see random values like 912a72603d420015.
This is poor practice, and can also lead to difficult-to-debug crashes. For
example, when kcov is enabled, the kcov instrumentation attempts to read
preempt_count out of the current task, which goes via the PACA. This then
crashes in the zImage case.
To resolve this:
- move the PACA setup to before the cpu feature parsing.
- because we no longer have access to cpu feature flags in PACA setup,
change the HV feature test in the PACA setup path to consider the actual
value of the MSR rather than the CPU feature.
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
Cc: sta...@vger.kernel.org
Reviewed-by: Andrew Donnellan
Suggested-by: Michael Ellerman
Signed-off-by: Daniel Axtens
---
Regards,
Daniel
>
> cheers
Re: [PATCH v3] powerpc: setup_64: set up PACA earlier to avoid kcov problems
Nicholas Piggin writes:
> Daniel Axtens's on March 6, 2020 5:30 pm:
>> 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.
>
> Hmm. Problem is that equally we want PACA to be sane before we call too
> far into the rest of the kernel ("generic dt parsing code").
But currently we call into that code with no paca at all. Or rather,
with r13 pointing somewhere random that will be interpreted as being a
paca.
This took a while for Daniel to debug because depending on how you boot
r13 contains a different junk value. That junk value may not point to
memory at all, or if it does the memory it points to may or may not send
you down the wrong path, depending on which exact bit you're looking at
in some random location.
So this is really not about kcov from my POV, that's just how we
discovered it.
cheers
Re: [PATCH v3] powerpc: setup_64: set up PACA earlier to avoid kcov problems
Daniel Axtens's on March 6, 2020 5:30 pm:
> 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.
Hmm. Problem is that equally we want PACA to be sane before we call too
far into the rest of the kernel ("generic dt parsing code").
Does KCOV really need to instrument code this early? If not, then
change
if (!in_task())
return false;
to
if (system_state != SYSTEM_RUNNING)
return false;
if (!in_task())
return false;
If it does need to instrument early, then something like
if (system_state >= SYSTEM_SCHEDULING && !in_task())
return false;
Thanks,
Nick
Re: [PATCH v3] powerpc: setup_64: set up PACA earlier to avoid kcov problems
Andrew Donnellan writes:
> On 6/3/20 6:30 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
>> Suggested-by: Michael Ellerman
>> Signed-off-by: Daniel Axtens
>>
>> ---
>>
>> 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.
>>
>> v3: Update comment, thanks Christophe Leroy.
>> Remove a comment in dt_cpu_ftrs.c that is no longer accurate - thanks
>>Andrew. I think we want to retain all the code still, but I'm open to
>>being told otherwise.
>
> Thanks for doing that.
>
> This patch and the justification doesn't seem obviously wrong, and is
> snowpatch-clean.
>
> Reviewed-by: Andrew Donnellan
>
> (Is it worth cc'ing this to stable in case there are other situations we
> haven't foreseen where we hit the unpredictable r13 data? Few people use
> kcov...)
I did briefly consider it but didn't believe it reached the stable
criteria:
| It must fix a real bug that bothers people (not a, “This could be a
| problem...” type thing).
On reflection it's a real bug (boot hang), it bothers me, and presumably
also you due to the syzkaller interaction, and I am led to believe we
are both people, so I guess I'll do a v3 with cc: stable. Thanks!
Regards,
Daniel
>
> --
> Andrew Donnellan OzLabs, ADL Canberra
> a...@linux.ibm.com IBM Australia Limited
Re: [PATCH v3] powerpc: setup_64: set up PACA earlier to avoid kcov problems
On 6/3/20 6:30 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
Suggested-by: Michael Ellerman
Signed-off-by: Daniel Axtens
---
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.
v3: Update comment, thanks Christophe Leroy.
Remove a comment in dt_cpu_ftrs.c that is no longer accurate - thanks
Andrew. I think we want to retain all the code still, but I'm open to
being told otherwise.
Thanks for doing that.
This patch and the justification doesn't seem obviously wrong, and is
snowpatch-clean.
Reviewed-by: Andrew Donnellan
(Is it worth cc'ing this to stable in case there are other situations we
haven't foreseen where we hit the unpredictable r13 data? Few people use
kcov...)
--
Andrew Donnellan OzLabs, ADL Canberra
a...@linux.ibm.com IBM Australia Limited
[PATCH v3] powerpc: setup_64: set up PACA earlier to avoid kcov problems
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
Suggested-by: Michael Ellerman
Signed-off-by: Daniel Axtens
---
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.
v3: Update comment, thanks Christophe Leroy.
Remove a comment in dt_cpu_ftrs.c that is no longer accurate - thanks
Andrew. I think we want to retain all the code still, but I'm open to
being told otherwise.
---
arch/powerpc/kernel/dt_cpu_ftrs.c | 1 -
arch/powerpc/kernel/paca.c| 2 +-
arch/powerpc/kernel/setup_64.c| 20 +++-
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c
b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 182b4047c1ef..36bc0d5c4f3a 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -139,7 +139,6 @@ static void __init cpufeatures_setup_cpu(void)
/* 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);
}
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..2259da8e8685 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -285,18 +285,28 @@ void __init early_setup(unsigned
