[uml-devel] um: PTRACE_SETREGSET failure with XSTATE on Kabylake CPU

[Thomas Meyer]

Hi,

I finally did figure out where in the host kernel the ptrace syscall
fails with -EFAULT.

In arch/x86/kernel/fpu/regset.c:130:

114 int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
115 unsigned int pos, unsigned int count,
116 const void *kbuf, const void __user *ubuf)
117 {
118 struct fpu *fpu = &target->thread.fpu;
119 struct xregs_state *xsave;
120 int ret;
121
122 if (!boot_cpu_has(X86_FEATURE_XSAVE))
123 return -ENODEV;
124
125 pr_info("in xstateregs_set");
126
127 /*
128 * A whole standard-format XSAVE buffer is needed:
129 */
130 if ((pos != 0) || (count < fpu_user_xstate_size)) {
131 pr_info("EFAULT from xstateregs_set");
132-> pr_info("pos = %i, count = %i, fpu_user_xstate_size= %i\n", pos, count, fpu_user_xstate_size);
133 return -EFAULT;
134 }

Sadly I had to fallback to debugging by printk because kgdb/qemu
gdbstub, all didn't work for some unknown reason :-(

output is:
[ 69.598349] EFAULT from xstateregs_set
[ 69.598350] pos = 0, count = 832, fpu_user_xstate_size= 1088

calling code is in arch/x86/um/os-Linux/registers.c:

49 int restore_fp_registers(int pid, unsigned long *fp_regs)
50 {
51 struct iovec iov;
52
53 if (have_xstate_support) {
54 iov.iov_base = fp_regs;
55 iov.iov_len = sizeof(struct _xstate);
56 if (ptrace(PTRACE_SETREGSET, pid, NT_X86_XSTATE, &iov) < 0)
57 -> return -errno;
58 return 0;
59 } else {
60 return restore_i387_registers(pid, fp_regs);
61 }
62 }

it looks like _xstate is too short for above operation, I wonder why
PTRACE_GETREGSET works without a warning of too short size.

with kind regards
thomas

[Richard Weinberger]

Thomas,

Post by Thomas Meyer
Hi,
I finally did figure out where in the host kernel the ptrace syscall
fails with -EFAULT.

Nice! Thanks a lot for digging into this. I still had no chance to setup
Ipv6 to connect to your host and figure myself. ;-\

Post by Thomas Meyer
114 int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
115 unsigned int pos, unsigned int count,
116 const void *kbuf, const void __user *ubuf)
117 {
118 struct fpu *fpu = &target->thread.fpu;
119 struct xregs_state *xsave;
120 int ret;
121
122 if (!boot_cpu_has(X86_FEATURE_XSAVE))
123 return -ENODEV;
124
125 pr_info("in xstateregs_set");
126
127 /*
129 */
130 if ((pos != 0) || (count < fpu_user_xstate_size)) {
131 pr_info("EFAULT from xstateregs_set");
132-> pr_info("pos = %i, count = %i, fpu_user_xstate_size= %i\n", pos, count, fpu_user_xstate_size);
133 return -EFAULT;
134 }
Sadly I had to fallback to debugging by printk because kgdb/qemu
gdbstub, all didn't work for some unknown reason :-(

As always. printk is best debugger ever. ;-)

Post by Thomas Meyer
[ 69.598349] EFAULT from xstateregs_set
[ 69.598350] pos = 0, count = 832, fpu_user_xstate_size= 1088
49 int restore_fp_registers(int pid, unsigned long *fp_regs)
50 {
51 struct iovec iov;
52
53 if (have_xstate_support) {
54 iov.iov_base = fp_regs;
55 iov.iov_len = sizeof(struct _xstate);
56 if (ptrace(PTRACE_SETREGSET, pid, NT_X86_XSTATE, &iov) < 0)
57 -> return -errno;
58 return 0;
59 } else {
60 return restore_i387_registers(pid, fp_regs);
61 }
62 }
it looks like _xstate is too short for above operation, I wonder why
PTRACE_GETREGSET works without a warning of too short size.

Does PTRACE_GETREGSET return a size? Maybe we have to take this into account.
It could be that your host CPU has a smaller set.
Also check whether PTRACE_SETREGSET always fails.

Thanks,
//richard

[Thomas Meyer]

Post by Richard Weinberger
Thomas,

Post by Thomas Meyer
Hi,
I finally did figure out where in the host kernel the ptrace
syscall
fails with -EFAULT.

