[PATCH] ppc64: allow ptrace to set TM bits

[PATCH] ppc64: allow ptrace to set TM bits

[Laurent Dufour]

This patch allows the MSR bits relative to the Transactional memory
state to be manipulated through the ptrace API.

However, in the case the TM available bit is not set in the
manipulated MSR, the changes are ignored.

When dealing with the checkpointed MSR, we must be sure that the TM
state bits will not be set since the checkpointed state can't be a
transactional one.

This patch is a follow up of the Anshuman's series pushed by Simon
Guo recently, titled "Add new powerpc specific ELF core notes" :
https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-July/146711.html

Cc: Simon Guo <wei.guo.simon@gmail.com>
Cc: Anshuman Khandual <khandual@linux.vnet.ibm.com>
Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 1d8998bd6321..e2c16eb0cabe 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -161,8 +161,12 @@ const char *regs_query_register_name(unsigned int offset)
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
 #define MSR_DEBUGCHANGE	0
 #else
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+#define MSR_DEBUGCHANGE	(MSR_TS_MASK | MSR_SE | MSR_BE)
+#else
 #define MSR_DEBUGCHANGE	(MSR_SE | MSR_BE)
 #endif
+#endif
 
 /*
  * Max register writeable via put_reg
@@ -180,6 +184,12 @@ static unsigned long get_user_msr(struct task_struct *task)
 
 static int set_user_msr(struct task_struct *task, unsigned long msr)
 {
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+	if (!(task->thread.regs->msr & MSR_TM)) {
+		/* If TM is not available, discard TM bits changes */
+		msr &= ~(MSR_TM | MSR_TS_MASK);
+	}
+#endif
 	task->thread.regs->msr &= ~MSR_DEBUGCHANGE;
 	task->thread.regs->msr |= msr & MSR_DEBUGCHANGE;
 	return 0;
