linux-kernel - Re: [PATCH V10 02/28] powerpc, process: Add the function flush_tmregs_to

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]

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.