Re: [RFC PATCH v3 3/7] powerpc: Activate CONFIG_THREAD_INFO_IN_TASK

[Nicholas Piggin]

On Mon,  1 Oct 2018 12:30:23 +0000 (UTC)
Christophe Leroy <christophe.leroy@xxxxxx> wrote:

> This patch activates CONFIG_THREAD_INFO_IN_TASK which
> moves the thread_info into task_struct.
> 
> Moving thread_info into task_struct has the following advantages:
> - It protects thread_info from corruption in the case of stack
> overflows.
> - Its address is harder to determine if stack addresses are
> leaked, making a number of attacks more difficult.
> 
> This has the following consequences:
> - thread_info is now located at the top of task_struct.

"top"... I got confused for a minute thinking high address and
wondering how you can change CURRENT_THREAD_INFO just to point
to current :)



> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 07d9dce7eda6..4e98989b5512 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -422,3 +422,9 @@ checkbin:
>  
>  CLEAN_FILES += $(TOUT)
>  
> +ifdef CONFIG_SMP
> +prepare: task_cpu_prepare
> +
> +task_cpu_prepare: prepare0
> +       $(eval KBUILD_CFLAGS += -D_TASK_CPU=$(shell awk '{if ($$2 == "TI_CPU") print $$3;}' include/generated/asm-offsets.h))
> +endif
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index 447cbd1bee99..3a7e5561630b 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -120,7 +120,7 @@ extern int ptrace_put_reg(struct task_struct *task, int regno,
>  			  unsigned long data);
>  
>  #define current_pt_regs() \
> -	((struct pt_regs *)((unsigned long)current_thread_info() + THREAD_SIZE) - 1)
> +	((struct pt_regs *)((unsigned long)task_stack_page(current) + THREAD_SIZE) - 1)
>  /*
>   * We use the least-significant bit of the trap field to indicate
>   * whether we have saved the full set of registers, or only a
> diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
> index 95b66a0c639b..df519b7322e5 100644
> --- a/arch/powerpc/include/asm/smp.h
> +++ b/arch/powerpc/include/asm/smp.h
> @@ -83,7 +83,13 @@ int is_cpu_dead(unsigned int cpu);
>  /* 32-bit */
>  extern int smp_hw_index[];
>  
> -#define raw_smp_processor_id()	(current_thread_info()->cpu)
> +/*
> + * This is particularly ugly: it appears we can't actually get the definition
> + * of task_struct here, but we need access to the CPU this task is running on.
> + * Instead of using task_struct we're using _TASK_CPU which is extracted from
> + * asm-offsets.h by kbuild to get the current processor ID.
> + */
> +#define raw_smp_processor_id()		(*(unsigned int*)((void*)current + _TASK_CPU))

This is clever but yes ugly. Can't you include asm-offsets.h? riscv
seems to.

I'm not 100% sure on kgdb and kexec stuff but I think it seems okay.
Looks like a pretty nice cleanup too aside from the features it brings,
thanks for working on it.

Reviewed-by: Nicholas Piggin <npiggin@xxxxxxxxx>