@@ -193,6 +203,7 @@ static unsigned long get_user_ckpt_msr(struct task_struct *task)
 
 static int set_user_ckpt_msr(struct task_struct *task, unsigned long msr)
 {
+	msr &= ~MSR_TS_MASK; /* Checkpoint state can't be in transaction */
 	task->thread.ckpt_regs.msr &= ~MSR_DEBUGCHANGE;
 	task->thread.ckpt_regs.msr |= msr & MSR_DEBUGCHANGE;
 	return 0;
-- 
2.7.4

Re: [PATCH] ppc64: allow ptrace to set TM bits

[Simon Guo]

Hi Laurent,
On Fri, Jul 29, 2016 at 11:51:22AM +0200, Laurent Dufour wrote:
>  static int set_user_msr(struct task_struct *task, unsigned long msr)
>  {
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +	if (!(task->thread.regs->msr & MSR_TM)) {
> +		/* If TM is not available, discard TM bits changes */
> +		msr &= ~(MSR_TM | MSR_TS_MASK);
> +	}
> +#endif

I am not sure whether following is an issue:
Per PowerISA, any exception/interrupt will disable MSR[TM] bit 
automatically and mark MSR_TS to be suspended when it is 
transactional. It is possible that MSR[TM] = 0 and MSR[MSR_TS] != 0
(suspended). 

Will set_user_msr() be able to escape from the above?
 For example, one user space application encountered 
page fault during transaction, its task->thread.regs->msr & MSR_TM == 0
and MSR[MSR_TS] == suspended.  Then it is being traced and 
set_user_msr() is invoked on it. I think it will be incorrect to 
clear its  MSR_TS_MASK bits.....

Please correct me if I am wrong.

Thanks,
- Simon

Re: [PATCH] ppc64: allow ptrace to set TM bits

[Laurent Dufour]

On 02/08/2016 07:43, Simon Guo wrote:
> Hi Laurent,
> On Fri, Jul 29, 2016 at 11:51:22AM +0200, Laurent Dufour wrote:
>>  static int set_user_msr(struct task_struct *task, unsigned long msr)
>>  {
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> +	if (!(task->thread.regs->msr & MSR_TM)) {
>> +		/* If TM is not available, discard TM bits changes */
>> +		msr &= ~(MSR_TM | MSR_TS_MASK);
>> +	}
>> +#endif
> 
> I am not sure whether following is an issue:
> Per PowerISA, any exception/interrupt will disable MSR[TM] bit 
> automatically and mark MSR_TS to be suspended when it is 
> transactional. It is possible that MSR[TM] = 0 and MSR[MSR_TS] != 0
> (suspended). 
> 
> Will set_user_msr() be able to escape from the above?
>  For example, one user space application encountered 
> page fault during transaction, its task->thread.regs->msr & MSR_TM == 0
> and MSR[MSR_TS] == suspended.  Then it is being traced and 
> set_user_msr() is invoked on it. I think it will be incorrect to 
> clear its  MSR_TS_MASK bits.....

You raised a good point, but I'm not sure this case is valid.
The interrupt case you described is happening in the kernel, not in user
space, and set_user_msr() is dealing with the user space register's
state, not the kernel's one.
So it can't apply and I can't see how in user space MSR[TM]=0 and
MSR[TS]=1 could happen.

Am I missing something here ?

Thanks,
Laurent.

Re: [PATCH] ppc64: allow ptrace to set TM bits

[Cyril Bur]

On Tue, 2016-08-02 at 13:43 +0800, Simon Guo wrote:
> Hi Laurent,
> On Fri, Jul 29, 2016 at 11:51:22AM +0200, Laurent Dufour wrote:
> > 
> >  static int set_user_msr(struct task_struct *task, unsigned long
> > msr)
> >  {
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > +	if (!(task->thread.regs->msr & MSR_TM)) {
> > +		/* If TM is not available, discard TM bits changes
> > */
> > +		msr &= ~(MSR_TM | MSR_TS_MASK);
> > +	}
> > +#endif
> 
> I am not sure whether following is an issue:
> Per PowerISA, any exception/interrupt will disable MSR[TM] bit 
> automatically and mark MSR_TS to be suspended when it is 
> transactional. It is possible that MSR[TM] = 0 and MSR[MSR_TS] != 0
> (suspended). 
> 
> Will set_user_msr() be able to escape from the above?
>  For example, one user space application encountered 
> page fault during transaction, its task->thread.regs->msr & MSR_TM ==
> 0
> and MSR[MSR_TS] == suspended.  Then it is being traced and 
> set_user_msr() is invoked on it. I think it will be incorrect to 
> clear its  MSR_TS_MASK bits.....
> 
> (suspended).ible that MSRTM] = 0 and MSR[MSR_TS] != 0> (suspended).

> Please correct me if I am wrong.

I'm not very familiar with ptracing and exactly what can happen but I
agree with Simon. Trying to change an MSR with that possible condition
stated ("It is possible that MSR[TM] = 0 and MSR[MSR_TS] !=
0> (suspended)") to MSR_TS and MSR_TS_MASK bits all 0 will cause a Bad
Thing.

Cyril

> 
> Thanks,
> - Simon
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ppc64: allow ptrace to set TM bits

[Laurent Dufour]

On 22/08/2016 03:01, Cyril Bur wrote:
> On Tue, 2016-08-02 at 13:43 +0800, Simon Guo wrote:
>> Hi Laurent,
>> On Fri, Jul 29, 2016 at 11:51:22AM +0200, Laurent Dufour wrote:
>>>
>>>  static int set_user_msr(struct task_struct *task, unsigned long
>>> msr)
>>>  {
>>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>>> +	if (!(task->thread.regs->msr & MSR_TM)) {
>>> +		/* If TM is not available, discard TM bits changes
>>> */
>>> +		msr &= ~(MSR_TM | MSR_TS_MASK);
>>> +	}
>>> +#endif
>>
>> I am not sure whether following is an issue:
>> Per PowerISA, any exception/interrupt will disable MSR[TM] bit 
>> automatically and mark MSR_TS to be suspended when it is 
>> transactional. It is possible that MSR[TM] = 0 and MSR[MSR_TS] != 0
>> (suspended). 
>>
>> Will set_user_msr() be able to escape from the above?
>>  For example, one user space application encountered 
>> page fault during transaction, its task->thread.regs->msr & MSR_TM ==
>> 0
>> and MSR[MSR_TS] == suspended.  Then it is being traced and 
>> set_user_msr() is invoked on it. I think it will be incorrect to 
>> clear its  MSR_TS_MASK bits.....
>>
>> (suspended).ible that MSRTM] = 0 and MSR[MSR_TS] != 0> (suspended).
> 
>> Please correct me if I am wrong.
> 
> I'm not very familiar with ptracing and exactly what can happen but I
> agree with Simon. Trying to change an MSR with that possible condition
> stated ("It is possible that MSR[TM] = 0 and MSR[MSR_TS] !=
> 0> (suspended)") to MSR_TS and MSR_TS_MASK bits all 0 will cause a Bad
> Thing.

I don't think this may happen since from the user space point of view,
we can't have MSR[TM]=0 & MSR[MSR_TS]=1, because MSR[TM] will be set
once the transaction is initiated.

Anyway, this patch is no more required since the recent patch
http://patchwork.ozlabs.org/patch/661358/ is now dropping the TM state
form the signal return path, which is enough fof CRIU to work fine.

Cheers,
Laurent.