Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

2017-01-30 Thread Yu-cheng Yu 
On Mon, Jan 30, 2017 at 04:45:21PM +0100, Borislav Petkov wrote:
> On Mon, Jan 30, 2017 at 10:57:28AM +0100, Ingo Molnar wrote:
> > Would anyone object to using u32 in these prototypes?
> 
> Well, would there be any disadvantage to forcing them to u32?
> Potentially by something else wanting to use those interfaces besides
> the regset thing and that something else doesn't like u32s?
> 
> Otherwise, I don't see a problem.
> 
> I mean, if 4G are not enough for xstate dimensions then we have a whole
> different problem.

This function pair was intended to be similar to user_regset_copyout(), 
user_regset_copyin() used for the standard-format XSAVE area copying.
I totally agree it is complex and should be simplified.  Why don't we
do both places? 

Yu-cheng

Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

2017-01-30 Thread Yu-cheng Yu 
On Mon, Jan 30, 2017 at 04:45:21PM +0100, Borislav Petkov wrote:
> On Mon, Jan 30, 2017 at 10:57:28AM +0100, Ingo Molnar wrote:
> > Would anyone object to using u32 in these prototypes?
> 
> Well, would there be any disadvantage to forcing them to u32?
> Potentially by something else wanting to use those interfaces besides
> the regset thing and that something else doesn't like u32s?
> 
> Otherwise, I don't see a problem.
> 
> I mean, if 4G are not enough for xstate dimensions then we have a whole
> different problem.

This function pair was intended to be similar to user_regset_copyout(), 
user_regset_copyin() used for the standard-format XSAVE area copying.
I totally agree it is complex and should be simplified.  Why don't we
do both places? 

Yu-cheng

Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

2017-01-30 Thread Borislav Petkov 
On Mon, Jan 30, 2017 at 10:57:28AM +0100, Ingo Molnar wrote:
> Would anyone object to using u32 in these prototypes?

Well, would there be any disadvantage to forcing them to u32?
Potentially by something else wanting to use those interfaces besides
the regset thing and that something else doesn't like u32s?

Otherwise, I don't see a problem.

I mean, if 4G are not enough for xstate dimensions then we have a whole
different problem.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

2017-01-30 Thread Borislav Petkov 
On Mon, Jan 30, 2017 at 10:57:28AM +0100, Ingo Molnar wrote:
> Would anyone object to using u32 in these prototypes?

Well, would there be any disadvantage to forcing them to u32?
Potentially by something else wanting to use those interfaces besides
the regset thing and that something else doesn't like u32s?

Otherwise, I don't see a problem.

I mean, if 4G are not enough for xstate dimensions then we have a whole
different problem.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

2017-01-30 Thread Ingo Molnar 

* Borislav Petkov  wrote:

> On Thu, Jan 26, 2017 at 11:22:49AM +0100, Ingo Molnar wrote:
> > The 'kbuf' parameter is unused in the _user() side of the API, remove it.
> > 
> > This simplifies the code and makes it easier to think about.
> 
> ...
> 
> > @@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned 
> > int count, void *kbuf, stru
> >  }
> >  
> >  static inline int
> > -__copy_xstate_to_user(unsigned int pos, unsigned int count,
> > - void *kbuf, void __user *ubuf,
> > - const void *data, const int start_pos,
> > - const int end_pos)
> > +__copy_xstate_to_user(unsigned int pos, unsigned int count, void __user 
> > *ubuf, const void *data, const int start_pos, const int end_pos)
> 
> That and similar lines are insanely long and could be broken.

Yeah, so that's one point of the series: the interface was insanely complex 
(the 
original sin is that of the regset interfaces), and one symptom of that 
complexity 
are these overly long prototypes - the above one has 7 arguments (!!). Another, 
far more serious symptom of the complexity were the bugs that Rik found.

The solution was not to break the prototype into multiple lines and thus paper 
over one symptom of complexity, but to _reduce_ complexity.

So at the end of the series the basic copy_xstate_to_user() prototype looks 
like 
this:

 static inline int
 __copy_xstate_to_user(void __user *ubuf, const void *data, unsigned int 
offset, unsigned int size, unsigned int size_total)

