linux-kernel - Re: [RFC PATCH v3 3/7] powerpc: Activate CONFIG_THREAD_INFO_IN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181003155243.70806497@roar.ozlabs.ibm.com>
Date:   Wed, 3 Oct 2018 15:52:43 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     Christophe LEROY <christophe.leroy@....fr>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        aneesh.kumar@...ux.vnet.ibm.com, linux-kernel@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: [RFC PATCH v3 3/7] powerpc: Activate CONFIG_THREAD_INFO_IN_TASK

On Wed, 3 Oct 2018 07:47:05 +0200
Christophe LEROY <christophe.leroy@....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@....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?

Thanks,
Nick