[PATCH v4] powerpc: Export thread_struct.used_vr/used_vsr to user space

[PATCH v4] powerpc: Export thread_struct.used_vr/used_vsr to user space

[wei.guo.simon]

From: Simon Guo <wei.guo.simon@gmail.com>

These 2 fields track whether user process has used Altivec/VSX
registers or not. They are used by kernel to setup signal frame
on user stack correctly regarding vector part.

CRIU(Checkpoint and Restore In User space) builds signal frame
for restored process. It will need this export information to
setup signal frame correctly. And CRIU will need to restore these
2 fields for the restored process.

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Rashmica Gupta <rashmicy@gmail.com>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
Cc: Laurent Dufour <ldufour@linux.vnet.ibm.com>
Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
Reviewed-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
--

v3 -> v4:
-  Copying 64 bit data from/to user space using get_user/put_user
is not supported on all architectures, and may result in the
following build error on 32 bits build:
   arch/powerpc/kernel/built-in.o: In function `arch_ptrace':
   >> (.text+0xad4): undefined reference to `__get_user_bad'
using copy_from_user/copy_to_user instead.

v2 -> v3:
- enlarge reg_usage from 32 to 64 bits
- prefix ptrace API with PPC_

v1 -> v2:
- minor change for coding style
---
 arch/powerpc/include/uapi/asm/ptrace.h | 11 ++++++++
 arch/powerpc/kernel/ptrace.c           | 48 ++++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/ptrace32.c         |  2 ++
 3 files changed, 61 insertions(+)

diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
index 8036b38..b357677 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -176,6 +176,17 @@ struct pt_regs {
 #define PTRACE_GETREGS64	  0x16
 #define PTRACE_SETREGS64	  0x17
 
+/*
+ * Get or set some register used bit.
+ * The flags will be saved in a 64 bit data.
+ * Currently it is only used for VR/VSR usage.
+ */
+#define PPC_PTRACE_GET_REGS_USAGE	  0x97
+#define PPC_PTRACE_SET_REGS_USAGE	  0x96
+
+#define PPC_PTRACE_REGS_USAGE_VR_BIT  0x01UL
+#define PPC_PTRACE_REGS_USAGE_VSR_BIT 0x02UL
+
 /* Calls to trace a 64bit program from a 32bit program */
 #define PPC_PTRACE_PEEKTEXT_3264 0x95
 #define PPC_PTRACE_PEEKDATA_3264 0x94
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index a9aa2a5..d00d7c0 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -3018,6 +3018,54 @@ long arch_ptrace(struct task_struct *child, long request,
 					     REGSET_SPE, 0, 35 * sizeof(u32),
 					     datavp);
 #endif
+	case PPC_PTRACE_GET_REGS_USAGE:
+		{
+			u64 *u64_datap = (u64 *)datavp;
+			u64 reg_usage = 0;
+
+			if (addr != sizeof(u64))
+				return -EINVAL;
+
+#ifdef CONFIG_ALTIVEC
+			if (child->thread.used_vr)
+				reg_usage |= PPC_PTRACE_REGS_USAGE_VR_BIT;
+#endif
+#ifdef CONFIG_VSX
+			if (child->thread.used_vsr)
+				reg_usage |= PPC_PTRACE_REGS_USAGE_VSR_BIT;
+#endif
+			ret =  copy_to_user(u64_datap,
+					&reg_usage,
+					sizeof(reg_usage)) ?
+				-EFAULT : 0;
+			break;
+		}
+
+	case PPC_PTRACE_SET_REGS_USAGE:
+		{
+			u64 *u64_datap = (u64 *)datavp;
+			u64 reg_usage = 0;
+
+			if (addr != sizeof(u64))
+				return -EINVAL;
+
+			ret = copy_from_user(&reg_usage,
+					u64_datap,
+					sizeof(reg_usage)) ?
+				-EFAULT : 0;
+
+			if (ret)
+				return ret;
+#ifdef CONFIG_ALTIVEC
+			child->thread.used_vr =
+				!!(reg_usage & PPC_PTRACE_REGS_USAGE_VR_BIT);
+#endif
+#ifdef CONFIG_VSX
+			child->thread.used_vsr =
+				!!(reg_usage & PPC_PTRACE_REGS_USAGE_VSR_BIT);
+#endif
+			break;
+		}
 
 	default:
 		ret = ptrace_request(child, request, addr, data);
diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c
index f52b7db..3aaa773 100644
--- a/arch/powerpc/kernel/ptrace32.c
+++ b/arch/powerpc/kernel/ptrace32.c
@@ -305,6 +305,8 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 	case PPC_PTRACE_GETHWDBGINFO:
 	case PPC_PTRACE_SETHWDEBUG:
 	case PPC_PTRACE_DELHWDEBUG:
+	case PPC_PTRACE_GET_REGS_USAGE:
+	case PPC_PTRACE_SET_REGS_USAGE:
 		ret = arch_ptrace(child, request, addr, data);
 		break;
 
-- 
1.8.3.1

Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space

[Michael Ellerman]

On Thu, 2016-07-07 at 07:49:36 UTC, Simon Guo wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
> 
> These 2 fields track whether user process has used Altivec/VSX
> registers or not. They are used by kernel to setup signal frame
> on user stack correctly regarding vector part.

I still dislike this. It's just exporting internal kernel state, which I know is
the point.

And I'd still like to know why we're the only arch that needs to do this.

I'm not saying I won't merge it, but I'd like to understand it better first.

> CRIU(Checkpoint and Restore In User space) builds signal frame
> for restored process. It will need this export information to
> setup signal frame correctly. And CRIU will need to restore these
> 2 fields for the restored process.

I don't really know how CRIU works, but ..

Does the kernel write a sigframe for the process that's being checkpointed? If
so can't you infer the state of the bits based on what was written?

Alternately, when restoring, can you setup the sigframe with the Altivec/VSX
fields populated, and the kernel will then load them, regardless of whether
they were actually used or not prior to the checkpoint?

cheers

Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space

[Laurent Dufour]

On 07/07/2016 13:15, Michael Ellerman wrote:
> On Thu, 2016-07-07 at 07:49:36 UTC, Simon Guo wrote:
>> From: Simon Guo <wei.guo.simon@gmail.com>
>>
>> These 2 fields track whether user process has used Altivec/VSX
>> registers or not. They are used by kernel to setup signal frame
>> on user stack correctly regarding vector part.
> 
> I still dislike this. It's just exporting internal kernel state, which I know is
> the point.
> 
> And I'd still like to know why we're the only arch that needs to do this.
> 
> I'm not saying I won't merge it, but I'd like to understand it better first.
> 
>> CRIU(Checkpoint and Restore In User space) builds signal frame
>> for restored process. It will need this export information to
>> setup signal frame correctly. And CRIU will need to restore these
>> 2 fields for the restored process.
> 
> I don't really know how CRIU works, but ..
> 
> Does the kernel write a sigframe for the process that's being checkpointed? If
> so can't you infer the state of the bits based on what was written?

Hi Michael,

Basically, CRIU checkpoints the process register's state through the
ptrace API, and it restores it through a signal frame at restart time.
This is quite odd but that the way it works on all the CRIU's supported
architectures.

Obviously everything is done from/in user space, so the sigframe
building too.
Since we can't know from user space if the thread has used or not the
Altivec/VSX registers, since we can't rely on the MSR bits, we always
dump these registers.

> Alternately, when restoring, can you setup the sigframe with the Altivec/VSX
> fields populated, and the kernel will then load them, regardless of whether
> they were actually used or not prior to the checkpoint?

In the case of Altivec/VSX fields, we currently force the kernel to
retrieve them from the signal frame by setting MSR_VEC/MSR_VSX so
restore_sigcontext() will copy them to the kernel thread's state.
However this doesn't touch to used_vsr and used_vr which may remain at 0.

Most of the time this is fine, but in the case a thread which has really
used those registers is catching a signal just after the restore and
before it has touched to these registers again (and so set used_vsr/vr),
these registers will not be pushed in the newly built signal frame since
setup_sigcontext() check for used_vsr/vr before pushing the registers on
the stack.
This may be an issue in the case the thread wants to changed those
registers (don't ask me why :)) in the stacked signal frame from the
signal handler since they will not be there...

Being able to get and set the used_vr and used_vsr thread's variables,
fixes this issue.

Cheers,
Laurent.

Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space

[Benjamin Herrenschmidt]

On Thu, 2016-07-07 at 15:12 +0200, Laurent Dufour wrote:
> 
> Basically, CRIU checkpoints the process register's state through the
> ptrace API, and it restores it through a signal frame at restart time.
> This is quite odd but that the way it works on all the CRIU's supported
> architectures.
> 
> Obviously everything is done from/in user space, so the sigframe
> building too.
> Since we can't know from user space if the thread has used or not the
> Altivec/VSX registers, since we can't rely on the MSR bits, we always
> dump these registers.

Right, however is that an issue ? These days with glibc using V{M,S}X
for things like memcpy I would think there is little to gain in trying
to avoid dumping them.

> > Alternately, when restoring, can you setup the sigframe with the Altivec/VSX
> > fields populated, and the kernel will then load them, regardless of whether
> > they were actually used or not prior to the checkpoint?
> 
> In the case of Altivec/VSX fields, we currently force the kernel to
> retrieve them from the signal frame by setting MSR_VEC/MSR_VSX so
> restore_sigcontext() will copy them to the kernel thread's state.

Yup, that's the way to go.

> However this doesn't touch to used_vsr and used_vr which may remain at 0.

That would be a kernel bug.

> Most of the time this is fine, but in the case a thread which has really
> used those registers is catching a signal just after the restore and
> before it has touched to these registers again (and so set used_vsr/vr),
> these registers will not be pushed in the newly built signal frame since
> setup_sigcontext() check for used_vsr/vr before pushing the registers on
> the stack.
> This may be an issue in the case the thread wants to changed those
> registers (don't ask me why :)) in the stacked signal frame from the
> signal handler since they will not be there...
> 
> Being able to get and set the used_vr and used_vsr thread's variables,
> fixes this issue.

I think the right fix is that if a restore_sigcontext() has the MSR bits set,
it should set the corresponding used_* flag.

Or is there a reason why that won't work ?

Cheers,
Ben.

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

Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space

[Benjamin Herrenschmidt]

On Thu, 2016-07-07 at 23:21 +1000, Benjamin Herrenschmidt wrote:
> 
> I think the right fix is that if a restore_sigcontext() has the MSR
> bits set,
> it should set the corresponding used_* flag.

Something like this:

(totally untested)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index b6aa378..1bf074e 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -698,6 +698,7 @@ static long restore_user_regs(struct pt_regs *regs,
 		if (__copy_from_user(&current->thread.vr_state, &sr->mc_vregs,
 				     sizeof(sr->mc_vregs)))
 			return 1;
+		current->thread.used_vr = true;
 	} else if (current->thread.used_vr)
 		memset(&current->thread.vr_state, 0,
 		       ELF_NVRREG * sizeof(vector128));
@@ -724,6 +725,7 @@ static long restore_user_regs(struct pt_regs *regs,
 		 */
 		if (copy_vsx_from_user(current, &sr->mc_vsregs))
 			return 1;
+		current->thread.used_vsr = true;
 	} else if (current->thread.used_vsr)
 		for (i = 0; i < 32 ; i++)
 			current->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = 0;
@@ -743,6 +745,7 @@ static long restore_user_regs(struct pt_regs *regs,
 		if (__copy_from_user(current->thread.evr, &sr->mc_vregs,
 				     ELF_NEVRREG * sizeof(u32)))
 			return 1;
+		current->thread.used_spe = true;
 	} else if (current->thread.used_spe)
 		memset(current->thread.evr, 0, ELF_NEVRREG * sizeof(u32));
 
@@ -799,6 +802,7 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 				     &tm_sr->mc_vregs,
 				     sizeof(sr->mc_vregs)))
 			return 1;
+		current->thread.used_vr = true;
 	} else if (current->thread.used_vr) {
 		memset(&current->thread.vr_state, 0,
 		       ELF_NVRREG * sizeof(vector128));
@@ -832,6 +836,7 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 		if (copy_vsx_from_user(current, &sr->mc_vsregs) ||
 		    copy_transact_vsx_from_user(current, &tm_sr->mc_vsregs))
 			return 1;
+		current->thread.used_vsr = true;
 	} else if (current->thread.used_vsr)
 		for (i = 0; i < 32 ; i++) {
 			current->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = 0;
@@ -848,6 +853,7 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 		if (__copy_from_user(current->thread.evr, &sr->mc_vregs,
 				     ELF_NEVRREG * sizeof(u32)))
 			return 1;
+		current->thread.used_spe = true;
 	} else if (current->thread.used_spe)
 		memset(current->thread.evr, 0, ELF_NEVRREG * sizeof(u32));
 
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 2552079..8704269 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -363,9 +363,11 @@ static long restore_sigcontext(struct pt_regs *regs, sigset_t *set, int sig,
 	if (v_regs && !access_ok(VERIFY_READ, v_regs, 34 * sizeof(vector128)))
 		return -EFAULT;
 	/* Copy 33 vec registers (vr0..31 and vscr) from the stack */
-	if (v_regs != NULL && (msr & MSR_VEC) != 0)
+	if (v_regs != NULL && (msr & MSR_VEC) != 0) {
 		err |= __copy_from_user(&current->thread.vr_state, v_regs,
 					33 * sizeof(vector128));
+		current->thread.used_vr = true;
+	}
 	else if (current->thread.used_vr)
 		memset(&current->thread.vr_state, 0, 33 * sizeof(vector128));
 	/* Always get VRSAVE back */
@@ -385,9 +387,10 @@ static long restore_sigcontext(struct pt_regs *regs, sigset_t *set, int sig,
 	 * buffer for formatting, then into the taskstruct.
 	 */
 	v_regs += ELF_NVRREG;
-	if ((msr & MSR_VSX) != 0)
+	if ((msr & MSR_VSX) != 0) {
 		err |= copy_vsx_from_user(current, v_regs);
-	else
+		current->thread.used_vsr = true;
+	} else
 		for (i = 0; i < 32 ; i++)
 			current->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = 0;
 #endif
@@ -482,6 +485,7 @@ static long restore_tm_sigcontexts(struct pt_regs *regs,
 					33 * sizeof(vector128));
 		err |= __copy_from_user(&current->thread.transact_vr, tm_v_regs,
 					33 * sizeof(vector128));
+		current->thread.used_vr = true;
 	}
 	else if (current->thread.used_vr) {
 		memset(&current->thread.vr_state, 0, 33 * sizeof(vector128));
@@ -515,6 +519,7 @@ static long restore_tm_sigcontexts(struct pt_regs *regs,
 		tm_v_regs += ELF_NVRREG;
 		err |= copy_vsx_from_user(current, v_regs);
 		err |= copy_transact_vsx_from_user(current, tm_v_regs);
+		current->thread.used_vsr = true;
 	} else {
 		for (i = 0; i < 32 ; i++) {
 			current->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = 0;

Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space

[Laurent Dufour]

On 07/07/2016 15:21, Benjamin Herrenschmidt wrote:
> On Thu, 2016-07-07 at 15:12 +0200, Laurent Dufour wrote:
>>
>> Basically, CRIU checkpoints the process register's state through the
>> ptrace API, and it restores it through a signal frame at restart time.
>> This is quite odd but that the way it works on all the CRIU's supported
>> architectures.
>>
>> Obviously everything is done from/in user space, so the sigframe
>> building too.
>> Since we can't know from user space if the thread has used or not the
>> Altivec/VSX registers, since we can't rely on the MSR bits, we always
>> dump these registers.
> 
> Right, however is that an issue ? These days with glibc using V{M,S}X
> for things like memcpy I would think there is little to gain in trying
> to avoid dumping them.
> 
>>> Alternately, when restoring, can you setup the sigframe with the Altivec/VSX
>>> fields populated, and the kernel will then load them, regardless of whether
>>> they were actually used or not prior to the checkpoint?
>>
>> In the case of Altivec/VSX fields, we currently force the kernel to
>> retrieve them from the signal frame by setting MSR_VEC/MSR_VSX so
>> restore_sigcontext() will copy them to the kernel thread's state.
> 
> Yup, that's the way to go.
> 
>> However this doesn't touch to used_vsr and used_vr which may remain at 0.
> 
> That would be a kernel bug.
> 
>> Most of the time this is fine, but in the case a thread which has really
>> used those registers is catching a signal just after the restore and
>> before it has touched to these registers again (and so set used_vsr/vr),
>> these registers will not be pushed in the newly built signal frame since
>> setup_sigcontext() check for used_vsr/vr before pushing the registers on
>> the stack.
>> This may be an issue in the case the thread wants to changed those
>> registers (don't ask me why :)) in the stacked signal frame from the
>> signal handler since they will not be there...
>>
>> Being able to get and set the used_vr and used_vsr thread's variables,
>> fixes this issue.
> 
> I think the right fix is that if a restore_sigcontext() has the MSR bits set,
> it should set the corresponding used_* flag.
> 
> Or is there a reason why that won't work ?

I got your point and I agree that most of the time now, the Altivec/VSX
registers are used by libc. In that case is there still a need for the
lazy Altivec/VSX registers dump in the signal frame ?

I'm fine with your proposal, except that every restarted process will
have the used_vr/used_vsx turned on after the restart since we can't
check if these registers were used or not at checkpoint time.
But that may be a minor point...

Cheers,
Laurent.

Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space

[Simon Guo]

On Thu, Jul 07, 2016 at 11:21:18PM +1000, Benjamin Herrenschmidt wrote:
> I think the right fix is that if a restore_sigcontext() has the MSR bits set,
> it should set the corresponding used_* flag.
> 
> Or is there a reason why that won't work ?

That sounds reaonable to me.
I will prepare a patch based on that.

Michael, Ben, Laurent,
Thanks the discussion and proposal.

- Simon

Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space

[Simon Guo]

On Fri, Jul 08, 2016 at 08:02:42PM +1000, Michael Ellerman wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > On Thu, 2016-07-07 at 23:21 +1000, Benjamin Herrenschmidt wrote:
> >> 
> >> I think the right fix is that if a restore_sigcontext() has the MSR
> >> bits set,
> >> it should set the corresponding used_* flag.
> >
> > Something like this:
> >
> > (totally untested)
> 
> Simon/Laurent, can you guys test this and let me know if it works for
> your usecase.

Just notice this note.
Yes I will test it on CRIU and update later.

Thanks,
- Simon

Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space

[Simon Guo]

Michael, Ben,
On Fri, Jul 08, 2016 at 08:02:42PM +1000, Michael Ellerman wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > On Thu, 2016-07-07 at 23:21 +1000, Benjamin Herrenschmidt wrote:
> >> 
> >> I think the right fix is that if a restore_sigcontext() has the MSR
> >> bits set,
> >> it should set the corresponding used_* flag.
> >
> > Something like this:
> >
> > (totally untested)
> 
> Simon/Laurent, can you guys test this and let me know if it works for
> your usecase.
> 

Sorry for the late reply. I tested Ben's patch and the fix works for the CRIU
case mentioned before.

Thanks,
Simon

Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space

[Simon Guo]

On Thu, Jul 21, 2016 at 08:57:29PM +1000, Michael Ellerman wrote:
> Can one of you send a properly formatted and signed-off patch.

I will work on that.

Thanks,
Simon