which is less complex and shorter as well. It could probably be shortened 
further:

 static inline int
 __copy_xstate_to_user(void __user *ubuf, const void *data, u32 offset, u32 
size, u32 total)

because our regset (and user-copy) APIs are intentionally 32-bit - but this 
would 
depart from the existing signature style so I'm not sure we want to do it 
unilaterally.

Would anyone object to using u32 in these prototypes?

Thanks,

Ingo

Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

2017-01-30 Thread Ingo Molnar 

* Borislav Petkov  wrote:

> On Thu, Jan 26, 2017 at 11:22:49AM +0100, Ingo Molnar wrote:
> > The 'kbuf' parameter is unused in the _user() side of the API, remove it.
> > 
> > This simplifies the code and makes it easier to think about.
> 
> ...
> 
> > @@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned 
> > int count, void *kbuf, stru
> >  }
> >  
> >  static inline int
> > -__copy_xstate_to_user(unsigned int pos, unsigned int count,
> > - void *kbuf, void __user *ubuf,
> > - const void *data, const int start_pos,
> > - const int end_pos)
> > +__copy_xstate_to_user(unsigned int pos, unsigned int count, void __user 
> > *ubuf, const void *data, const int start_pos, const int end_pos)
> 
> That and similar lines are insanely long and could be broken.

Yeah, so that's one point of the series: the interface was insanely complex 
(the 
original sin is that of the regset interfaces), and one symptom of that 
complexity 
are these overly long prototypes - the above one has 7 arguments (!!). Another, 
far more serious symptom of the complexity were the bugs that Rik found.

The solution was not to break the prototype into multiple lines and thus paper 
over one symptom of complexity, but to _reduce_ complexity.

So at the end of the series the basic copy_xstate_to_user() prototype looks 
like 
this:

 static inline int
 __copy_xstate_to_user(void __user *ubuf, const void *data, unsigned int 
offset, unsigned int size, unsigned int size_total)

which is less complex and shorter as well. It could probably be shortened 
further:

 static inline int
 __copy_xstate_to_user(void __user *ubuf, const void *data, u32 offset, u32 
size, u32 total)

because our regset (and user-copy) APIs are intentionally 32-bit - but this 
would 
depart from the existing signature style so I'm not sure we want to do it 
unilaterally.

Would anyone object to using u32 in these prototypes?

Thanks,

Ingo

Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

2017-01-27 Thread Borislav Petkov 
On Thu, Jan 26, 2017 at 11:22:49AM +0100, Ingo Molnar wrote:
> The 'kbuf' parameter is unused in the _user() side of the API, remove it.
> 
> This simplifies the code and makes it easier to think about.

...

> @@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned 
> int count, void *kbuf, stru
>  }
>  
>  static inline int
> -__copy_xstate_to_user(unsigned int pos, unsigned int count,
> -   void *kbuf, void __user *ubuf,
> -   const void *data, const int start_pos,
> -   const int end_pos)
> +__copy_xstate_to_user(unsigned int pos, unsigned int count, void __user 
> *ubuf, const void *data, const int start_pos, const int end_pos)

That and similar lines are insanely long and could be broken.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

2017-01-27 Thread Borislav Petkov 
On Thu, Jan 26, 2017 at 11:22:49AM +0100, Ingo Molnar wrote:
> The 'kbuf' parameter is unused in the _user() side of the API, remove it.
> 
> This simplifies the code and makes it easier to think about.

...

> @@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned 
> int count, void *kbuf, stru
>  }
>  
>  static inline int
> -__copy_xstate_to_user(unsigned int pos, unsigned int count,
> -   void *kbuf, void __user *ubuf,
> -   const void *data, const int start_pos,
> -   const int end_pos)
> +__copy_xstate_to_user(unsigned int pos, unsigned int count, void __user 
> *ubuf, const void *data, const int start_pos, const int end_pos)

That and similar lines are insanely long and could be broken.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

2017-01-26 Thread Ingo Molnar 
The 'kbuf' parameter is unused in the _user() side of the API, remove it.

This simplifies the code and makes it easier to think about.

