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
| ||
|
Date: Wed, 02 Mar 2016 09:59:06 +0530 From: Anshuman Khandual <khandual@...ux.vnet.ibm.com> To: Cyril Bur <cyrilbur@...il.com> CC: linuxppc-dev@...abs.org, linux-kernel@...r.kernel.org, Michael Neuling <mikey@...ling.org> Subject: Re: [PATCH V10 02/28] powerpc, process: Add the function flush_tmregs_to_thread On 03/02/2016 05:45 AM, Cyril Bur wrote: > On Tue, 16 Feb 2016 14:29:32 +0530 > Anshuman Khandual <khandual@...ux.vnet.ibm.com> wrote: > >> This patch creates a function flush_tmregs_to_thread which >> will then be used by subsequent patches in this series. The >> function checks for self tracing ptrace interface attempts >> while in the TM context and logs appropriate warning message. >> > > Hi Anshuman, > > You'll have to bare with me, my ptrace knowledge is non existent so you might > have to walk me though some aspects. > > I have been playing with FPU/VMX and VSX saving so I thought I'd take a look. Sure. > >> Signed-off-by: Anshuman Khandual <khandual@...ux.vnet.ibm.com> >> --- >> arch/powerpc/include/asm/switch_to.h | 8 ++++++++ >> arch/powerpc/kernel/process.c | 20 ++++++++++++++++++++ >> 2 files changed, 28 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h >> index 5b268b6..7b297bf 100644 >> --- a/arch/powerpc/include/asm/switch_to.h >> +++ b/arch/powerpc/include/asm/switch_to.h >> @@ -70,6 +70,14 @@ static inline void disable_kernel_spe(void) >> } >> #endif >> >> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM >> +extern void flush_tmregs_to_thread(struct task_struct *); >> +#else >> +static inline void flush_tmregs_to_thread(struct task_struct *t) >> +{ >> +} >> +#endif >> + >> static inline void clear_task_ebb(struct task_struct *t) >> { >> #ifdef CONFIG_PPC_BOOK3S_64 >> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c >> index dccc87e..2c4fa7f 100644 >> --- a/arch/powerpc/kernel/process.c >> +++ b/arch/powerpc/kernel/process.c >> @@ -918,6 +918,26 @@ static inline void restore_sprs(struct thread_struct *old_thread, >> #endif >> } >> > > > >> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM >> +void flush_tmregs_to_thread(struct task_struct *tsk) >> +{ >> + /* >> + * Process self tracing is not yet supported through >> + * ptrace interface. Ptrace generic code should have >> + * prevented this from happening in the first place. >> + * Warn once here with the message, if some how it >> + * is attempted. >> + */ >> + WARN_ONCE(tsk == current, >> + "Not expecting ptrace on self: TM regs may be incorrect\n"); >> + >> + /* >> + * If task is not current, it should have been flushed >> + * already to it's thread_struct during __switch_to(). >> + */ > > I totally agree except this highlights something that I notice in subsequent > patches, and existing code. All the *_{get,set}() functions call > flush_*_to_thread() when, as per your comment (and my understanding of task > switching) there really shouldn't be a need to do that. My only thought is that > this could be a relic of uniprocessor days when it would have been necessary but > Anton recently stripped that out. Are you able to shed some light here? Its been sometime I had looked into this aspect of the series. I remember Michael Neuling and myself discussed about this and settled on a single WARN_ON here as nothing else was required to be done in the function. It may be possible that all the flush_*_to_thread() functions used else where are because of uniprocessor concerns. I dont understand completely our context save/restore paths including the lazy ones. I believed that these flush_*_to_thread() routines just made sure task struct has the latest values of the thread context in case of some complicated save/restore paths might not have done the complete save at that point in time. If you think that all these flush_*_to_thread() functions used through out POWER ptrace need review to see whether they are required or not anymore I would suggest we should do it as a separate patch after this series and I am willing to work with you on that. > > The reason I ask is that if the flush_*_to_thread() calls ARE actually > important then I worry that this function is inadequate... I guess we went through that and finally settled on WARN_ON once but dont remember the exact context now. Will look into all previous discussions on this and get back.
Powered by blists - more mailing lists