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

[Christophe LEROY <christophe.leroy@c-s.fr>]

Le 03/10/2018 à 07:30, Nicholas Piggin a écrit :
> On Mon,  1 Oct 2018 12:30:23 +0000 (UTC)
> Christophe Leroy <christophe.leroy@c-s.fr> 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 :)

Would 'beginning' be less confusing ?

> 
> 
> 
>> 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.

riscv has a clean asm-offsets.h . Our's defines constant with the same 
name as those defined in other headers which are included in C files. So 
including asm-offsets in C files does create conflicts like:

./include/generated/asm-offsets.h:71:0: warning: "TASK_SIZE" redefined
  #define TASK_SIZE -2147483648 /* TASK_SIZE */
./arch/powerpc/include/asm/processor.h:95:0: note: this is the location 
of the previous definition
  #define TASK_SIZE (CONFIG_TASK_SIZE)

./include/generated/asm-offsets.h:98:0: warning: "NSEC_PER_SEC" redefined
  #define NSEC_PER_SEC 1000000000 /* NSEC_PER_SEC */
./include/linux/time64.h:36:0: note: this is the location of the 
previous definition
  #define NSEC_PER_SEC 1000000000L

./arch/powerpc/include/asm/nohash/32/pgtable.h:34:0: warning: 
"PGD_TABLE_SIZE" redefined
  #define PGD_TABLE_SIZE (sizeof(pgd_t) << PGD_INDEX_SIZE)
./include/generated/asm-offsets.h:101:0: note: this is the location of 
the previous definition
  #define PGD_TABLE_SIZE 256 /* PGD_TABLE_SIZE */

...

In v2, I had a patch to fix those redundancies 
(https://patchwork.ozlabs.org/patch/974363/) but I found it unconvenient.

> 
> 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.

Thanks for reviewing it.

> 
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> 

Christophe