lkml.org 
[lkml]   [2018]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH v3 3/7] powerpc: Activate CONFIG_THREAD_INFO_IN_TASK
From
Date


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

\
 
 \ /
  Last update: 2018-10-03 07:47    [W:0.047 / U:2.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site