Nice! Thanks a lot for digging into this. I still had no chance to setup
Ipv6 to connect to your host and figure myself. ;-\

Post by Thomas Meyer
114 int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
115 unsigned int pos, unsigned int count,
116 const void *kbuf, const void __user *ubuf)
117 {
118 struct fpu *fpu = &target->thread.fpu;
119 struct xregs_state *xsave;
120 int ret;
121
122 if (!boot_cpu_has(X86_FEATURE_XSAVE))
123 return -ENODEV;
124
125 pr_info("in xstateregs_set");
126
127 /*
129 */
130 if ((pos != 0) || (count < fpu_user_xstate_size)) {
131 pr_info("EFAULT from xstateregs_set");
132-> pr_info("pos = %i, count = %i,
fpu_user_xstate_size= %i\n", pos, count, fpu_user_xstate_size);
133 return -EFAULT;
134 }
Sadly I had to fallback to debugging by printk because kgdb/qemu
gdbstub, all didn't work for some unknown reason :-(

As always. printk is best debugger ever. ;-)

Post by Thomas Meyer
[ 69.598349] EFAULT from xstateregs_set
[ 69.598350] pos = 0, count = 832, fpu_user_xstate_size= 1088
49 int restore_fp_registers(int pid, unsigned long *fp_regs)
50 {
51 struct iovec iov;
52
53 if (have_xstate_support) {
54 iov.iov_base = fp_regs;
55 iov.iov_len = sizeof(struct _xstate);
56 if (ptrace(PTRACE_SETREGSET, pid,
NT_X86_XSTATE, &iov) < 0)
57 -> return -errno;
58 return 0;
59 } else {
60 return restore_i387_registers(pid, fp_regs);
61 }
62 }
it looks like _xstate is too short for above operation, I wonder why
PTRACE_GETREGSET works without a warning of too short size.

Does PTRACE_GETREGSET return a size?

Yes, it returns 832. the size of struct _xstate.

Post by Richard Weinberger
Maybe we have to take this into account.
It could be that your host CPU has a smaller set.
Also check whether PTRACE_SETREGSET always fails.

In UML the first userspace ptrace always fails, so init get's killed.

The check "count < fpu_user_xstate_size" was introduced by commit:

commit 91c3dba7dbc199191272f4a9863f86ea3bfd679f
Author: Yu-cheng Yu <yu-***@intel.com>
Date: Fri Jun 17 13:07:17 2016 -0700

x86/fpu/xstate: Fix PTRACE frames for XSAVES

XSAVES uses compacted format and is a kernel instruction. The kernel
should use standard-format, non-supervisor state data for PTRACE.

So to summarize:

- PTRACE_GETREGSET with NT_X86_XSTATE gets 832 and return 832, with no
error.

- PTRACE_SETREGSET get 832 (sizeof struct _xstate) but wants at least
1088, otherwise it will fail with -EFAULT (why not -EINVAL?)

Ideas?

Post by Richard Weinberger
Thanks,
//richard

[Richard Weinberger]

[adding x86 folks]

Post by Thomas Meyer

Post by Richard Weinberger
Thomas,

Post by Thomas Meyer
Hi,
I finally did figure out where in the host kernel the ptrace
syscall
fails with -EFAULT.

Nice! Thanks a lot for digging into this. I still had no chance to setup
Ipv6 to connect to your host and figure myself. ;-\

Post by Thomas Meyer
114 int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
115 unsigned int pos, unsigned int count,
116 const void *kbuf, const void __user *ubuf)
117 {
118 struct fpu *fpu = &target->thread.fpu;
119 struct xregs_state *xsave;
120 int ret;
121
122 if (!boot_cpu_has(X86_FEATURE_XSAVE))
123 return -ENODEV;
124
125 pr_info("in xstateregs_set");
126
127 /*
129 */
130 if ((pos != 0) || (count < fpu_user_xstate_size)) {
131 pr_info("EFAULT from xstateregs_set");
132-> pr_info("pos = %i, count = %i,
fpu_user_xstate_size= %i\n", pos, count, fpu_user_xstate_size);
133 return -EFAULT;
134 }
Sadly I had to fallback to debugging by printk because kgdb/qemu
gdbstub, all didn't work for some unknown reason :-(

As always. printk is best debugger ever. ;-)

Post by Thomas Meyer
[ 69.598349] EFAULT from xstateregs_set
[ 69.598350] pos = 0, count = 832, fpu_user_xstate_size= 1088
49 int restore_fp_registers(int pid, unsigned long *fp_regs)
50 {
51 struct iovec iov;
52
53 if (have_xstate_support) {
54 iov.iov_base = fp_regs;
55 iov.iov_len = sizeof(struct _xstate);
56 if (ptrace(PTRACE_SETREGSET, pid,
NT_X86_XSTATE, &iov) < 0)
57 -> return -errno;
58 return 0;
59 } else {
60 return restore_i387_registers(pid, fp_regs);
61 }
62 }
it looks like _xstate is too short for above operation, I wonder why
PTRACE_GETREGSET works without a warning of too short size.

Does PTRACE_GETREGSET return a size?

Yes, it returns 832. the size of struct _xstate.

Post by Richard Weinberger
Maybe we have to take this into account.
It could be that your host CPU has a smaller set.
Also check whether PTRACE_SETREGSET always fails.

In UML the first userspace ptrace always fails, so init get's killed.
commit 91c3dba7dbc199191272f4a9863f86ea3bfd679f
Date: Fri Jun 17 13:07:17 2016 -0700
x86/fpu/xstate: Fix PTRACE frames for XSAVES
XSAVES uses compacted format and is a kernel instruction. The kernel
should use standard-format, non-supervisor state data for PTRACE.
- PTRACE_GETREGSET with NT_X86_XSTATE gets 832 and return 832, with no
error.
- PTRACE_SETREGSET get 832 (sizeof struct _xstate) but wants at least
1088, otherwise it will fail with -EFAULT (why not -EINVAL?)
Ideas?

Post by Richard Weinberger
Thanks,
//richard

[Richard Weinberger]

Yu-cheng,

Post by Thomas Meyer
- PTRACE_GETREGSET with NT_X86_XSTATE gets 832 and return 832, with no
error.
- PTRACE_SETREGSET get 832 (sizeof struct _xstate) but wants at least
1088, otherwise it will fail with -EFAULT (why not -EINVAL?)
Ideas?

We considered allowing a partial XSAVE buffer for PTRACE_SETREGSET, but
it was that the XSAVE instruction requires a full-size buffer led to
this choice. Using a smaller buffer for XSAVE causes a fault.

So, this code is not supposed to work?

iov.iov_base = fp_regs;
iov.iov_len = sizeof(struct _xstate);
ptrace(PTRACE_GETREGSET, pid, NT_X86_XSTATE, &iov);
ptrace(PTRACE_SETREGSET, pid, NT_X86_XSTATE, &iov);

This is what UML does and on Thomas's new Laptop PTRACE_SETREGSET is failing.

Thanks,
//richard

[Richard Weinberger]

Yu-cheng,

Post by Richard Weinberger
Yu-cheng,

Post by Thomas Meyer
- PTRACE_GETREGSET with NT_X86_XSTATE gets 832 and return 832, with no
error.
- PTRACE_SETREGSET get 832 (sizeof struct _xstate) but wants at least
1088, otherwise it will fail with -EFAULT (why not -EINVAL?)
Ideas?

We considered allowing a partial XSAVE buffer for PTRACE_SETREGSET, but
it was that the XSAVE instruction requires a full-size buffer led to
this choice. Using a smaller buffer for XSAVE causes a fault.

So, this code is not supposed to work?
iov.iov_base = fp_regs;
iov.iov_len = sizeof(struct _xstate);
ptrace(PTRACE_GETREGSET, pid, NT_X86_XSTATE, &iov);
ptrace(PTRACE_SETREGSET, pid, NT_X86_XSTATE, &iov);
This is what UML does and on Thomas's new Laptop PTRACE_SETREGSET is failing.

Hmm, I think we need to do what gdb does, it uses a buffer of size X86_XSTATE_MAX_SIZE.

Thanks,
//richard

[Thomas Meyer]

Post by Richard Weinberger
Yu-cheng,

Post by Richard Weinberger
Yu-cheng,

Post by Thomas Meyer
- PTRACE_GETREGSET with NT_X86_XSTATE gets 832 and return 832, with no
error.
- PTRACE_SETREGSET get 832 (sizeof struct _xstate) but wants at least
1088, otherwise it will fail with -EFAULT (why not -EINVAL?)
Ideas?

We considered allowing a partial XSAVE buffer for PTRACE_SETREGSET, but
it was that the XSAVE instruction requires a full-size buffer led to
this choice. Using a smaller buffer for XSAVE causes a fault.

So, this code is not supposed to work?
iov.iov_base = fp_regs;
iov.iov_len = sizeof(struct _xstate);
ptrace(PTRACE_GETREGSET, pid, NT_X86_XSTATE, &iov);
ptrace(PTRACE_SETREGSET, pid, NT_X86_XSTATE, &iov);
This is what UML does and on Thomas's new Laptop PTRACE_SETREGSET is failing.

Hmm, I think we need to do what gdb does, it uses a buffer of size X86_XSTATE_MAX_SIZE.

http://elixir.free-electrons.com/linux/latest/source/arch/x86/kernel/fpu/xstate.c#L626

Hi,

Is there a user space API to get this value?

GDB has a fixed X86_XSTATE_MAX_SIZE of 2688. That can become an issue.
Yu-cheng

[Natale Patriciello]

Post by Richard Weinberger
Yu-cheng,

Post by Richard Weinberger
Yu-cheng,

Post by Thomas Meyer
- PTRACE_GETREGSET with NT_X86_XSTATE gets 832 and return 832, with no
error.
- PTRACE_SETREGSET get 832 (sizeof struct _xstate) but wants at least
1088, otherwise it will fail with -EFAULT (why not -EINVAL?)
Ideas?

[cut text and CC'ed persons]

It is funny to see that this problem was firstly reported here [1] in
February 2017 without being considered until someone else bought a new
laptop :)

Anyway, thank you for digging into this; my temporary workaround at
the time was to use always the *_i387_registers functions.

HTH,
Nat

[1] https://sourceforge.net/p/user-mode-linux/mailman/message/35663374/

[Thomas Meyer]

Post by Natale Patriciello

Post by Richard Weinberger
Yu-cheng,

Post by Richard Weinberger
Yu-cheng,

Post by Thomas Meyer
- PTRACE_GETREGSET with NT_X86_XSTATE gets 832 and
return 832, with no
error.
- PTRACE_SETREGSET get 832 (sizeof struct _xstate) but
wants at least
1088, otherwise it will fail with -EFAULT (why not -EINVAL?)
Ideas?

[cut text and CC'ed persons]
It is funny to see that this problem was firstly reported here [1] in
February 2017 without being considered until someone else bought a new
laptop :)

Yes, I like my new laptop :-)

