Re: [uml-devel] [PATCH v2] um: add dummy ioremap and iounmap functions

[Arnd Bergmann <arnd@ar...>]

On Wed, Jun 21, 2017 at 11:31 PM, Logan Gunthorpe <logang@...> wrote:
>
>
> On 21/06/17 03:30 PM, Richard Weinberger wrote:
>> Since Arnd stays silent and um is the only remaining odd-ball, let's merge this
>> in v4.13.
>
> Great! Thanks!

Sorry for missing most of the discussion earlier. The last proposed version
(returning NULL to always fail) seems fine to me, too.

       Arnd

Re: [uml-devel] [PATCH v2] um: add dummy ioremap and iounmap functions

[Richard Weinberger <richard@no...>]

Logan,

Am 21.06.2017 um 23:25 schrieb Logan Gunthorpe:
> Poke.
> 
> On 08/06/17 01:17 PM, Richard Weinberger wrote:
>> Am 08.06.2017 um 20:53 schrieb Logan Gunthorpe:
>> IMHO an ifdef in scatterlist code does not hurt.
>> It is equally ugly than mocking ioremap for UML.
> 
> I disagree. Having ifdefs scattered around all call sites of a function
> is *much* worse than having an extra mock function tucked away in a
> header file somewhere.
> 
> It's a very common pattern in the kernel for providers of functions that
> depend on a configuration option to provide mock functions when the
> configuration option is not selected. This prevents needing every caller
> of said function to put #ifdefs around the call. For a few examples:
> 
> include/linux/blkdev.h:1952
> include/linux/dax.h:24
> include/linux/pci.h:1329
> 
> And, frankly, it's _exactly_ what Linus Torvalds himself was arguing
> against in the link I sent up-thread [1].
> 
>> So, I'm puzzled.
>> Arnd, what do you think?
>> Shall !HAS_IOMEM archs just mock these functions?
> 
> So, once again, 'um' is now the only architecture that has this problem
> since tile and s390 accepted my patches (and without a fuss too). So can
> you please consider merging this patch or proposing something that will
> also fix the problem?

Since Arnd stays silent and um is the only remaining odd-ball, let's merge this
in v4.13.

Thanks,
//richard

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

[Richard Weinberger <richard@no...>]

Thomas, Natale,

Am 21.06.2017 um 11:44 schrieb Thomas Meyer:
>> 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...

> Yes, I like my new laptop :-)
> 
>> 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. ;-\

> 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.

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

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

[Thomas Meyer <thomas@m3...>]

Am Mittwoch, den 21.06.2017, 10:39 +0200 schrieb Natale Patriciello:
> 2017-06-20 22:13 GMT+02:00 Thomas Meyer <thomas@...>:
> > 
> > > Am 20.06.2017 um 21:53 schrieb Yu-cheng Yu <yu-cheng.yu@...
> > > >:
> > > 
> > > > On Tue, 2017-06-20 at 20:59 +0200, Richard Weinberger wrote:
> > > > Yu-cheng,
> > > > 
> > > > > Am 20.06.2017 um 20:17 schrieb Richard Weinberger:
> > > > > Yu-cheng,
> > > > > 
> > > > > Am 20.06.2017 um 20:04 schrieb Yu-cheng Yu:
> > > > > > > > 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?
> 
> [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 :-)

> 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.

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

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

[Natale Patriciello <natale.patriciello@gm...>]

2017-06-20 22:13 GMT+02:00 Thomas Meyer <thomas@...>:
>
>> Am 20.06.2017 um 21:53 schrieb Yu-cheng Yu <yu-cheng.yu@...>:
>>
>>> On Tue, 2017-06-20 at 20:59 +0200, Richard Weinberger wrote:
>>> Yu-cheng,
>>>
>>>> Am 20.06.2017 um 20:17 schrieb Richard Weinberger:
>>>> Yu-cheng,
>>>>
>>>> Am 20.06.2017 um 20:04 schrieb Yu-cheng Yu:
>>>>>>> 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?