Cc: Andy Lutomirski 
Cc: Borislav Petkov 
Cc: Dave Hansen 
Cc: Fenghua Yu 
Cc: H. Peter Anvin 
Cc: Linus Torvalds 
Cc: Oleg Nesterov 
Cc: Rik van Riel 
Cc: Thomas Gleixner 
Cc: Yu-cheng Yu 
Cc: Fenghua Yu 
Signed-off-by: Ingo Molnar 
---
 arch/x86/include/asm/fpu/xstate.h |  2 +-
 arch/x86/kernel/fpu/regset.c  |  2 +-
 arch/x86/kernel/fpu/xstate.c  | 25 +++--
 3 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h 
b/arch/x86/include/asm/fpu/xstate.h
index c762574a245f..65bd68c30cd0 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -49,7 +49,7 @@ void *get_xsave_addr(struct xregs_state *xsave, int xstate);
 const void *get_xsave_field_ptr(int xstate_field);
 int using_compacted_format(void);
 int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, 
struct xregs_state *xsave);
-int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf, void 
__user *ubuf, struct xregs_state *xsave);
+int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user 
*ubuf, struct xregs_state *xsave);
 int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
 struct xregs_state *xsave);
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index a5f2b38a47dc..62a2d1154e45 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -94,7 +94,7 @@ int xstateregs_get(struct task_struct *target, const struct 
user_regset *regset,
if (kbuf)
ret = copy_xstate_to_kernel(pos, count, kbuf, xsave);
else
-   ret = copy_xstate_to_user(pos, count, kbuf, ubuf, 
xsave);
+   ret = copy_xstate_to_user(pos, count, ubuf, xsave);
} else {
fpstate_sanitize_xstate(fpu);
/*
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 6ac8e4a759ff..1ac3d2ae73e6 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int 
count, void *kbuf, stru
 }
 
 static inline int
-__copy_xstate_to_user(unsigned int pos, unsigned int count,
- void *kbuf, void __user *ubuf,
- const void *data, const int start_pos,
- const int end_pos)
+__copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, 
const void *data, const int start_pos, const int end_pos)
 {
if ((count == 0) || (pos < start_pos))
return 0;
@@ -1021,12 +1018,8 @@ __copy_xstate_to_user(unsigned int pos, unsigned int 
count,
if (end_pos < 0 || pos < end_pos) {
unsigned int copy = (end_pos < 0 ? count : min(count, end_pos - 
pos));
 
-   if (kbuf) {
-   memcpy(kbuf + pos, data, copy);
-   } else {
-   if (__copy_to_user(ubuf + pos, data, copy))
-   return -EFAULT;
-   }
+   if (__copy_to_user(ubuf + pos, data, copy))
+   return -EFAULT;
}
return 0;
 }
@@ -1037,8 +1030,7 @@ __copy_xstate_to_user(unsigned int pos, unsigned int 
count,
  * zero. This is called from xstateregs_get() and there we check the CPU
  * has XSAVES.
  */
-int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf,
-   void __user *ubuf, struct xregs_state *xsave)
+int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user 
*ubuf, struct xregs_state *xsave)
 {
unsigned int offset, size;
int ret, i;
@@ -1063,8 +1055,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int 
count, void *kbuf,
offset = offsetof(struct xregs_state, header);
size = sizeof(header);
 
-   ret = __copy_xstate_to_user(offset, size, kbuf, ubuf, , 0, 
count);
-
+   ret = __copy_xstate_to_user(offset, size, ubuf, , 0, count);
if (ret)
return ret;
 
@@ -1078,8 +1069,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int 
count, void *kbuf,
offset = xstate_offsets[i];
size = xstate_sizes[i];
 
-   ret = __copy_xstate_to_user(offset, size, kbuf, ubuf, 
src, 0, count);
-
+   ret = __copy_xstate_to_user(offset, size, ubuf, src, 0, 
count);
if (ret)
return ret;
 
@@ -1095,8 +1085,7

[PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

2017-01-26 Thread Ingo Molnar 
The 'kbuf' parameter is unused in the _user() side of the API, remove it.

This simplifies the code and makes it easier to think about.

Cc: Andy Lutomirski 
Cc: Borislav Petkov 
Cc: Dave Hansen 
Cc: Fenghua Yu 
Cc: H. Peter Anvin 
Cc: Linus Torvalds 
Cc: Oleg Nesterov 
Cc: Rik van Riel 
Cc: Thomas Gleixner 
Cc: Yu-cheng Yu 
Cc: Fenghua Yu 
Signed-off-by: Ingo Molnar 
---
 arch/x86/include/asm/fpu/xstate.h |  2 +-
 arch/x86/kernel/fpu/regset.c  |  2 +-
 arch/x86/kernel/fpu/xstate.c  | 25 +++--
 3 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h 
b/arch/x86/include/asm/fpu/xstate.h
index c762574a245f..65bd68c30cd0 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -49,7 +49,7 @@ void *get_xsave_addr(struct xregs_state *xsave, int xstate);
 const void *get_xsave_field_ptr(int xstate_field);
 int using_compacted_format(void);
 int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, 
struct xregs_state *xsave);
-int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf, void 
__user *ubuf, struct xregs_state *xsave);
+int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user 
*ubuf, struct xregs_state *xsave);
 int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
 struct xregs_state *xsave);
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index a5f2b38a47dc..62a2d1154e45 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -94,7 +94,7 @@ int xstateregs_get(struct task_struct *target, const struct 
user_regset *regset,
if (kbuf)
ret = copy_xstate_to_kernel(pos, count, kbuf, xsave);
else
-   ret = copy_xstate_to_user(pos, count, kbuf, ubuf, 
xsave);
+   ret = copy_xstate_to_user(pos, count, ubuf, xsave);
} else {
fpstate_sanitize_xstate(fpu);
/*
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 6ac8e4a759ff..1ac3d2ae73e6 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int 
count, void *kbuf, stru
 }
 
 static inline int
-__copy_xstate_to_user(unsigned int pos, unsigned int count,
- void *kbuf, void __user *ubuf,
- const void *data, const int start_pos,
- const int end_pos)
+__copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, 
const void *data, const int start_pos, const int end_pos)
 {
if ((count == 0) || (pos < start_pos))
return 0;
@@ -1021,12 +1018,8 @@ __copy_xstate_to_user(unsigned int pos, unsigned int 
count,
if (end_pos < 0 || pos < end_pos) {
unsigned int copy = (end_pos < 0 ? count : min(count, end_pos - 
pos));
 
-   if (kbuf) {
-   memcpy(kbuf + pos, data, copy);
-   } else {
-   if (__copy_to_user(ubuf + pos, data, copy))
-   return -EFAULT;
-   }
+   if (__copy_to_user(ubuf + pos, data, copy))
+   return -EFAULT;
}
return 0;
 }
@@ -1037,8 +1030,7 @@ __copy_xstate_to_user(unsigned int pos, unsigned int 
count,
  * zero. This is called from xstateregs_get() and there we check the CPU
  * has XSAVES.
  */
-int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf,
-   void __user *ubuf, struct xregs_state *xsave)
+int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user 
*ubuf, struct xregs_state *xsave)
 {
unsigned int offset, size;
int ret, i;
@@ -1063,8 +1055,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int 
count, void *kbuf,
offset = offsetof(struct xregs_state, header);
size = sizeof(header);
 
-   ret = __copy_xstate_to_user(offset, size, kbuf, ubuf, , 0, 
count);
-
+   ret = __copy_xstate_to_user(offset, size, ubuf, , 0, count);
if (ret)
return ret;
 
@@ -1078,8 +1069,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int 
count, void *kbuf,
offset = xstate_offsets[i];
size = xstate_sizes[i];
 
-   ret = __copy_xstate_to_user(offset, size, kbuf, ubuf, 
src, 0, count);
-
+   ret = __copy_xstate_to_user(offset, size, ubuf, src, 0, 
count);
if (ret)
return ret;
 
@@ -1095,8 +1085,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int 
count, void *kbuf,
offset = offsetof(struct fxregs_state, sw_reserved);
size = sizeof(xstate_fx_sw_bytes);
 
-   ret = __copy_xstate_to_user(offset, size, kbuf, ubuf,