Post by Natale Patriciello
Anyway, thank you for digging into this; my temporary workaround at
the time was to use always the *_i387_registers functions.

Oops, there is the complete thread with the same problem. But sorry I
don't follow uml-user, just uml-devel :-(

As a quick fix you can try this:

diff --git a/arch/x86/um/os-Linux/registers.c b/arch/x86/um/os-Linux/registers.c
index 00f54a91bb4b..6eac8220ab29 100644
--- a/arch/x86/um/os-Linux/registers.c
+++ b/arch/x86/um/os-Linux/registers.c
@@ -30,7 +30,7 @@ int save_fp_registers(int pid, unsigned long *fp_regs)

if (have_xstate_support) {
iov.iov_base = fp_regs;
- iov.iov_len = sizeof(struct _xstate);
+ iov.iov_len = HOST_FP_SIZE * sizeof(unsigned long);
if (ptrace(PTRACE_GETREGSET, pid, NT_X86_XSTATE, &iov) < 0)
return -errno;
return 0;
@@ -49,10 +49,9 @@ int restore_i387_registers(int pid, unsigned long *fp_regs)
int restore_fp_registers(int pid, unsigned long *fp_regs)
{
struct iovec iov;
-
if (have_xstate_support) {
iov.iov_base = fp_regs;
- iov.iov_len = sizeof(struct _xstate);
+ iov.iov_len = HOST_FP_SIZE * sizeof(unsigned long);
if (ptrace(PTRACE_SETREGSET, pid, NT_X86_XSTATE, &iov) < 0)
return -errno;
return 0;
@@ -126,7 +125,7 @@ void arch_init_registers(int pid)
struct iovec iov;

iov.iov_base = &fp_regs;
- iov.iov_len = sizeof(struct _xstate);
+ iov.iov_len = HOST_FP_SIZE * sizeof(unsigned long);
if (ptrace(PTRACE_GETREGSET, pid, NT_X86_XSTATE, &iov) == 0)
have_xstate_support = 1;
}
diff --git a/arch/x86/um/user-offsets.c b/arch/x86/um/user-offsets.c
index cb3c22370cf5..9dccbbbf2fd1 100644
--- a/arch/x86/um/user-offsets.c
+++ b/arch/x86/um/user-offsets.c
@@ -50,7 +50,7 @@ void foo(void)
DEFINE(HOST_GS, GS);
DEFINE(HOST_ORIG_AX, ORIG_EAX);
#else
- DEFINE(HOST_FP_SIZE, sizeof(struct _xstate) / sizeof(unsigned long));
+ DEFINE_LONGS(HOST_FP_SIZE, 2688);
DEFINE_LONGS(HOST_BX, RBX);
DEFINE_LONGS(HOST_CX, RCX);
DEFINE_LONGS(HOST_DI, RDI);


a better fix would be to make the fp regs in struct uml_pt_regs dynamic
depending on the current kernel idea of the xsave area size.

Post by Natale Patriciello
HTH,
Nat
[1] https://sourceforge.net/p/user-mode-linux/mailman/message/3566337
4/

[Richard Weinberger]

Thomas, Natale,

Post by Thomas Meyer

Post by Natale Patriciello
It is funny to see that this problem was firstly reported here [1] in
February 2017 without being considered until someone else bought a new
laptop :)

sometimes mails/issues get lost...

Post by Thomas Meyer
Yes, I like my new laptop :-)

