Messages in this thread |  | | Date | Wed, 3 Oct 2018 16:23:50 +1000 | From | Nicholas Piggin <> | Subject | Re: [RFC PATCH v3 3/7] powerpc: Activate CONFIG_THREAD_INFO_IN_TASK |
| |
On Wed, 3 Oct 2018 08:04:49 +0200 Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> Le 03/10/2018 à 07:52, Nicholas Piggin a écrit : > > On Wed, 3 Oct 2018 07:47:05 +0200 > > Christophe LEROY <christophe.leroy@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@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.
Yeah that's true. I guess this is okay for a one-off hack.
Thanks, Nick
|  |