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

[Christophe LEROY]

Le 03/10/2018 Ã 07:30, Nicholas Piggin a ÃcritÂ:
> 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 :)

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@xxxxxxxxx>

Christophe