Post by Natale Patriciello
Anyway, thank you for digging into this; my temporary workaround at
the time was to use always the *_i387_registers functions.

Oops, there is the complete thread with the same problem. But sorry I
don't follow uml-user, just uml-devel :-(

We really should merge these lists. ;-\

Post by Thomas Meyer
diff --git a/arch/x86/um/os-Linux/registers.c b/arch/x86/um/os-Linux/registers.c
index 00f54a91bb4b..6eac8220ab29 100644
--- a/arch/x86/um/os-Linux/registers.c
+++ b/arch/x86/um/os-Linux/registers.c
@@ -30,7 +30,7 @@ int save_fp_registers(int pid, unsigned long *fp_regs)
if (have_xstate_support) {
iov.iov_base = fp_regs;
- iov.iov_len = sizeof(struct _xstate);
+ iov.iov_len = HOST_FP_SIZE * sizeof(unsigned long);
if (ptrace(PTRACE_GETREGSET, pid, NT_X86_XSTATE, &iov) < 0)
return -errno;
return 0;
@@ -49,10 +49,9 @@ int restore_i387_registers(int pid, unsigned long *fp_regs)
int restore_fp_registers(int pid, unsigned long *fp_regs)
{
struct iovec iov;
-
if (have_xstate_support) {
iov.iov_base = fp_regs;
- iov.iov_len = sizeof(struct _xstate);
+ iov.iov_len = HOST_FP_SIZE * sizeof(unsigned long);
if (ptrace(PTRACE_SETREGSET, pid, NT_X86_XSTATE, &iov) < 0)
return -errno;
return 0;
@@ -126,7 +125,7 @@ void arch_init_registers(int pid)
struct iovec iov;
iov.iov_base = &fp_regs;
- iov.iov_len = sizeof(struct _xstate);
+ iov.iov_len = HOST_FP_SIZE * sizeof(unsigned long);
if (ptrace(PTRACE_GETREGSET, pid, NT_X86_XSTATE, &iov) == 0)
have_xstate_support = 1;
}
diff --git a/arch/x86/um/user-offsets.c b/arch/x86/um/user-offsets.c
index cb3c22370cf5..9dccbbbf2fd1 100644
--- a/arch/x86/um/user-offsets.c
+++ b/arch/x86/um/user-offsets.c
@@ -50,7 +50,7 @@ void foo(void)
DEFINE(HOST_GS, GS);
DEFINE(HOST_ORIG_AX, ORIG_EAX);
#else
- DEFINE(HOST_FP_SIZE, sizeof(struct _xstate) / sizeof(unsigned long));
+ DEFINE_LONGS(HOST_FP_SIZE, 2688);
DEFINE_LONGS(HOST_BX, RBX);
DEFINE_LONGS(HOST_CX, RCX);
DEFINE_LONGS(HOST_DI, RDI);
a better fix would be to make the fp regs in struct uml_pt_regs dynamic
depending on the current kernel idea of the xsave area size.

And we should consider a mechanism to save/restore the full regset only when needed.
Otherwise the context switch is even more slower.

Thanks,
//richard