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

[Christophe LEROY]

Le 03/10/2018 à 07:52, Nicholas Piggin a écrit :
> On Wed, 3 Oct 2018 07:47:05 +0200
> Christophe LEROY <christophe.leroy at c-s.fr> wrote:
> 
>> Le 03/10/2018 à 07:30, Nicholas Piggin a écrit :
>>> On Mon,  1 Oct 2018 12:30:23 +0000 (UTC)
>>> Christophe Leroy <christophe.leroy at 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 ?
> 
> Yes, good idea.
> 
>>>> @@ -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 */
>>
>> ...
> 
> Okay.
> 
>>
>> In v2, I had a patch to fix those redundancies
>> (https://patchwork.ozlabs.org/patch/974363/) but I found it unconvenient.
> 
> Because of merge conflicts, or you did not like the new names?

Both, because of the amount of changes it implies, and also because of 
the new names. I find it quite convenient to be able to use same names 
both in C and ASM. And I didn't want my serie to imply big-bangs in 
unrelated or not directly related topics.

Christophe

> 
> Thanks,
> Nick
>