[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/

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

[Thomas Meyer <thomas@m3...>]

> Am 20.06.2017 um 21:53 schrieb Yu-cheng Yu <yu-cheng.yu@...>:
> 
>> On Tue, 2017-06-20 at 20:59 +0200, Richard Weinberger wrote:
>> Yu-cheng,
>> 
>>> Am 20.06.2017 um 20:17 schrieb Richard Weinberger:
>>> Yu-cheng,
>>> 
>>> Am 20.06.2017 um 20:04 schrieb Yu-cheng Yu:
>>>>>> 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?
>>>> 
>>>> 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.
> 
> Linux kernel determines XSAVE buffer size from CPUID:
> 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
> 
> 

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

[Richard Weinberger <richard@no...>]

Yu-cheng,

Am 20.06.2017 um 20:17 schrieb Richard Weinberger:
> Yu-cheng,
> 
> Am 20.06.2017 um 20:04 schrieb Yu-cheng Yu:
>>>> 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?
>>
>> 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

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

[Richard Weinberger <richard@no...>]

Yu-cheng,

Am 20.06.2017 um 20:04 schrieb Yu-cheng Yu:
>>> 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?
> 
> 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

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

[Richard Weinberger <richard@no...>]

[adding x86 folks]

Am 20.06.2017 um 10:49 schrieb Thomas Meyer:
> Am Dienstag, den 20.06.2017, 08:58 +0200 schrieb Richard Weinberger:
>> Thomas,
>>
>> Am 20.06.2017 um 03:56 schrieb 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. ;-\
>>
>>> 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 :-(
>>
>> As always. printk is best debugger ever. ;-)
>>
>>> 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.
>>
>> Does PTRACE_GETREGSET return a size?
> 
> Yes, it returns 832. the size of struct _xstate.
> 
>> 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-cheng.yu@...>
> 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?
> 
>>
>> Thanks,
>> //richard

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

[Thomas Meyer <thomas@m3...>]

Am Dienstag, den 20.06.2017, 08:58 +0200 schrieb Richard Weinberger:
> Thomas,
> 
> Am 20.06.2017 um 03:56 schrieb 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. ;-\
> 
> > 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 :-(
> 
> As always. printk is best debugger ever. ;-)
> 
> > 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.
> 
> Does PTRACE_GETREGSET return a size?

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

> 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-cheng.yu@...>
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?

> 
> Thanks,
> //richard

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

[Richard Weinberger <richard@no...>]

Thomas,

Am 20.06.2017 um 03:56 schrieb 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. ;-\

> 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 :-(

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

> 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.

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

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

[Thomas Meyer <thomas@m3...>]

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

Re: [uml-devel] [PATCH v2] um: add dummy ioremap and iounmap functions

[Richard Weinberger <richard@no...>]

Am 08.06.2017 um 20:53 schrieb Logan Gunthorpe:
> Any thoughts on this? My patches for the other architectures are already
> in linux-next. um is the only one that remains.

IMHO an ifdef in scatterlist code does not hurt.
It is equally ugly than mocking ioremap for UML.

So, I'm puzzled.
Arnd, what do you think?
Shall !HAS_IOMEM archs just mock these functions?

Thanks,
//richard

Re: [uml-devel] [PATCH 22/35] um: defconfig: Cleanup from old Kconfig options

[Richard Weinberger <richard@no...>]

Am 08.06.2017 um 18:10 schrieb Krzysztof Kozlowski:
> Remove old, dead Kconfig option INET_LRO. It is gone since
> commit 7bbf3cae65b6 ("ipv4: Remove inet_lro library").
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@...>

Acked-by: Richard Weinberger <richard@...>

Thanks,
//richard

Re: [uml-devel] [PATCH v2] um: Avoid longjmp/setjmp symbol clashes with libpthread.a

[Richard Weinberger <richard@no...>]

Florian,

Am 05.06.2017 um 21:32 schrieb Florian Fainelli:
> On 05/23/2017 05:32 PM, Florian Fainelli wrote:
>> Building a statically linked UML kernel on a Centos 6.9 host resulted in
>> the following linking failure (GCC 4.4, glibc-2.12):
>>
>> /usr/lib/gcc/x86_64-redhat-linux/4.4.7/../../../../lib64/libpthread.a(libpthread.o):
>> In function `siglongjmp':
>> (.text+0x8490): multiple definition of `longjmp'
>> arch/x86/um/built-in.o:/local/users/fainelli/openwrt/trunk/build_dir/target-x86_64_musl/linux-uml/linux-4.4.69/arch/x86/um/setjmp_64.S:44:
>> first defined here
>> /usr/lib/gcc/x86_64-redhat-linux/4.4.7/../../../../lib64/libpthread.a(libpthread.o):
>> In function `sem_open':
>> (.text+0x77cd): warning: the use of `mktemp' is dangerous, better use
>> `mkstemp'
>> collect2: ld returned 1 exit status
>> make[4]: *** [vmlinux] Error 1
>>
>> Adopt a solution similar to the one done for vmap where we define
>> longjmp/setjmp to be kernel_longjmp/setjmp. In the process, make sure we
>> do rename the functions in arch/x86/um/setjmp_*.S accordingly.
>>
>> Fixes: a7df4716d195 ("um: link with -lpthread")
>> Signed-off-by: Florian Fainelli <f.fainelli@...>
> 
> Richard, we are kind of hijacking this thread now that was originally
> about statically linking UML, is this particular patch okay?

Hehe, yes.
This patch is good, I like it. :)
It will part of the next pull request.

Thanks,
//richard

Re: [uml-devel] [PATCH v2] um: Avoid longjmp/setjmp symbol clashes with libpthread.a

[Richard Weinberger <richard@no...>]

Am 01.06.2017 um 22:15 schrieb Florian Fainelli:
> On 06/01/2017 01:11 PM, Richard Weinberger wrote:
>> Florian,
>>
>> Am 01.06.2017 um 21:38 schrieb Florian Fainelli:
>>>> Presumably because we are not using the same glibc version? The one I
>>>> have installed on this machine is glibc-2.12, do you want me to attach a
>>>> copy of it?
>>>
>>> Richard, what do we do with this?
>>
>> I'd like to see the issues that Thomas sees also get addressed.
> 
> Sure, but that seems orthogonal? In the absence of an answer from Eli,
> either you could take my patch or just send reverts of Eli's two
> commits, whichever you prefer.

Or you and Thomas could investigate. :-)

Thanks,
//richard

Re: [uml-devel] [PATCH v2] um: Avoid longjmp/setjmp symbol clashes with libpthread.a

[Richard Weinberger <richard@no...>]

Florian,

Am 01.06.2017 um 21:38 schrieb Florian Fainelli:
>> Presumably because we are not using the same glibc version? The one I
>> have installed on this machine is glibc-2.12, do you want me to attach a
>> copy of it?
> 
> Richard, what do we do with this?

I'd like to see the issues that Thomas sees also get addressed.

Thanks,
//richard

Re: [uml-devel] [PATCH v2] um: add dummy ioremap and iounmap functions

[Geert Uytterhoeven <geert@li...>]

Hi Logan,

On Thu, May 25, 2017 at 5:53 PM, Logan Gunthorpe <logang@...> wrote:
> On 25/05/17 09:48 AM, Richard Weinberger wrote:
>> Which ones are failing?
>> I thought we killed the problem by making CONFIG_COMPILE_TEST depend on !UML.
>
> None, at the moment. My work is trying to add iomem support to scatter
> lists and thus I want to call ioremap in scatterlist.c. That's when
> things fail to build. We could put an '#ifdef HAS_IOMEM' around only the
> uses, but I thought this approach was cleaner. However, if people would
> rather I do that, let me know and I will.
>
>> I was never a fan of that approach because in my opinion drivers should have
>> proper dependencies, including a dependency on HAS_IOMEM.
>
> This doesn't really work when we try to use it in a core library which
> can't depend on HAS_IOMEM.

Still, those code patch could be protected by #ifdef CONFIG_HAS_IOMEM,
or better, if (IS_ENABLED(CONFIG_HAS_IOMEM)).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [uml-devel] [PATCH v2] um: add dummy ioremap and iounmap functions

[Richard Weinberger <richard@no...>]

Logan,

Am 25.05.2017 um 17:42 schrieb Logan Gunthorpe:
> The user mode architecture does not provide ioremap or iounmap, and
> because of this, the arch won't build when the functions are used in some
> core libraries.

Which ones are failing?
I thought we killed the problem by making CONFIG_COMPILE_TEST depend on !UML.

> I have designs to use these functions in scatterlist.c where they'd
> almost certainly never be called on the um architecture but it does need
> to compile. Thus, if the function is ever hit it returns NULL.

I was never a fan of that approach because in my opinion drivers should have
proper dependencies, including a dependency on HAS_IOMEM.

But I'll no longer block these attempts if we can get rid of tons of fallout
every kernel release.

Thanks,
//richard

Re: [uml-devel] [PATCH] um: add dummy ioremap and iounmap functions

[Al Viro <viro@ZenIV.linux.org.uk>]

On Wed, May 24, 2017 at 04:00:52PM -0600, Logan Gunthorpe wrote:
> The user mode architecture does not provide ioremap or iounmap, and
> because of this, the arch won't build when the functions are used in some
> core libraries.
> 
> I have designs to use these functions in scatterlist.c where they'd
> almost certainly never be called on the um architecture but it does need
> to compile.

Then make it fail there.

Incidentally, s390 has ioremap() only when CONFIG_PCI is set.

[uml-devel] [PATCH v2] um: Add mark_rodata_ro support

[Thomas Meyer <thomas@m3...>]

Signed-off-by: Thomas Meyer <thomas@...>
---
 arch/um/Kconfig.common |  1 +
 arch/um/kernel/mem.c   | 13 ++++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/um/Kconfig.common b/arch/um/Kconfig.common
index 85f6dd2..061009b 100644
--- a/arch/um/Kconfig.common
+++ b/arch/um/Kconfig.common
@@ -2,6 +2,7 @@ config UML
 	bool
 	default y
 	select ARCH_HAS_KCOV
+	select ARCH_HAS_STRICT_KERNEL_RWX
 	select HAVE_ARCH_AUDITSYSCALL
 	select HAVE_ARCH_SECCOMP_FILTER
 	select HAVE_UID16
diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
index e7437ec..8705eff 100644
--- a/arch/um/kernel/mem.c
+++ b/arch/um/kernel/mem.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <asm/fixmap.h>
 #include <asm/page.h>
+#include <asm/sections.h>
 #include <as-layout.h>
 #include <init.h>
 #include <kern.h>
@@ -168,7 +169,6 @@ void __init paging_init(void)
  * This can't do anything because nothing in the kernel image can be freed
  * since it's not in kernel physical memory.
  */
-
 void free_initmem(void)
 {
 }
@@ -238,3 +238,14 @@ void *uml_kmalloc(int size, int flags)
 {
 	return kmalloc(size, flags);
 }
+
+void mark_rodata_ro(void)
+{
+	/* rodata_start/end must be PAGE_SIZE aligend! */
+	unsigned long rodata_start = (unsigned long) __start_rodata;
+	unsigned long rodata_end = (unsigned long) __end_rodata;
+
+	printk(KERN_INFO "Write protecting the kernel read-only data: %luk\n",
+	       (rodata_end - rodata_start) >> 10);
+	os_protect_memory((void*)rodata_start, (rodata_end - rodata_start), 1, 0, 0);
+}

Re: [uml-devel] [PATCH v2] um: Avoid longjmp/setjmp symbol clashes with libpthread.a

[Richard Weinberger <richard@no...>]

Florian,

Am 24.05.2017 um 02:32 schrieb Florian Fainelli:
> Building a statically linked UML kernel on a Centos 6.9 host resulted in
> the following linking failure (GCC 4.4, glibc-2.12):
> 
> /usr/lib/gcc/x86_64-redhat-linux/4.4.7/../../../../lib64/libpthread.a(libpthread.o):
> In function `siglongjmp':
> (.text+0x8490): multiple definition of `longjmp'
> arch/x86/um/built-in.o:/local/users/fainelli/openwrt/trunk/build_dir/target-x86_64_musl/linux-uml/linux-4.4.69/arch/x86/um/setjmp_64.S:44:
> first defined here
> /usr/lib/gcc/x86_64-redhat-linux/4.4.7/../../../../lib64/libpthread.a(libpthread.o):
> In function `sem_open':
> (.text+0x77cd): warning: the use of `mktemp' is dangerous, better use
> `mkstemp'
> collect2: ld returned 1 exit status
> make[4]: *** [vmlinux] Error 1
> 
> Adopt a solution similar to the one done for vmap where we define
> longjmp/setjmp to be kernel_longjmp/setjmp. In the process, make sure we
> do rename the functions in arch/x86/um/setjmp_*.S accordingly.

What is not so clear to me, why are you facing this build issue and other users, including me,
not?

Thanks,
//richard

[uml-devel] [PATCH] um: userspace - be more verbose in ptrace set regs error

[Thomas Meyer <thomas@m3...>]

When ptrace fails to set GP/FP regs for the target process,
log the error before crashing the UML kernel.

Signed-off-by: Thomas Meyer <thomas@...>
---
 arch/um/os-Linux/skas/process.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/um/os-Linux/skas/process.c b/arch/um/os-Linux/skas/process.c
index 4530867..819d686 100644
--- a/arch/um/os-Linux/skas/process.c
+++ b/arch/um/os-Linux/skas/process.c
@@ -352,11 +352,17 @@ void userspace(struct uml_pt_regs *regs)
 		 * fail.  In this case, there is nothing to do but
 		 * just kill the process.
 		 */
-		if (ptrace(PTRACE_SETREGS, pid, 0, regs->gp))
+		if (ptrace(PTRACE_SETREGS, pid, 0, regs->gp)) {
+			printk(UM_KERN_ERR "userspace - ptrace set regs "
+			       "failed, errno = %d\n", errno);
 			fatal_sigsegv();
+		}
 
-		if (put_fp_registers(pid, regs->fp))
+		if (put_fp_registers(pid, regs->fp)) {
+			printk(UM_KERN_ERR "userspace - ptrace set fp regs "
+			       "failed, errno = %d\n", errno);
 			fatal_sigsegv();
+		}
 
 		/* Now we set local_using_sysemu to be used for one loop */
 		local_using_sysemu = get_using_sysemu();

Re: [uml-devel] um: xstate changes breaks my UML setup

[Thomas Meyer <thomas@m3...>]

> Am 23.05.2017 um 21:30 schrieb Richard Weinberger <richard.weinberger@...>:
> 
> Thomas,
> 
>> On Tue, May 23, 2017 at 7:56 PM, Thomas Meyer <thomas@...> wrote:
>> Hi,
>> 
>> 
>> 
>> to make UML work again with the latest Fedora Installation I Need to revert
>> those commits:
>> 
>> b6024b21fec8367ef961a771cc9dde31f1831965
>> 
>> a78ff1112263fdd871d3506dbcff44f6f12e8423
>> 
>> 
>> 
>> A reproducer script/config is available here:
>> 
>> https://github.com/thomasmey/uml/blob/master/fedora-26-install.sh
> 
> Please start using the CC feature of your mailer. :-)

Damn, I did so, but Windows 10 Mail is just so broken...

> 
> -- 
> Thanks,
> //richard
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> User-mode-linux-devel mailing list
> User-mode-linux-devel@...
> https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] um: prinft patches from Masami Hiramatsu

[Richard Weinberger <richard.weinberger@gm...>]

On Tue, May 23, 2017 at 7:56 PM, Thomas Meyer <thomas@...> wrote:
> Hi,
>
>
>
> did you see those patches?
>
>
>
> https://marc.info/?l=linux-kernel&m=149337486632399&w=2

Yes, they are on my TODO.

-- 
Thanks,
//richard