Re: [PATCH v2] powerpc: setup_64: set up PACA earlier to avoid kcov problems

[Andrew Donnellan <ajd@linux.ibm.com>]

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