Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 05/05/2015 10:27 AM, Borislav Petkov wrote: > Yeah, I said "Applied" but didn't know at the time Ingo was doing the > big FPU cleanup: > > https://lkml.kernel.org/r/1430843228-13749-1-git-send-email-mi...@kernel.org > > So let's wait until the dust settles, I think rediffing this patch > should be easy and simply made to call fpu__save() which is the new name > but we'll have to doublecheck. Hi Borislav, This fixes a bug present in kernels today. Until we have an ETA for Ingo's stuff to get merged, I'd really hope you can consider taking bugfixes like these. BTW, I didn't really expect you to pull this in directly. I think it's easier if the entire MPX set is pulled in at once. Otherwise, the x86 maintainers will have to drop this from the set before merging, and it might not be 100% obvious how this patch got merged via another route. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 05/05/2015 10:27 AM, Borislav Petkov wrote: Yeah, I said Applied but didn't know at the time Ingo was doing the big FPU cleanup: https://lkml.kernel.org/r/1430843228-13749-1-git-send-email-mi...@kernel.org So let's wait until the dust settles, I think rediffing this patch should be easy and simply made to call fpu__save() which is the new name but we'll have to doublecheck. Hi Borislav, This fixes a bug present in kernels today. Until we have an ETA for Ingo's stuff to get merged, I'd really hope you can consider taking bugfixes like these. BTW, I didn't really expect you to pull this in directly. I think it's easier if the entire MPX set is pulled in at once. Otherwise, the x86 maintainers will have to drop this from the set before merging, and it might not be 100% obvious how this patch got merged via another route. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Sat, Apr 25, 2015 at 11:31:19AM +0200, Borislav Petkov wrote: > On Wed, Apr 22, 2015 at 11:27:31AM -0700, Dave Hansen wrote: > > > > From: Dave Hansen > > > > Changes from "v19": > > * remove 'tsk' argument to get_xsave_addr() since the code > >can only realistically work on 'current', and fix up the > >comment a bit to match. > > > > Changes from "v17": > > * fix s/xstate/xsave_field/ in the function comment > > * remove EXPORT_SYMBOL_GPL() > > > > --- > > From: Dave Hansen > > > > The MPX code appears to be saving off the FPU in an unsafe > > way. It does not disable preemption or ensure that the > > FPU state has been allocated. > > > > This patch introduces a new helper which will do both of > > those things internally. > > > > Note that this requires a patch from Oleg in order to work > > properly. It is currently in tip/x86/fpu. > > > > > commit f893959b0898bd876673adbeb6798bdf25c034d7 > > > Author: Oleg Nesterov > > > Date: Fri Mar 13 18:30:30 2015 +0100 > > > > > >x86/fpu: Don't abuse drop_init_fpu() in flush_thread() > > > > Signed-off-by: Dave Hansen > > Cc: Oleg Nesterov > > Cc: b...@alien8.de > > Cc: Rik van Riel > > Cc: Suresh Siddha > > Cc: Andy Lutomirski > > Cc: Thomas Gleixner > > Cc: Ingo Molnar > > Cc: "H. Peter Anvin" > > Cc: Fenghua Yu > > Cc: the arch/x86 maintainers > > --- > > > > b/arch/x86/include/asm/xsave.h |1 + > > b/arch/x86/kernel/xsave.c | 32 > > 2 files changed, 33 insertions(+) > > Applied, thanks. Yeah, I said "Applied" but didn't know at the time Ingo was doing the big FPU cleanup: https://lkml.kernel.org/r/1430843228-13749-1-git-send-email-mi...@kernel.org So let's wait until the dust settles, I think rediffing this patch should be easy and simply made to call fpu__save() which is the new name but we'll have to doublecheck. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Sat, Apr 25, 2015 at 11:31:19AM +0200, Borislav Petkov wrote: On Wed, Apr 22, 2015 at 11:27:31AM -0700, Dave Hansen wrote: From: Dave Hansen dave.han...@linux.intel.com Changes from v19: * remove 'tsk' argument to get_xsave_addr() since the code can only realistically work on 'current', and fix up the comment a bit to match. Changes from v17: * fix s/xstate/xsave_field/ in the function comment * remove EXPORT_SYMBOL_GPL() --- From: Dave Hansen dave.han...@linux.intel.com The MPX code appears to be saving off the FPU in an unsafe way. It does not disable preemption or ensure that the FPU state has been allocated. This patch introduces a new helper which will do both of those things internally. Note that this requires a patch from Oleg in order to work properly. It is currently in tip/x86/fpu. commit f893959b0898bd876673adbeb6798bdf25c034d7 Author: Oleg Nesterov o...@redhat.com Date: Fri Mar 13 18:30:30 2015 +0100 x86/fpu: Don't abuse drop_init_fpu() in flush_thread() Signed-off-by: Dave Hansen dave.han...@linux.intel.com Cc: Oleg Nesterov o...@redhat.com Cc: b...@alien8.de Cc: Rik van Riel r...@redhat.com Cc: Suresh Siddha sbsid...@gmail.com Cc: Andy Lutomirski l...@amacapital.net Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: Fenghua Yu fenghua...@intel.com Cc: the arch/x86 maintainers x...@kernel.org --- b/arch/x86/include/asm/xsave.h |1 + b/arch/x86/kernel/xsave.c | 32 2 files changed, 33 insertions(+) Applied, thanks. Yeah, I said Applied but didn't know at the time Ingo was doing the big FPU cleanup: https://lkml.kernel.org/r/1430843228-13749-1-git-send-email-mi...@kernel.org So let's wait until the dust settles, I think rediffing this patch should be easy and simply made to call fpu__save() which is the new name but we'll have to doublecheck. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Wed, Apr 22, 2015 at 11:27:31AM -0700, Dave Hansen wrote: > > From: Dave Hansen > > Changes from "v19": > * remove 'tsk' argument to get_xsave_addr() since the code >can only realistically work on 'current', and fix up the >comment a bit to match. > > Changes from "v17": > * fix s/xstate/xsave_field/ in the function comment > * remove EXPORT_SYMBOL_GPL() > > --- > From: Dave Hansen > > The MPX code appears to be saving off the FPU in an unsafe > way. It does not disable preemption or ensure that the > FPU state has been allocated. > > This patch introduces a new helper which will do both of > those things internally. > > Note that this requires a patch from Oleg in order to work > properly. It is currently in tip/x86/fpu. > > > commit f893959b0898bd876673adbeb6798bdf25c034d7 > > Author: Oleg Nesterov > > Date: Fri Mar 13 18:30:30 2015 +0100 > > > >x86/fpu: Don't abuse drop_init_fpu() in flush_thread() > > Signed-off-by: Dave Hansen > Cc: Oleg Nesterov > Cc: b...@alien8.de > Cc: Rik van Riel > Cc: Suresh Siddha > Cc: Andy Lutomirski > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: Fenghua Yu > Cc: the arch/x86 maintainers > --- > > b/arch/x86/include/asm/xsave.h |1 + > b/arch/x86/kernel/xsave.c | 32 > 2 files changed, 33 insertions(+) Applied, thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Wed, Apr 22, 2015 at 11:27:31AM -0700, Dave Hansen wrote: From: Dave Hansen dave.han...@linux.intel.com Changes from v19: * remove 'tsk' argument to get_xsave_addr() since the code can only realistically work on 'current', and fix up the comment a bit to match. Changes from v17: * fix s/xstate/xsave_field/ in the function comment * remove EXPORT_SYMBOL_GPL() --- From: Dave Hansen dave.han...@linux.intel.com The MPX code appears to be saving off the FPU in an unsafe way. It does not disable preemption or ensure that the FPU state has been allocated. This patch introduces a new helper which will do both of those things internally. Note that this requires a patch from Oleg in order to work properly. It is currently in tip/x86/fpu. commit f893959b0898bd876673adbeb6798bdf25c034d7 Author: Oleg Nesterov o...@redhat.com Date: Fri Mar 13 18:30:30 2015 +0100 x86/fpu: Don't abuse drop_init_fpu() in flush_thread() Signed-off-by: Dave Hansen dave.han...@linux.intel.com Cc: Oleg Nesterov o...@redhat.com Cc: b...@alien8.de Cc: Rik van Riel r...@redhat.com Cc: Suresh Siddha sbsid...@gmail.com Cc: Andy Lutomirski l...@amacapital.net Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: Fenghua Yu fenghua...@intel.com Cc: the arch/x86 maintainers x...@kernel.org --- b/arch/x86/include/asm/xsave.h |1 + b/arch/x86/kernel/xsave.c | 32 2 files changed, 33 insertions(+) Applied, thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
From: Dave Hansen
Changes from "v19":
* remove 'tsk' argument to get_xsave_addr() since the code
can only realistically work on 'current', and fix up the
comment a bit to match.
Changes from "v17":
* fix s/xstate/xsave_field/ in the function comment
* remove EXPORT_SYMBOL_GPL()
---
From: Dave Hansen
The MPX code appears to be saving off the FPU in an unsafe
way. It does not disable preemption or ensure that the
FPU state has been allocated.
This patch introduces a new helper which will do both of
those things internally.
Note that this requires a patch from Oleg in order to work
properly. It is currently in tip/x86/fpu.
> commit f893959b0898bd876673adbeb6798bdf25c034d7
> Author: Oleg Nesterov
> Date: Fri Mar 13 18:30:30 2015 +0100
>
>x86/fpu: Don't abuse drop_init_fpu() in flush_thread()
Signed-off-by: Dave Hansen
Cc: Oleg Nesterov
Cc: b...@alien8.de
Cc: Rik van Riel
Cc: Suresh Siddha
Cc: Andy Lutomirski
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: "H. Peter Anvin"
Cc: Fenghua Yu
Cc: the arch/x86 maintainers
---
b/arch/x86/include/asm/xsave.h |1 +
b/arch/x86/kernel/xsave.c | 32
2 files changed, 33 insertions(+)
diff -puN arch/x86/include/asm/xsave.h~tsk_get_xsave_addr
arch/x86/include/asm/xsave.h
--- a/arch/x86/include/asm/xsave.h~tsk_get_xsave_addr 2015-04-22
11:16:17.781800602 -0700
+++ b/arch/x86/include/asm/xsave.h 2015-04-22 11:16:17.786800827 -0700
@@ -252,6 +252,7 @@ static inline int xrestore_user(struct x
}
void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
+void *get_xsave_field(int xstate_field);
void setup_xstate_comp(void);
#endif
diff -puN arch/x86/kernel/xsave.c~tsk_get_xsave_addr arch/x86/kernel/xsave.c
--- a/arch/x86/kernel/xsave.c~tsk_get_xsave_addr2015-04-22
11:16:17.782800647 -0700
+++ b/arch/x86/kernel/xsave.c 2015-04-22 11:16:17.786800827 -0700
@@ -741,3 +741,35 @@ void *get_xsave_addr(struct xsave_struct
return (void *)xsave + xstate_comp_offsets[feature];
}
EXPORT_SYMBOL_GPL(get_xsave_addr);
+
+/*
+ * This wraps up the common operations that need to occur when retrieving
+ * data from xsave state. It first ensures that the current task was
+ * using the FPU and retrieves the data in to a buffer. It then calculates
+ * the offset of the requested field in the buffer.
+ *
+ * This function is safe to call whether the FPU is in use or not.
+ *
+ * Note that this only works on the current task.
+ *
+ * Inputs:
+ * @xsave_field: state which is defined in xsave.h (e.g. XSTATE_FP,
+ * XSTATE_SSE, etc...)
+ * Output:
+ * address of the state in the xsave area.
+ */
+void *get_xsave_field(int xsave_field)
+{
+ union thread_xstate *xstate;
+
+ if (!tsk_used_math(current))
+ return NULL;
+ /*
+* unlazy_fpu() is poorly named and will actually
+* save the xstate off in to the memory buffer.
+*/
+ unlazy_fpu(current);
+ xstate = current->thread.fpu.state;
+
+ return get_xsave_addr(>xsave, xsave_field);
+}
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
From: Dave Hansen dave.han...@linux.intel.com
Changes from v19:
* remove 'tsk' argument to get_xsave_addr() since the code
can only realistically work on 'current', and fix up the
comment a bit to match.
Changes from v17:
* fix s/xstate/xsave_field/ in the function comment
* remove EXPORT_SYMBOL_GPL()
---
From: Dave Hansen dave.han...@linux.intel.com
The MPX code appears to be saving off the FPU in an unsafe
way. It does not disable preemption or ensure that the
FPU state has been allocated.
This patch introduces a new helper which will do both of
those things internally.
Note that this requires a patch from Oleg in order to work
properly. It is currently in tip/x86/fpu.
commit f893959b0898bd876673adbeb6798bdf25c034d7
Author: Oleg Nesterov o...@redhat.com
Date: Fri Mar 13 18:30:30 2015 +0100
x86/fpu: Don't abuse drop_init_fpu() in flush_thread()
Signed-off-by: Dave Hansen dave.han...@linux.intel.com
Cc: Oleg Nesterov o...@redhat.com
Cc: b...@alien8.de
Cc: Rik van Riel r...@redhat.com
Cc: Suresh Siddha sbsid...@gmail.com
Cc: Andy Lutomirski l...@amacapital.net
Cc: Thomas Gleixner t...@linutronix.de
Cc: Ingo Molnar mi...@redhat.com
Cc: H. Peter Anvin h...@zytor.com
Cc: Fenghua Yu fenghua...@intel.com
Cc: the arch/x86 maintainers x...@kernel.org
---
b/arch/x86/include/asm/xsave.h |1 +
b/arch/x86/kernel/xsave.c | 32
2 files changed, 33 insertions(+)
diff -puN arch/x86/include/asm/xsave.h~tsk_get_xsave_addr
arch/x86/include/asm/xsave.h
--- a/arch/x86/include/asm/xsave.h~tsk_get_xsave_addr 2015-04-22
11:16:17.781800602 -0700
+++ b/arch/x86/include/asm/xsave.h 2015-04-22 11:16:17.786800827 -0700
@@ -252,6 +252,7 @@ static inline int xrestore_user(struct x
}
void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
+void *get_xsave_field(int xstate_field);
void setup_xstate_comp(void);
#endif
diff -puN arch/x86/kernel/xsave.c~tsk_get_xsave_addr arch/x86/kernel/xsave.c
--- a/arch/x86/kernel/xsave.c~tsk_get_xsave_addr2015-04-22
11:16:17.782800647 -0700
+++ b/arch/x86/kernel/xsave.c 2015-04-22 11:16:17.786800827 -0700
@@ -741,3 +741,35 @@ void *get_xsave_addr(struct xsave_struct
return (void *)xsave + xstate_comp_offsets[feature];
}
EXPORT_SYMBOL_GPL(get_xsave_addr);
+
+/*
+ * This wraps up the common operations that need to occur when retrieving
+ * data from xsave state. It first ensures that the current task was
+ * using the FPU and retrieves the data in to a buffer. It then calculates
+ * the offset of the requested field in the buffer.
+ *
+ * This function is safe to call whether the FPU is in use or not.
+ *
+ * Note that this only works on the current task.
+ *
+ * Inputs:
+ * @xsave_field: state which is defined in xsave.h (e.g. XSTATE_FP,
+ * XSTATE_SSE, etc...)
+ * Output:
+ * address of the state in the xsave area.
+ */
+void *get_xsave_field(int xsave_field)
+{
+ union thread_xstate *xstate;
+
+ if (!tsk_used_math(current))
+ return NULL;
+ /*
+* unlazy_fpu() is poorly named and will actually
+* save the xstate off in to the memory buffer.
+*/
+ unlazy_fpu(current);
+ xstate = current-thread.fpu.state;
+
+ return get_xsave_addr(xstate-xsave, xsave_field);
+}
_
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
Changes from "v17":
* fix s/xstate/xsave_field/ in the function comment
* remove EXPORT_SYMBOL_GPL()
---
From: Dave Hansen
The MPX code appears to be saving off the FPU in an unsafe
way. It does not disable preemption or ensure that the
FPU state has been allocated.
This patch introduces a new helper which will do both of
those things internally.
Note that this requires a patch from Oleg in order to work
properly. It is currently in tip/x86/fpu.
> commit f893959b0898bd876673adbeb6798bdf25c034d7
> Author: Oleg Nesterov
> Date: Fri Mar 13 18:30:30 2015 +0100
>
>x86/fpu: Don't abuse drop_init_fpu() in flush_thread()
Signed-off-by: Dave Hansen
Cc: Oleg Nesterov
Cc: b...@alien8.de
Cc: Rik van Riel
Cc: Suresh Siddha
Cc: Andy Lutomirski
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: "H. Peter Anvin"
Cc: Fenghua Yu
Cc: the arch/x86 maintainers
---
b/arch/x86/include/asm/xsave.h |1 +
b/arch/x86/kernel/xsave.c | 31 +++
2 files changed, 32 insertions(+)
diff -puN arch/x86/include/asm/xsave.h~tsk_get_xsave_addr
arch/x86/include/asm/xsave.h
--- a/arch/x86/include/asm/xsave.h~tsk_get_xsave_addr 2015-03-27
14:35:03.883722015 -0700
+++ b/arch/x86/include/asm/xsave.h 2015-03-27 14:35:03.888722240 -0700
@@ -252,6 +252,7 @@ static inline int xrestore_user(struct x
}
void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
+void *tsk_get_xsave_field(struct task_struct *tsk, int xstate_field);
void setup_xstate_comp(void);
#endif
diff -puN arch/x86/kernel/xsave.c~tsk_get_xsave_addr arch/x86/kernel/xsave.c
--- a/arch/x86/kernel/xsave.c~tsk_get_xsave_addr2015-03-27
14:35:03.885722105 -0700
+++ b/arch/x86/kernel/xsave.c 2015-03-27 14:35:03.889722286 -0700
@@ -740,3 +740,34 @@ void *get_xsave_addr(struct xsave_struct
return (void *)xsave + xstate_comp_offsets[feature];
}
EXPORT_SYMBOL_GPL(get_xsave_addr);
+
+/*
+ * This wraps up the common operations that need to occur when retrieving
+ * data from an xsave struct. It first ensures that the task was actually
+ * using the FPU and retrieves the data in to a buffer. It then calculates
+ * the offset of the requested field in the buffer.
+ *
+ * This function is safe to call whether the FPU is in use or not.
+ *
+ * Inputs:
+ * @tsk: the task from which we are fetching xsave state
+ * @xsave_field: state which is defined in xsave.h (e.g. XSTATE_FP,
+ * XSTATE_SSE, etc...)
+ * Output:
+ * address of the state in the xsave area.
+ */
+void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
+{
+ union thread_xstate *xstate;
+
+ if (!used_math())
+ return NULL;
+ /*
+* unlazy_fpu() is poorly named and will actually
+* save the xstate off in to the memory buffer.
+*/
+ unlazy_fpu(tsk);
+ xstate = tsk->thread.fpu.state;
+
+ return get_xsave_addr(>xsave, xsave_field);
+}
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
Just in case, 1, 2, and 9 looks good to me.
I didn't get the rest of this series, but I am sure it doesn't need
my review ;)
On 03/26, Dave Hansen wrote:
>
> From: Dave Hansen
>
> The MPX code appears to be saving off the FPU in an unsafe
> way. It does not disable preemption or ensure that the
> FPU state has been allocated.
>
> This patch introduces a new helper which will do both of
> those things internally.
>
> Note that this requires a patch from Oleg in order to work
> properly. It is currently in tip/x86/fpu.
>
> > commit f893959b0898bd876673adbeb6798bdf25c034d7
> > Author: Oleg Nesterov
> > Date: Fri Mar 13 18:30:30 2015 +0100
> >
> >x86/fpu: Don't abuse drop_init_fpu() in flush_thread()
>
> Signed-off-by: Dave Hansen
> Cc: Oleg Nesterov
> Cc: b...@alien8.de
> Cc: Rik van Riel
> Cc: Suresh Siddha
> Cc: Andy Lutomirski
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: "H. Peter Anvin"
> Cc: Fenghua Yu
> Cc: the arch/x86 maintainers
> ---
>
> b/arch/x86/include/asm/xsave.h |1 +
> b/arch/x86/kernel/xsave.c | 32
> 2 files changed, 33 insertions(+)
>
> diff -puN arch/x86/include/asm/xsave.h~tsk_get_xsave_addr
> arch/x86/include/asm/xsave.h
> --- a/arch/x86/include/asm/xsave.h~tsk_get_xsave_addr 2015-03-26
> 11:27:04.738204327 -0700
> +++ b/arch/x86/include/asm/xsave.h2015-03-26 11:27:04.743204552 -0700
> @@ -252,6 +252,7 @@ static inline int xrestore_user(struct x
> }
>
> void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
> +void *tsk_get_xsave_field(struct task_struct *tsk, int xstate_field);
> void setup_xstate_comp(void);
>
> #endif
> diff -puN arch/x86/kernel/xsave.c~tsk_get_xsave_addr arch/x86/kernel/xsave.c
> --- a/arch/x86/kernel/xsave.c~tsk_get_xsave_addr 2015-03-26
> 11:27:04.740204417 -0700
> +++ b/arch/x86/kernel/xsave.c 2015-03-26 11:27:04.744204597 -0700
> @@ -740,3 +740,35 @@ void *get_xsave_addr(struct xsave_struct
> return (void *)xsave + xstate_comp_offsets[feature];
> }
> EXPORT_SYMBOL_GPL(get_xsave_addr);
> +
> +/*
> + * This wraps up the common operations that need to occur when retrieving
> + * data from an xsave struct. It first ensures that the task was actually
> + * using the FPU and retrieves the data in to a buffer. It then calculates
> + * the offset of the requested field in the buffer.
> + *
> + * This function is safe to call whether the FPU is in use or not.
> + *
> + * Inputs:
> + * tsk: the task from which we are fetching xsave state
> + * xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
> + * etc.)
> + * Output:
> + * address of the state in the xsave area.
> + */
> +void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
> +{
> + union thread_xstate *xstate;
> +
> + if (!used_math())
> + return NULL;
> + /*
> + * unlazy_fpu() is poorly named and will actually
> + * save the xstate off in to the memory buffer.
> + */
> + unlazy_fpu(tsk);
> + xstate = tsk->thread.fpu.state;
> +
> + return get_xsave_addr(>xsave, xsave_field);
> +}
> +EXPORT_SYMBOL_GPL(tsk_get_xsave_field);
> _
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/27/2015 08:15 AM, Borislav Petkov wrote: > I'm not sure we want to export this to modules... and MPX cannot be > built as a module either. So why export it? The thing I was wrapping (get_xsave_addr()) was EXPORT_SYMBOL_GPL(). I assumed that it was exported for good reason and that my wrapper should follow suit. I do expect this to get used by more things than just MPX going forward. But, I guess folks can just export it if and when they need it. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Thu, Mar 26, 2015 at 11:33:34AM -0700, Dave Hansen wrote:
>
> From: Dave Hansen
>
> The MPX code appears to be saving off the FPU in an unsafe
> way. It does not disable preemption or ensure that the
> FPU state has been allocated.
>
> This patch introduces a new helper which will do both of
> those things internally.
>
> Note that this requires a patch from Oleg in order to work
> properly. It is currently in tip/x86/fpu.
>
> > commit f893959b0898bd876673adbeb6798bdf25c034d7
> > Author: Oleg Nesterov
> > Date: Fri Mar 13 18:30:30 2015 +0100
> >
> >x86/fpu: Don't abuse drop_init_fpu() in flush_thread()
>
> Signed-off-by: Dave Hansen
> Cc: Oleg Nesterov
> Cc: b...@alien8.de
> Cc: Rik van Riel
> Cc: Suresh Siddha
> Cc: Andy Lutomirski
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: "H. Peter Anvin"
> Cc: Fenghua Yu
> Cc: the arch/x86 maintainers
> ---
>
> b/arch/x86/include/asm/xsave.h |1 +
> b/arch/x86/kernel/xsave.c | 32
> 2 files changed, 33 insertions(+)
>
> diff -puN arch/x86/include/asm/xsave.h~tsk_get_xsave_addr
> arch/x86/include/asm/xsave.h
> --- a/arch/x86/include/asm/xsave.h~tsk_get_xsave_addr 2015-03-26
> 11:27:04.738204327 -0700
> +++ b/arch/x86/include/asm/xsave.h2015-03-26 11:27:04.743204552 -0700
> @@ -252,6 +252,7 @@ static inline int xrestore_user(struct x
> }
>
> void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
> +void *tsk_get_xsave_field(struct task_struct *tsk, int xstate_field);
> void setup_xstate_comp(void);
>
> #endif
> diff -puN arch/x86/kernel/xsave.c~tsk_get_xsave_addr arch/x86/kernel/xsave.c
> --- a/arch/x86/kernel/xsave.c~tsk_get_xsave_addr 2015-03-26
> 11:27:04.740204417 -0700
> +++ b/arch/x86/kernel/xsave.c 2015-03-26 11:27:04.744204597 -0700
> @@ -740,3 +740,35 @@ void *get_xsave_addr(struct xsave_struct
> return (void *)xsave + xstate_comp_offsets[feature];
> }
> EXPORT_SYMBOL_GPL(get_xsave_addr);
> +
> +/*
> + * This wraps up the common operations that need to occur when retrieving
> + * data from an xsave struct. It first ensures that the task was actually
> + * using the FPU and retrieves the data in to a buffer. It then calculates
> + * the offset of the requested field in the buffer.
> + *
> + * This function is safe to call whether the FPU is in use or not.
> + *
> + * Inputs:
> + * tsk: the task from which we are fetching xsave state
> + * xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
I think you mean @xsave_field here?
> + * etc.)
> + * Output:
> + * address of the state in the xsave area.
> + */
> +void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
> +{
> + union thread_xstate *xstate;
> +
> + if (!used_math())
> + return NULL;
> + /*
> + * unlazy_fpu() is poorly named and will actually
> + * save the xstate off in to the memory buffer.
> + */
> + unlazy_fpu(tsk);
> + xstate = tsk->thread.fpu.state;
> +
> + return get_xsave_addr(>xsave, xsave_field);
> +}
> +EXPORT_SYMBOL_GPL(tsk_get_xsave_field);
I'm not sure we want to export this to modules... and MPX cannot be
built as a module either. So why export it?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Thu, Mar 26, 2015 at 11:33:34AM -0700, Dave Hansen wrote:
From: Dave Hansen dave.han...@linux.intel.com
The MPX code appears to be saving off the FPU in an unsafe
way. It does not disable preemption or ensure that the
FPU state has been allocated.
This patch introduces a new helper which will do both of
those things internally.
Note that this requires a patch from Oleg in order to work
properly. It is currently in tip/x86/fpu.
commit f893959b0898bd876673adbeb6798bdf25c034d7
Author: Oleg Nesterov o...@redhat.com
Date: Fri Mar 13 18:30:30 2015 +0100
x86/fpu: Don't abuse drop_init_fpu() in flush_thread()
Signed-off-by: Dave Hansen dave.han...@linux.intel.com
Cc: Oleg Nesterov o...@redhat.com
Cc: b...@alien8.de
Cc: Rik van Riel r...@redhat.com
Cc: Suresh Siddha sbsid...@gmail.com
Cc: Andy Lutomirski l...@amacapital.net
Cc: Thomas Gleixner t...@linutronix.de
Cc: Ingo Molnar mi...@redhat.com
Cc: H. Peter Anvin h...@zytor.com
Cc: Fenghua Yu fenghua...@intel.com
Cc: the arch/x86 maintainers x...@kernel.org
---
b/arch/x86/include/asm/xsave.h |1 +
b/arch/x86/kernel/xsave.c | 32
2 files changed, 33 insertions(+)
diff -puN arch/x86/include/asm/xsave.h~tsk_get_xsave_addr
arch/x86/include/asm/xsave.h
--- a/arch/x86/include/asm/xsave.h~tsk_get_xsave_addr 2015-03-26
11:27:04.738204327 -0700
+++ b/arch/x86/include/asm/xsave.h2015-03-26 11:27:04.743204552 -0700
@@ -252,6 +252,7 @@ static inline int xrestore_user(struct x
}
void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
+void *tsk_get_xsave_field(struct task_struct *tsk, int xstate_field);
void setup_xstate_comp(void);
#endif
diff -puN arch/x86/kernel/xsave.c~tsk_get_xsave_addr arch/x86/kernel/xsave.c
--- a/arch/x86/kernel/xsave.c~tsk_get_xsave_addr 2015-03-26
11:27:04.740204417 -0700
+++ b/arch/x86/kernel/xsave.c 2015-03-26 11:27:04.744204597 -0700
@@ -740,3 +740,35 @@ void *get_xsave_addr(struct xsave_struct
return (void *)xsave + xstate_comp_offsets[feature];
}
EXPORT_SYMBOL_GPL(get_xsave_addr);
+
+/*
+ * This wraps up the common operations that need to occur when retrieving
+ * data from an xsave struct. It first ensures that the task was actually
+ * using the FPU and retrieves the data in to a buffer. It then calculates
+ * the offset of the requested field in the buffer.
+ *
+ * This function is safe to call whether the FPU is in use or not.
+ *
+ * Inputs:
+ * tsk: the task from which we are fetching xsave state
+ * xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
I think you mean @xsave_field here?
+ * etc.)
+ * Output:
+ * address of the state in the xsave area.
+ */
+void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
+{
+ union thread_xstate *xstate;
+
+ if (!used_math())
+ return NULL;
+ /*
+ * unlazy_fpu() is poorly named and will actually
+ * save the xstate off in to the memory buffer.
+ */
+ unlazy_fpu(tsk);
+ xstate = tsk-thread.fpu.state;
+
+ return get_xsave_addr(xstate-xsave, xsave_field);
+}
+EXPORT_SYMBOL_GPL(tsk_get_xsave_field);
I'm not sure we want to export this to modules... and MPX cannot be
built as a module either. So why export it?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/27/2015 08:15 AM, Borislav Petkov wrote: I'm not sure we want to export this to modules... and MPX cannot be built as a module either. So why export it? The thing I was wrapping (get_xsave_addr()) was EXPORT_SYMBOL_GPL(). I assumed that it was exported for good reason and that my wrapper should follow suit. I do expect this to get used by more things than just MPX going forward. But, I guess folks can just export it if and when they need it. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
Just in case, 1, 2, and 9 looks good to me.
I didn't get the rest of this series, but I am sure it doesn't need
my review ;)
On 03/26, Dave Hansen wrote:
From: Dave Hansen dave.han...@linux.intel.com
The MPX code appears to be saving off the FPU in an unsafe
way. It does not disable preemption or ensure that the
FPU state has been allocated.
This patch introduces a new helper which will do both of
those things internally.
Note that this requires a patch from Oleg in order to work
properly. It is currently in tip/x86/fpu.
commit f893959b0898bd876673adbeb6798bdf25c034d7
Author: Oleg Nesterov o...@redhat.com
Date: Fri Mar 13 18:30:30 2015 +0100
x86/fpu: Don't abuse drop_init_fpu() in flush_thread()
Signed-off-by: Dave Hansen dave.han...@linux.intel.com
Cc: Oleg Nesterov o...@redhat.com
Cc: b...@alien8.de
Cc: Rik van Riel r...@redhat.com
Cc: Suresh Siddha sbsid...@gmail.com
Cc: Andy Lutomirski l...@amacapital.net
Cc: Thomas Gleixner t...@linutronix.de
Cc: Ingo Molnar mi...@redhat.com
Cc: H. Peter Anvin h...@zytor.com
Cc: Fenghua Yu fenghua...@intel.com
Cc: the arch/x86 maintainers x...@kernel.org
---
b/arch/x86/include/asm/xsave.h |1 +
b/arch/x86/kernel/xsave.c | 32
2 files changed, 33 insertions(+)
diff -puN arch/x86/include/asm/xsave.h~tsk_get_xsave_addr
arch/x86/include/asm/xsave.h
--- a/arch/x86/include/asm/xsave.h~tsk_get_xsave_addr 2015-03-26
11:27:04.738204327 -0700
+++ b/arch/x86/include/asm/xsave.h2015-03-26 11:27:04.743204552 -0700
@@ -252,6 +252,7 @@ static inline int xrestore_user(struct x
}
void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
+void *tsk_get_xsave_field(struct task_struct *tsk, int xstate_field);
void setup_xstate_comp(void);
#endif
diff -puN arch/x86/kernel/xsave.c~tsk_get_xsave_addr arch/x86/kernel/xsave.c
--- a/arch/x86/kernel/xsave.c~tsk_get_xsave_addr 2015-03-26
11:27:04.740204417 -0700
+++ b/arch/x86/kernel/xsave.c 2015-03-26 11:27:04.744204597 -0700
@@ -740,3 +740,35 @@ void *get_xsave_addr(struct xsave_struct
return (void *)xsave + xstate_comp_offsets[feature];
}
EXPORT_SYMBOL_GPL(get_xsave_addr);
+
+/*
+ * This wraps up the common operations that need to occur when retrieving
+ * data from an xsave struct. It first ensures that the task was actually
+ * using the FPU and retrieves the data in to a buffer. It then calculates
+ * the offset of the requested field in the buffer.
+ *
+ * This function is safe to call whether the FPU is in use or not.
+ *
+ * Inputs:
+ * tsk: the task from which we are fetching xsave state
+ * xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
+ * etc.)
+ * Output:
+ * address of the state in the xsave area.
+ */
+void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
+{
+ union thread_xstate *xstate;
+
+ if (!used_math())
+ return NULL;
+ /*
+ * unlazy_fpu() is poorly named and will actually
+ * save the xstate off in to the memory buffer.
+ */
+ unlazy_fpu(tsk);
+ xstate = tsk-thread.fpu.state;
+
+ return get_xsave_addr(xstate-xsave, xsave_field);
+}
+EXPORT_SYMBOL_GPL(tsk_get_xsave_field);
_
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
Changes from v17:
* fix s/xstate/xsave_field/ in the function comment
* remove EXPORT_SYMBOL_GPL()
---
From: Dave Hansen dave.han...@linux.intel.com
The MPX code appears to be saving off the FPU in an unsafe
way. It does not disable preemption or ensure that the
FPU state has been allocated.
This patch introduces a new helper which will do both of
those things internally.
Note that this requires a patch from Oleg in order to work
properly. It is currently in tip/x86/fpu.
commit f893959b0898bd876673adbeb6798bdf25c034d7
Author: Oleg Nesterov o...@redhat.com
Date: Fri Mar 13 18:30:30 2015 +0100
x86/fpu: Don't abuse drop_init_fpu() in flush_thread()
Signed-off-by: Dave Hansen dave.han...@linux.intel.com
Cc: Oleg Nesterov o...@redhat.com
Cc: b...@alien8.de
Cc: Rik van Riel r...@redhat.com
Cc: Suresh Siddha sbsid...@gmail.com
Cc: Andy Lutomirski l...@amacapital.net
Cc: Thomas Gleixner t...@linutronix.de
Cc: Ingo Molnar mi...@redhat.com
Cc: H. Peter Anvin h...@zytor.com
Cc: Fenghua Yu fenghua...@intel.com
Cc: the arch/x86 maintainers x...@kernel.org
---
b/arch/x86/include/asm/xsave.h |1 +
b/arch/x86/kernel/xsave.c | 31 +++
2 files changed, 32 insertions(+)
diff -puN arch/x86/include/asm/xsave.h~tsk_get_xsave_addr
arch/x86/include/asm/xsave.h
--- a/arch/x86/include/asm/xsave.h~tsk_get_xsave_addr 2015-03-27
14:35:03.883722015 -0700
+++ b/arch/x86/include/asm/xsave.h 2015-03-27 14:35:03.888722240 -0700
@@ -252,6 +252,7 @@ static inline int xrestore_user(struct x
}
void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
+void *tsk_get_xsave_field(struct task_struct *tsk, int xstate_field);
void setup_xstate_comp(void);
#endif
diff -puN arch/x86/kernel/xsave.c~tsk_get_xsave_addr arch/x86/kernel/xsave.c
--- a/arch/x86/kernel/xsave.c~tsk_get_xsave_addr2015-03-27
14:35:03.885722105 -0700
+++ b/arch/x86/kernel/xsave.c 2015-03-27 14:35:03.889722286 -0700
@@ -740,3 +740,34 @@ void *get_xsave_addr(struct xsave_struct
return (void *)xsave + xstate_comp_offsets[feature];
}
EXPORT_SYMBOL_GPL(get_xsave_addr);
+
+/*
+ * This wraps up the common operations that need to occur when retrieving
+ * data from an xsave struct. It first ensures that the task was actually
+ * using the FPU and retrieves the data in to a buffer. It then calculates
+ * the offset of the requested field in the buffer.
+ *
+ * This function is safe to call whether the FPU is in use or not.
+ *
+ * Inputs:
+ * @tsk: the task from which we are fetching xsave state
+ * @xsave_field: state which is defined in xsave.h (e.g. XSTATE_FP,
+ * XSTATE_SSE, etc...)
+ * Output:
+ * address of the state in the xsave area.
+ */
+void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
+{
+ union thread_xstate *xstate;
+
+ if (!used_math())
+ return NULL;
+ /*
+* unlazy_fpu() is poorly named and will actually
+* save the xstate off in to the memory buffer.
+*/
+ unlazy_fpu(tsk);
+ xstate = tsk-thread.fpu.state;
+
+ return get_xsave_addr(xstate-xsave, xsave_field);
+}
_
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
From: Dave Hansen
The MPX code appears to be saving off the FPU in an unsafe
way. It does not disable preemption or ensure that the
FPU state has been allocated.
This patch introduces a new helper which will do both of
those things internally.
Note that this requires a patch from Oleg in order to work
properly. It is currently in tip/x86/fpu.
> commit f893959b0898bd876673adbeb6798bdf25c034d7
> Author: Oleg Nesterov
> Date: Fri Mar 13 18:30:30 2015 +0100
>
>x86/fpu: Don't abuse drop_init_fpu() in flush_thread()
Signed-off-by: Dave Hansen
Cc: Oleg Nesterov
Cc: b...@alien8.de
Cc: Rik van Riel
Cc: Suresh Siddha
Cc: Andy Lutomirski
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: "H. Peter Anvin"
Cc: Fenghua Yu
Cc: the arch/x86 maintainers
---
b/arch/x86/include/asm/xsave.h |1 +
b/arch/x86/kernel/xsave.c | 32
2 files changed, 33 insertions(+)
diff -puN arch/x86/include/asm/xsave.h~tsk_get_xsave_addr
arch/x86/include/asm/xsave.h
--- a/arch/x86/include/asm/xsave.h~tsk_get_xsave_addr 2015-03-26
11:27:04.738204327 -0700
+++ b/arch/x86/include/asm/xsave.h 2015-03-26 11:27:04.743204552 -0700
@@ -252,6 +252,7 @@ static inline int xrestore_user(struct x
}
void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
+void *tsk_get_xsave_field(struct task_struct *tsk, int xstate_field);
void setup_xstate_comp(void);
#endif
diff -puN arch/x86/kernel/xsave.c~tsk_get_xsave_addr arch/x86/kernel/xsave.c
--- a/arch/x86/kernel/xsave.c~tsk_get_xsave_addr2015-03-26
11:27:04.740204417 -0700
+++ b/arch/x86/kernel/xsave.c 2015-03-26 11:27:04.744204597 -0700
@@ -740,3 +740,35 @@ void *get_xsave_addr(struct xsave_struct
return (void *)xsave + xstate_comp_offsets[feature];
}
EXPORT_SYMBOL_GPL(get_xsave_addr);
+
+/*
+ * This wraps up the common operations that need to occur when retrieving
+ * data from an xsave struct. It first ensures that the task was actually
+ * using the FPU and retrieves the data in to a buffer. It then calculates
+ * the offset of the requested field in the buffer.
+ *
+ * This function is safe to call whether the FPU is in use or not.
+ *
+ * Inputs:
+ * tsk: the task from which we are fetching xsave state
+ * xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
+ * etc.)
+ * Output:
+ * address of the state in the xsave area.
+ */
+void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
+{
+ union thread_xstate *xstate;
+
+ if (!used_math())
+ return NULL;
+ /*
+* unlazy_fpu() is poorly named and will actually
+* save the xstate off in to the memory buffer.
+*/
+ unlazy_fpu(tsk);
+ xstate = tsk->thread.fpu.state;
+
+ return get_xsave_addr(>xsave, xsave_field);
+}
+EXPORT_SYMBOL_GPL(tsk_get_xsave_field);
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
From: Dave Hansen dave.han...@linux.intel.com
The MPX code appears to be saving off the FPU in an unsafe
way. It does not disable preemption or ensure that the
FPU state has been allocated.
This patch introduces a new helper which will do both of
those things internally.
Note that this requires a patch from Oleg in order to work
properly. It is currently in tip/x86/fpu.
commit f893959b0898bd876673adbeb6798bdf25c034d7
Author: Oleg Nesterov o...@redhat.com
Date: Fri Mar 13 18:30:30 2015 +0100
x86/fpu: Don't abuse drop_init_fpu() in flush_thread()
Signed-off-by: Dave Hansen dave.han...@linux.intel.com
Cc: Oleg Nesterov o...@redhat.com
Cc: b...@alien8.de
Cc: Rik van Riel r...@redhat.com
Cc: Suresh Siddha sbsid...@gmail.com
Cc: Andy Lutomirski l...@amacapital.net
Cc: Thomas Gleixner t...@linutronix.de
Cc: Ingo Molnar mi...@redhat.com
Cc: H. Peter Anvin h...@zytor.com
Cc: Fenghua Yu fenghua...@intel.com
Cc: the arch/x86 maintainers x...@kernel.org
---
b/arch/x86/include/asm/xsave.h |1 +
b/arch/x86/kernel/xsave.c | 32
2 files changed, 33 insertions(+)
diff -puN arch/x86/include/asm/xsave.h~tsk_get_xsave_addr
arch/x86/include/asm/xsave.h
--- a/arch/x86/include/asm/xsave.h~tsk_get_xsave_addr 2015-03-26
11:27:04.738204327 -0700
+++ b/arch/x86/include/asm/xsave.h 2015-03-26 11:27:04.743204552 -0700
@@ -252,6 +252,7 @@ static inline int xrestore_user(struct x
}
void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
+void *tsk_get_xsave_field(struct task_struct *tsk, int xstate_field);
void setup_xstate_comp(void);
#endif
diff -puN arch/x86/kernel/xsave.c~tsk_get_xsave_addr arch/x86/kernel/xsave.c
--- a/arch/x86/kernel/xsave.c~tsk_get_xsave_addr2015-03-26
11:27:04.740204417 -0700
+++ b/arch/x86/kernel/xsave.c 2015-03-26 11:27:04.744204597 -0700
@@ -740,3 +740,35 @@ void *get_xsave_addr(struct xsave_struct
return (void *)xsave + xstate_comp_offsets[feature];
}
EXPORT_SYMBOL_GPL(get_xsave_addr);
+
+/*
+ * This wraps up the common operations that need to occur when retrieving
+ * data from an xsave struct. It first ensures that the task was actually
+ * using the FPU and retrieves the data in to a buffer. It then calculates
+ * the offset of the requested field in the buffer.
+ *
+ * This function is safe to call whether the FPU is in use or not.
+ *
+ * Inputs:
+ * tsk: the task from which we are fetching xsave state
+ * xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
+ * etc.)
+ * Output:
+ * address of the state in the xsave area.
+ */
+void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
+{
+ union thread_xstate *xstate;
+
+ if (!used_math())
+ return NULL;
+ /*
+* unlazy_fpu() is poorly named and will actually
+* save the xstate off in to the memory buffer.
+*/
+ unlazy_fpu(tsk);
+ xstate = tsk-thread.fpu.state;
+
+ return get_xsave_addr(xstate-xsave, xsave_field);
+}
+EXPORT_SYMBOL_GPL(tsk_get_xsave_field);
_
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
Hi Dave, On 03/25, Dave Hansen wrote: > > It may get > called on CPUs without eager FPU mode on. > > > http://git.kernel.org/cgit/linux/kernel/git/daveh/x86-mpx.git/commit/?h=mpx-v16=92d3e7c1664f766142904904e27e126888adb8a7 > > http://git.kernel.org/cgit/linux/kernel/git/daveh/x86-mpx.git/commit/?h=mpx-v16=18049953ae43a7ffa084a01613c1684bdf24dd2e > > All that the MPX code wants here is to read the in-memory copy of the > MPX registers, or error out. Yes, iirc we alredy discussed these fixes ? I still think that the "if (!xstate)" check at the start of tsk_get_xsave_field() will look better, but this is cosmetic. > So, for the purposes of this series: > > With the (so far unmerged to Linus's tree) changes to unlazy_fpu(), does > tsk_get_xsave_field()'s use of unlazy_fpu() look correct? I think yes. But let me remind just in case that this depends on "x86, fpu: unlazy_fpu: don't do __thread_fpu_end() if use_eager_fpu()". > Should we also be renaming tsk_get_xsave_field() to something more > appropriate? Oh, don't ask me ;) To me it looks fine. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/25/2015 05:45 AM, Oleg Nesterov wrote: > So far I do not understand this discussion ;) I didn't see the patches > and other emails... Hi Oleg, My patch set apparently didn't make it to LKML, but here are the two relevant ones. We're essentially replacing the MPX use of fpu_save_init(). CPUs with MPX should entirely have eager FPU mode on. But, the edges of the MPX code (do_bounds()) will call this to distinguish a plain #BR exception from a #BR caused by MPX. It may get called on CPUs without eager FPU mode on. > http://git.kernel.org/cgit/linux/kernel/git/daveh/x86-mpx.git/commit/?h=mpx-v16=92d3e7c1664f766142904904e27e126888adb8a7 > http://git.kernel.org/cgit/linux/kernel/git/daveh/x86-mpx.git/commit/?h=mpx-v16=18049953ae43a7ffa084a01613c1684bdf24dd2e All that the MPX code wants here is to read the in-memory copy of the MPX registers, or error out. So, for the purposes of this series: With the (so far unmerged to Linus's tree) changes to unlazy_fpu(), does tsk_get_xsave_field()'s use of unlazy_fpu() look correct? Should we also be renaming tsk_get_xsave_field() to something more appropriate? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/25, Borislav Petkov wrote: > > On Tue, Mar 24, 2015 at 09:01:44PM -0400, Rik van Riel wrote: > > Indeed, __save_init_fpu (yeah, terrible name) will save > > the in-register state to memory for you, so you can > > inspect it. > > > > Is there any reason not to rename __save_init_fpu to > > save_fpu_state, or just save_fpu? > > That whole place there needs more rubbing. > > So the way I see it, the "init" thing also says that the FPU is intact. Yes, this is my understanding too. And note that nobody actually wants this "init" part, so it actually means "destroy". I agree we should rename it later (at least). Plus unlazy_fpu() looks confusing too. Nobody actually wants to "unlazy", the callers want to save FPU state. So it could be named save_fpu_state() too ;) Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/24, Rik van Riel wrote: > > On 03/24/2015 08:18 PM, Andy Lutomirski wrote: > Is there any reason not to rename __save_init_fpu to > save_fpu_state, or just save_fpu? Please see another email. __save_init_fpu() saves the FPU state but it can also change the FPU registers. At least I think so ;) Otherwise, why switch_fpu_prepare() resets .last_cpu if it returns zero? And unlazy_fpu() should do the same. This all needs the cleanups, yes. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
So far I do not understand this discussion ;) I didn't see the patches
and other emails...
On 03/24, Andy Lutomirski wrote:
>
> On Tue, Mar 24, 2015 at 5:12 PM, Dave Hansen wrote:
> >>
> >> So why are you unlazying it?
> >
> > Oleg actually suggested it.
perhaps you are talking about math_error() ? It uses save_init_fpu()
in Linus's tree, but this is wrong. It was changed in tip/x86/fpu
to use unlazy_fpu() and save_init_fpu() was killed. Plus, just in
case, unlazy_fpu() was changed too and now it is
void unlazy_fpu(struct task_struct *tsk)
{
preempt_disable();
if (__thread_has_fpu(tsk)) {
if (use_eager_fpu()) {
__save_fpu(tsk);
} else {
__save_init_fpu(tsk);
__thread_fpu_end(tsk);
}
}
preempt_enable();
}
and in fact (I think) it needs another change later, __thread_fpu_end()
should depend on __save_init_fpu().
> >> IIUC, the xstae for current can be in one of three logical states:
> >>
> >> 1. Live in CPU regs. The in-memory copy is garbage and the state is
> >> in CPU regs.
> >> 2. Lazy. The in-memory copy and the CPU regs match. Writing to
> >> either copy is illegal.
> >> 3. In memory only. Writing to the in-memory copy is safe.
> >>
> >> IIUC, you want to read the xstate, do you're okay with #2 or #3. This
> >> would be tsk_get_xsave_field_for_read in my terminology.
> >>
> >> If you want to write the xstate, you'd need to be in state #3, which
> >> would be tsk_get_xsave_field_for_write.
> >>
> >> IIUC, unlazy_fpu just moves from from state 2 to 3.
> >
> > I won't completely claim to understand what's going on with the FPU
> > code, but I think your analysis is a bit off.
> >
> > unlazy_fpu() does __save_init_fpu() which (among other things) calls
> > xsave to dump the CPU registers to memory. That doesn't make any sense
> > to do if "The in-memory copy and the CPU regs match."
> >
> > IOW, unlazy_fpu() is called when the in-memory copy is garbage and takes
> > us to a state where we can look at the in-memory copy.
Not necessarily, but most probably I misunderstood... If __thread_has_fpu()
we don't and can't know whether in-memory copy and the CPU regs match, so
we simply write FPU state to memory.
> I think that __save_init_fpu (called by unlazy_fpu) does that, but
> __thread_fpu_end calls __thread_clear_has_fpu, which, in turn, zaps
> fpu_owner_task, which will force an unnecessary xrstor. Or maybe not
> if we have further bugs.
Yes, see above. We do not really need unconditional __thread_fpu_end().
> Holy crap these functions are poorly named.
Yes... let me quote another email from me:
Perhaps you can also find a beter name for __save_init_fpu/etc ;) The
name clearly suggests that it does "save + init" while in fact it does
"save and may be destroy FPU state". At least for the callers, the fact
that "destroy" is actually "init" doesn't really matter.
But lets not rename it right now. This can conflict with the fixes we
need to do first.
So please do not rename it now ;) I had to switch to other tasks, but I hope
I will continue the FPU cleanups "soon".
> Also, what, if anything,
> guarantees that fpu_owner_task is set on entry to userspace?
Well, this probaly needs other cleanups. I am not sure __thread_set_has_fpu()
actually should set fpu_owner_task = current. But this is offtopic.
Again, I am not sure I understand your concern... but fpu_owner_task should
be set if __thread_has_fpu(). Otherwise we do not care.
But __thread_fpu_end() must _clear_ fpu_owner_task, this is what we do care.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Tue, Mar 24, 2015 at 09:01:44PM -0400, Rik van Riel wrote: > Indeed, __save_init_fpu (yeah, terrible name) will save > the in-register state to memory for you, so you can > inspect it. > > Is there any reason not to rename __save_init_fpu to > save_fpu_state, or just save_fpu? That whole place there needs more rubbing. So the way I see it, the "init" thing also says that the FPU is intact. __save_fpu() does *SAVE too but it misses the FNSAVE fallback for old machines. Why TF is that so? It also doesn't do FNCLEX and IMHO the clearing of exceptions is what the "init" aspect in the name tries to imply. I think we should simply merge those into a main one called save_fpu_state() (what Rik suggests) which does all that __save_init_fpu() does currently and a low-level helper __save_fpu() which does only the *SAVE saving of context work. Meh, not ecstatic about it either but it is the cleanup worth and maybe later more stuff will collapse during further cleaning... -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Tue, Mar 24, 2015 at 09:01:44PM -0400, Rik van Riel wrote: Indeed, __save_init_fpu (yeah, terrible name) will save the in-register state to memory for you, so you can inspect it. Is there any reason not to rename __save_init_fpu to save_fpu_state, or just save_fpu? That whole place there needs more rubbing. So the way I see it, the init thing also says that the FPU is intact. __save_fpu() does *SAVE too but it misses the FNSAVE fallback for old machines. Why TF is that so? It also doesn't do FNCLEX and IMHO the clearing of exceptions is what the init aspect in the name tries to imply. I think we should simply merge those into a main one called save_fpu_state() (what Rik suggests) which does all that __save_init_fpu() does currently and a low-level helper __save_fpu() which does only the *SAVE saving of context work. Meh, not ecstatic about it either but it is the cleanup worth and maybe later more stuff will collapse during further cleaning... -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
So far I do not understand this discussion ;) I didn't see the patches
and other emails...
On 03/24, Andy Lutomirski wrote:
On Tue, Mar 24, 2015 at 5:12 PM, Dave Hansen dave.han...@intel.com wrote:
So why are you unlazying it?
Oleg actually suggested it.
perhaps you are talking about math_error() ? It uses save_init_fpu()
in Linus's tree, but this is wrong. It was changed in tip/x86/fpu
to use unlazy_fpu() and save_init_fpu() was killed. Plus, just in
case, unlazy_fpu() was changed too and now it is
void unlazy_fpu(struct task_struct *tsk)
{
preempt_disable();
if (__thread_has_fpu(tsk)) {
if (use_eager_fpu()) {
__save_fpu(tsk);
} else {
__save_init_fpu(tsk);
__thread_fpu_end(tsk);
}
}
preempt_enable();
}
and in fact (I think) it needs another change later, __thread_fpu_end()
should depend on __save_init_fpu().
IIUC, the xstae for current can be in one of three logical states:
1. Live in CPU regs. The in-memory copy is garbage and the state is
in CPU regs.
2. Lazy. The in-memory copy and the CPU regs match. Writing to
either copy is illegal.
3. In memory only. Writing to the in-memory copy is safe.
IIUC, you want to read the xstate, do you're okay with #2 or #3. This
would be tsk_get_xsave_field_for_read in my terminology.
If you want to write the xstate, you'd need to be in state #3, which
would be tsk_get_xsave_field_for_write.
IIUC, unlazy_fpu just moves from from state 2 to 3.
I won't completely claim to understand what's going on with the FPU
code, but I think your analysis is a bit off.
unlazy_fpu() does __save_init_fpu() which (among other things) calls
xsave to dump the CPU registers to memory. That doesn't make any sense
to do if The in-memory copy and the CPU regs match.
IOW, unlazy_fpu() is called when the in-memory copy is garbage and takes
us to a state where we can look at the in-memory copy.
Not necessarily, but most probably I misunderstood... If __thread_has_fpu()
we don't and can't know whether in-memory copy and the CPU regs match, so
we simply write FPU state to memory.
I think that __save_init_fpu (called by unlazy_fpu) does that, but
__thread_fpu_end calls __thread_clear_has_fpu, which, in turn, zaps
fpu_owner_task, which will force an unnecessary xrstor. Or maybe not
if we have further bugs.
Yes, see above. We do not really need unconditional __thread_fpu_end().
Holy crap these functions are poorly named.
Yes... let me quote another email from me:
Perhaps you can also find a beter name for __save_init_fpu/etc ;) The
name clearly suggests that it does save + init while in fact it does
save and may be destroy FPU state. At least for the callers, the fact
that destroy is actually init doesn't really matter.
But lets not rename it right now. This can conflict with the fixes we
need to do first.
So please do not rename it now ;) I had to switch to other tasks, but I hope
I will continue the FPU cleanups soon.
Also, what, if anything,
guarantees that fpu_owner_task is set on entry to userspace?
Well, this probaly needs other cleanups. I am not sure __thread_set_has_fpu()
actually should set fpu_owner_task = current. But this is offtopic.
Again, I am not sure I understand your concern... but fpu_owner_task should
be set if __thread_has_fpu(). Otherwise we do not care.
But __thread_fpu_end() must _clear_ fpu_owner_task, this is what we do care.
Oleg.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/25/2015 05:45 AM, Oleg Nesterov wrote: So far I do not understand this discussion ;) I didn't see the patches and other emails... Hi Oleg, My patch set apparently didn't make it to LKML, but here are the two relevant ones. We're essentially replacing the MPX use of fpu_save_init(). CPUs with MPX should entirely have eager FPU mode on. But, the edges of the MPX code (do_bounds()) will call this to distinguish a plain #BR exception from a #BR caused by MPX. It may get called on CPUs without eager FPU mode on. http://git.kernel.org/cgit/linux/kernel/git/daveh/x86-mpx.git/commit/?h=mpx-v16id=92d3e7c1664f766142904904e27e126888adb8a7 http://git.kernel.org/cgit/linux/kernel/git/daveh/x86-mpx.git/commit/?h=mpx-v16id=18049953ae43a7ffa084a01613c1684bdf24dd2e All that the MPX code wants here is to read the in-memory copy of the MPX registers, or error out. So, for the purposes of this series: With the (so far unmerged to Linus's tree) changes to unlazy_fpu(), does tsk_get_xsave_field()'s use of unlazy_fpu() look correct? Should we also be renaming tsk_get_xsave_field() to something more appropriate? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/25, Borislav Petkov wrote: On Tue, Mar 24, 2015 at 09:01:44PM -0400, Rik van Riel wrote: Indeed, __save_init_fpu (yeah, terrible name) will save the in-register state to memory for you, so you can inspect it. Is there any reason not to rename __save_init_fpu to save_fpu_state, or just save_fpu? That whole place there needs more rubbing. So the way I see it, the init thing also says that the FPU is intact. Yes, this is my understanding too. And note that nobody actually wants this init part, so it actually means destroy. I agree we should rename it later (at least). Plus unlazy_fpu() looks confusing too. Nobody actually wants to unlazy, the callers want to save FPU state. So it could be named save_fpu_state() too ;) Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/24, Rik van Riel wrote: On 03/24/2015 08:18 PM, Andy Lutomirski wrote: Is there any reason not to rename __save_init_fpu to save_fpu_state, or just save_fpu? Please see another email. __save_init_fpu() saves the FPU state but it can also change the FPU registers. At least I think so ;) Otherwise, why switch_fpu_prepare() resets .last_cpu if it returns zero? And unlazy_fpu() should do the same. This all needs the cleanups, yes. Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
Hi Dave, On 03/25, Dave Hansen wrote: It may get called on CPUs without eager FPU mode on. http://git.kernel.org/cgit/linux/kernel/git/daveh/x86-mpx.git/commit/?h=mpx-v16id=92d3e7c1664f766142904904e27e126888adb8a7 http://git.kernel.org/cgit/linux/kernel/git/daveh/x86-mpx.git/commit/?h=mpx-v16id=18049953ae43a7ffa084a01613c1684bdf24dd2e All that the MPX code wants here is to read the in-memory copy of the MPX registers, or error out. Yes, iirc we alredy discussed these fixes ? I still think that the if (!xstate) check at the start of tsk_get_xsave_field() will look better, but this is cosmetic. So, for the purposes of this series: With the (so far unmerged to Linus's tree) changes to unlazy_fpu(), does tsk_get_xsave_field()'s use of unlazy_fpu() look correct? I think yes. But let me remind just in case that this depends on x86, fpu: unlazy_fpu: don't do __thread_fpu_end() if use_eager_fpu(). Should we also be renaming tsk_get_xsave_field() to something more appropriate? Oh, don't ask me ;) To me it looks fine. Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/24/2015 08:18 PM, Andy Lutomirski wrote: > On Tue, Mar 24, 2015 at 5:12 PM, Dave Hansen wrote: >> I won't completely claim to understand what's going on with the FPU >> code, but I think your analysis is a bit off. >> >> unlazy_fpu() does __save_init_fpu() which (among other things) calls >> xsave to dump the CPU registers to memory. That doesn't make any sense >> to do if "The in-memory copy and the CPU regs match." >> >> IOW, unlazy_fpu() is called when the in-memory copy is garbage and takes >> us to a state where we can look at the in-memory copy. > > I think that __save_init_fpu (called by unlazy_fpu) does that, but > __thread_fpu_end calls __thread_clear_has_fpu, which, in turn, zaps > fpu_owner_task, which will force an unnecessary xrstor. Or maybe not > if we have further bugs. Indeed, __save_init_fpu (yeah, terrible name) will save the in-register state to memory for you, so you can inspect it. Is there any reason not to rename __save_init_fpu to save_fpu_state, or just save_fpu? -- All rights reversed -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
[add Borislav] I swear it would actually be an improvement if we just randomized the function names. fpu_817, fpu_717, etc. At least no one would think they understand them... On Tue, Mar 24, 2015 at 5:18 PM, Andy Lutomirski wrote: > On Tue, Mar 24, 2015 at 5:12 PM, Dave Hansen wrote: >> On 03/24/2015 04:52 PM, Andy Lutomirski wrote: >>> On Tue, Mar 24, 2015 at 4:42 PM, Dave Hansen wrote: On 03/24/2015 03:28 PM, Andy Lutomirski wrote: > Your function appears to be getting it for write (I assume that's what > the unlazy_fpu is for), so I'd rather have it called > tsk_get_xsave_field_for_write or something like that. It should be entirely read-only. For MPX (the only user of get_xsave_addr() iirc), we are only worried about getting the status codes (and addresses) out of the bndstatus register and making sure that the kernel-recorded bounds directory address matches the bndcfgu (configuration) register. We don't ever write to the registers. >>> >>> So why are you unlazying it? >> >> Oleg actually suggested it. >> >>> IIUC, the xstae for current can be in one of three logical states: >>> >>> 1. Live in CPU regs. The in-memory copy is garbage and the state is >>> in CPU regs. >>> 2. Lazy. The in-memory copy and the CPU regs match. Writing to >>> either copy is illegal. >>> 3. In memory only. Writing to the in-memory copy is safe. >>> >>> IIUC, you want to read the xstate, do you're okay with #2 or #3. This >>> would be tsk_get_xsave_field_for_read in my terminology. >>> >>> If you want to write the xstate, you'd need to be in state #3, which >>> would be tsk_get_xsave_field_for_write. >>> >>> IIUC, unlazy_fpu just moves from from state 2 to 3. >> >> I won't completely claim to understand what's going on with the FPU >> code, but I think your analysis is a bit off. >> >> unlazy_fpu() does __save_init_fpu() which (among other things) calls >> xsave to dump the CPU registers to memory. That doesn't make any sense >> to do if "The in-memory copy and the CPU regs match." >> >> IOW, unlazy_fpu() is called when the in-memory copy is garbage and takes >> us to a state where we can look at the in-memory copy. > > I think that __save_init_fpu (called by unlazy_fpu) does that, but > __thread_fpu_end calls __thread_clear_has_fpu, which, in turn, zaps > fpu_owner_task, which will force an unnecessary xrstor. Or maybe not > if we have further bugs. > > Holy crap these functions are poorly named. Also, what, if anything, > guarantees that fpu_owner_task is set on entry to userspace? Do we > even need it to be set? Oleg, help? > > --Andy > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Tue, Mar 24, 2015 at 5:12 PM, Dave Hansen wrote: > On 03/24/2015 04:52 PM, Andy Lutomirski wrote: >> On Tue, Mar 24, 2015 at 4:42 PM, Dave Hansen wrote: >>> On 03/24/2015 03:28 PM, Andy Lutomirski wrote: Your function appears to be getting it for write (I assume that's what the unlazy_fpu is for), so I'd rather have it called tsk_get_xsave_field_for_write or something like that. >>> >>> It should be entirely read-only. >>> >>> For MPX (the only user of get_xsave_addr() iirc), we are only worried >>> about getting the status codes (and addresses) out of the bndstatus >>> register and making sure that the kernel-recorded bounds directory >>> address matches the bndcfgu (configuration) register. >>> >>> We don't ever write to the registers. >> >> So why are you unlazying it? > > Oleg actually suggested it. > >> IIUC, the xstae for current can be in one of three logical states: >> >> 1. Live in CPU regs. The in-memory copy is garbage and the state is >> in CPU regs. >> 2. Lazy. The in-memory copy and the CPU regs match. Writing to >> either copy is illegal. >> 3. In memory only. Writing to the in-memory copy is safe. >> >> IIUC, you want to read the xstate, do you're okay with #2 or #3. This >> would be tsk_get_xsave_field_for_read in my terminology. >> >> If you want to write the xstate, you'd need to be in state #3, which >> would be tsk_get_xsave_field_for_write. >> >> IIUC, unlazy_fpu just moves from from state 2 to 3. > > I won't completely claim to understand what's going on with the FPU > code, but I think your analysis is a bit off. > > unlazy_fpu() does __save_init_fpu() which (among other things) calls > xsave to dump the CPU registers to memory. That doesn't make any sense > to do if "The in-memory copy and the CPU regs match." > > IOW, unlazy_fpu() is called when the in-memory copy is garbage and takes > us to a state where we can look at the in-memory copy. I think that __save_init_fpu (called by unlazy_fpu) does that, but __thread_fpu_end calls __thread_clear_has_fpu, which, in turn, zaps fpu_owner_task, which will force an unnecessary xrstor. Or maybe not if we have further bugs. Holy crap these functions are poorly named. Also, what, if anything, guarantees that fpu_owner_task is set on entry to userspace? Do we even need it to be set? Oleg, help? --Andy -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/24/2015 04:52 PM, Andy Lutomirski wrote: > On Tue, Mar 24, 2015 at 4:42 PM, Dave Hansen wrote: >> On 03/24/2015 03:28 PM, Andy Lutomirski wrote: >>> Your function appears to be getting it for write (I assume that's what >>> the unlazy_fpu is for), so I'd rather have it called >>> tsk_get_xsave_field_for_write or something like that. >> >> It should be entirely read-only. >> >> For MPX (the only user of get_xsave_addr() iirc), we are only worried >> about getting the status codes (and addresses) out of the bndstatus >> register and making sure that the kernel-recorded bounds directory >> address matches the bndcfgu (configuration) register. >> >> We don't ever write to the registers. > > So why are you unlazying it? Oleg actually suggested it. > IIUC, the xstae for current can be in one of three logical states: > > 1. Live in CPU regs. The in-memory copy is garbage and the state is > in CPU regs. > 2. Lazy. The in-memory copy and the CPU regs match. Writing to > either copy is illegal. > 3. In memory only. Writing to the in-memory copy is safe. > > IIUC, you want to read the xstate, do you're okay with #2 or #3. This > would be tsk_get_xsave_field_for_read in my terminology. > > If you want to write the xstate, you'd need to be in state #3, which > would be tsk_get_xsave_field_for_write. > > IIUC, unlazy_fpu just moves from from state 2 to 3. I won't completely claim to understand what's going on with the FPU code, but I think your analysis is a bit off. unlazy_fpu() does __save_init_fpu() which (among other things) calls xsave to dump the CPU registers to memory. That doesn't make any sense to do if "The in-memory copy and the CPU regs match." IOW, unlazy_fpu() is called when the in-memory copy is garbage and takes us to a state where we can look at the in-memory copy. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Tue, Mar 24, 2015 at 4:42 PM, Dave Hansen wrote: > On 03/24/2015 03:28 PM, Andy Lutomirski wrote: >> Your function appears to be getting it for write (I assume that's what >> the unlazy_fpu is for), so I'd rather have it called >> tsk_get_xsave_field_for_write or something like that. > > It should be entirely read-only. > > For MPX (the only user of get_xsave_addr() iirc), we are only worried > about getting the status codes (and addresses) out of the bndstatus > register and making sure that the kernel-recorded bounds directory > address matches the bndcfgu (configuration) register. > > We don't ever write to the registers. So why are you unlazying it? IIUC, the xstae for current can be in one of three logical states: 1. Live in CPU regs. The in-memory copy is garbage and the state is in CPU regs. 2. Lazy. The in-memory copy and the CPU regs match. Writing to either copy is illegal. 3. In memory only. Writing to the in-memory copy is safe. IIUC, you want to read the xstate, do you're okay with #2 or #3. This would be tsk_get_xsave_field_for_read in my terminology. If you want to write the xstate, you'd need to be in state #3, which would be tsk_get_xsave_field_for_write. IIUC, unlazy_fpu just moves from from state 2 to 3. I could be totally wrong for any number of reasons, though. --Andy -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/24/2015 03:28 PM, Andy Lutomirski wrote: > Your function appears to be getting it for write (I assume that's what > the unlazy_fpu is for), so I'd rather have it called > tsk_get_xsave_field_for_write or something like that. It should be entirely read-only. For MPX (the only user of get_xsave_addr() iirc), we are only worried about getting the status codes (and addresses) out of the bndstatus register and making sure that the kernel-recorded bounds directory address matches the bndcfgu (configuration) register. We don't ever write to the registers. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Tue, Mar 24, 2015 at 3:20 PM, Dave Hansen wrote:
> From: Dave Hansen
>
> The MPX code appears to be saving off the FPU in an unsafe
> way. It does not disable preemption or ensure that the
> FPU state has been allocated.
>
> This patch introduces a new helper which will do both of
> those things internally to a helper.
>
> Signed-off-by: Dave Hansen
> Cc: Rik van Riel
> Cc: Suresh Siddha
> Cc: Andy Lutomirski
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: "H. Peter Anvin"
> Cc: Fenghua Yu
> Cc: the arch/x86 maintainers
> ---
> arch/x86/include/asm/xsave.h | 1 +
> arch/x86/kernel/xsave.c | 32
> 2 files changed, 33 insertions(+)
>
> diff --git a/arch/x86/include/asm/xsave.h b/arch/x86/include/asm/xsave.h
> index c9a6d68..86b58fb 100644
> --- a/arch/x86/include/asm/xsave.h
> +++ b/arch/x86/include/asm/xsave.h
> @@ -252,6 +252,7 @@ static inline int xrestore_user(struct xsave_struct
> __user *buf, u64 mask)
> }
>
> void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
> +void *tsk_get_xsave_field(struct task_struct *tsk, int xstate_field);
> void setup_xstate_comp(void);
>
> #endif
> diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
> index 34f66e5..9919e7e 100644
> --- a/arch/x86/kernel/xsave.c
> +++ b/arch/x86/kernel/xsave.c
> @@ -740,3 +740,35 @@ void *get_xsave_addr(struct xsave_struct *xsave, int
> xstate)
> return (void *)xsave + xstate_comp_offsets[feature];
> }
> EXPORT_SYMBOL_GPL(get_xsave_addr);
> +
> +/*
> + * This wraps up the common operations that need to occur when retrieving
> + * data from an xsave struct. It first ensures that the task was actually
> + * using the FPU and retrieves the data in to a buffer. It then calculates
> + * the offset of the requested field in the buffer.
> + *
> + * This function is safe to call whether the FPU is in use or not.
> + *
> + * Inputs:
> + * tsk: the task from which we are fetching xsave state
> + * xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
> + * etc.)
> + * Output:
> + * address of the state in the xsave area.
> + */
> +void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
> +{
> + union thread_xstate *xstate;
> +
> + unlazy_fpu(tsk);
> + xstate = tsk->thread.fpu.state;
> + /*
> +* This might be unallocated if the FPU
> +* was never in use.
> +*/
> + if (!xstate)
> + return NULL;
> +
> + return get_xsave_addr(>xsave, xsave_field);
> +}
> +EXPORT_SYMBOL_GPL(tsk_get_xsave_field);
> --
> 1.9.1
>
I have one objection to get_xsave_addr and tsk_get_xsave_field: I
think that get_xsave_addr should be called either
get_xsave_addr_for_read or get_xsave_addr_for_write, depending on
which of those it does.
Your function appears to be getting it for write (I assume that's what
the unlazy_fpu is for), so I'd rather have it called
tsk_get_xsave_field_for_write or something like that.
--Andy
--
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/24/2015 03:28 PM, Andy Lutomirski wrote: Your function appears to be getting it for write (I assume that's what the unlazy_fpu is for), so I'd rather have it called tsk_get_xsave_field_for_write or something like that. It should be entirely read-only. For MPX (the only user of get_xsave_addr() iirc), we are only worried about getting the status codes (and addresses) out of the bndstatus register and making sure that the kernel-recorded bounds directory address matches the bndcfgu (configuration) register. We don't ever write to the registers. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Tue, Mar 24, 2015 at 4:42 PM, Dave Hansen dave.han...@intel.com wrote: On 03/24/2015 03:28 PM, Andy Lutomirski wrote: Your function appears to be getting it for write (I assume that's what the unlazy_fpu is for), so I'd rather have it called tsk_get_xsave_field_for_write or something like that. It should be entirely read-only. For MPX (the only user of get_xsave_addr() iirc), we are only worried about getting the status codes (and addresses) out of the bndstatus register and making sure that the kernel-recorded bounds directory address matches the bndcfgu (configuration) register. We don't ever write to the registers. So why are you unlazying it? IIUC, the xstae for current can be in one of three logical states: 1. Live in CPU regs. The in-memory copy is garbage and the state is in CPU regs. 2. Lazy. The in-memory copy and the CPU regs match. Writing to either copy is illegal. 3. In memory only. Writing to the in-memory copy is safe. IIUC, you want to read the xstate, do you're okay with #2 or #3. This would be tsk_get_xsave_field_for_read in my terminology. If you want to write the xstate, you'd need to be in state #3, which would be tsk_get_xsave_field_for_write. IIUC, unlazy_fpu just moves from from state 2 to 3. I could be totally wrong for any number of reasons, though. --Andy -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Tue, Mar 24, 2015 at 3:20 PM, Dave Hansen dave.han...@intel.com wrote:
From: Dave Hansen dave.han...@linux.intel.com
The MPX code appears to be saving off the FPU in an unsafe
way. It does not disable preemption or ensure that the
FPU state has been allocated.
This patch introduces a new helper which will do both of
those things internally to a helper.
Signed-off-by: Dave Hansen dave.han...@linux.intel.com
Cc: Rik van Riel r...@redhat.com
Cc: Suresh Siddha sbsid...@gmail.com
Cc: Andy Lutomirski l...@amacapital.net
Cc: Thomas Gleixner t...@linutronix.de
Cc: Ingo Molnar mi...@redhat.com
Cc: H. Peter Anvin h...@zytor.com
Cc: Fenghua Yu fenghua...@intel.com
Cc: the arch/x86 maintainers x...@kernel.org
---
arch/x86/include/asm/xsave.h | 1 +
arch/x86/kernel/xsave.c | 32
2 files changed, 33 insertions(+)
diff --git a/arch/x86/include/asm/xsave.h b/arch/x86/include/asm/xsave.h
index c9a6d68..86b58fb 100644
--- a/arch/x86/include/asm/xsave.h
+++ b/arch/x86/include/asm/xsave.h
@@ -252,6 +252,7 @@ static inline int xrestore_user(struct xsave_struct
__user *buf, u64 mask)
}
void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
+void *tsk_get_xsave_field(struct task_struct *tsk, int xstate_field);
void setup_xstate_comp(void);
#endif
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 34f66e5..9919e7e 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -740,3 +740,35 @@ void *get_xsave_addr(struct xsave_struct *xsave, int
xstate)
return (void *)xsave + xstate_comp_offsets[feature];
}
EXPORT_SYMBOL_GPL(get_xsave_addr);
+
+/*
+ * This wraps up the common operations that need to occur when retrieving
+ * data from an xsave struct. It first ensures that the task was actually
+ * using the FPU and retrieves the data in to a buffer. It then calculates
+ * the offset of the requested field in the buffer.
+ *
+ * This function is safe to call whether the FPU is in use or not.
+ *
+ * Inputs:
+ * tsk: the task from which we are fetching xsave state
+ * xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
+ * etc.)
+ * Output:
+ * address of the state in the xsave area.
+ */
+void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
+{
+ union thread_xstate *xstate;
+
+ unlazy_fpu(tsk);
+ xstate = tsk-thread.fpu.state;
+ /*
+* This might be unallocated if the FPU
+* was never in use.
+*/
+ if (!xstate)
+ return NULL;
+
+ return get_xsave_addr(xstate-xsave, xsave_field);
+}
+EXPORT_SYMBOL_GPL(tsk_get_xsave_field);
--
1.9.1
I have one objection to get_xsave_addr and tsk_get_xsave_field: I
think that get_xsave_addr should be called either
get_xsave_addr_for_read or get_xsave_addr_for_write, depending on
which of those it does.
Your function appears to be getting it for write (I assume that's what
the unlazy_fpu is for), so I'd rather have it called
tsk_get_xsave_field_for_write or something like that.
--Andy
--
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On Tue, Mar 24, 2015 at 5:12 PM, Dave Hansen dave.han...@intel.com wrote: On 03/24/2015 04:52 PM, Andy Lutomirski wrote: On Tue, Mar 24, 2015 at 4:42 PM, Dave Hansen dave.han...@intel.com wrote: On 03/24/2015 03:28 PM, Andy Lutomirski wrote: Your function appears to be getting it for write (I assume that's what the unlazy_fpu is for), so I'd rather have it called tsk_get_xsave_field_for_write or something like that. It should be entirely read-only. For MPX (the only user of get_xsave_addr() iirc), we are only worried about getting the status codes (and addresses) out of the bndstatus register and making sure that the kernel-recorded bounds directory address matches the bndcfgu (configuration) register. We don't ever write to the registers. So why are you unlazying it? Oleg actually suggested it. IIUC, the xstae for current can be in one of three logical states: 1. Live in CPU regs. The in-memory copy is garbage and the state is in CPU regs. 2. Lazy. The in-memory copy and the CPU regs match. Writing to either copy is illegal. 3. In memory only. Writing to the in-memory copy is safe. IIUC, you want to read the xstate, do you're okay with #2 or #3. This would be tsk_get_xsave_field_for_read in my terminology. If you want to write the xstate, you'd need to be in state #3, which would be tsk_get_xsave_field_for_write. IIUC, unlazy_fpu just moves from from state 2 to 3. I won't completely claim to understand what's going on with the FPU code, but I think your analysis is a bit off. unlazy_fpu() does __save_init_fpu() which (among other things) calls xsave to dump the CPU registers to memory. That doesn't make any sense to do if The in-memory copy and the CPU regs match. IOW, unlazy_fpu() is called when the in-memory copy is garbage and takes us to a state where we can look at the in-memory copy. I think that __save_init_fpu (called by unlazy_fpu) does that, but __thread_fpu_end calls __thread_clear_has_fpu, which, in turn, zaps fpu_owner_task, which will force an unnecessary xrstor. Or maybe not if we have further bugs. Holy crap these functions are poorly named. Also, what, if anything, guarantees that fpu_owner_task is set on entry to userspace? Do we even need it to be set? Oleg, help? --Andy -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/24/2015 08:18 PM, Andy Lutomirski wrote: On Tue, Mar 24, 2015 at 5:12 PM, Dave Hansen dave.han...@intel.com wrote: I won't completely claim to understand what's going on with the FPU code, but I think your analysis is a bit off. unlazy_fpu() does __save_init_fpu() which (among other things) calls xsave to dump the CPU registers to memory. That doesn't make any sense to do if The in-memory copy and the CPU regs match. IOW, unlazy_fpu() is called when the in-memory copy is garbage and takes us to a state where we can look at the in-memory copy. I think that __save_init_fpu (called by unlazy_fpu) does that, but __thread_fpu_end calls __thread_clear_has_fpu, which, in turn, zaps fpu_owner_task, which will force an unnecessary xrstor. Or maybe not if we have further bugs. Indeed, __save_init_fpu (yeah, terrible name) will save the in-register state to memory for you, so you can inspect it. Is there any reason not to rename __save_init_fpu to save_fpu_state, or just save_fpu? -- All rights reversed -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
[add Borislav] I swear it would actually be an improvement if we just randomized the function names. fpu_817, fpu_717, etc. At least no one would think they understand them... On Tue, Mar 24, 2015 at 5:18 PM, Andy Lutomirski l...@amacapital.net wrote: On Tue, Mar 24, 2015 at 5:12 PM, Dave Hansen dave.han...@intel.com wrote: On 03/24/2015 04:52 PM, Andy Lutomirski wrote: On Tue, Mar 24, 2015 at 4:42 PM, Dave Hansen dave.han...@intel.com wrote: On 03/24/2015 03:28 PM, Andy Lutomirski wrote: Your function appears to be getting it for write (I assume that's what the unlazy_fpu is for), so I'd rather have it called tsk_get_xsave_field_for_write or something like that. It should be entirely read-only. For MPX (the only user of get_xsave_addr() iirc), we are only worried about getting the status codes (and addresses) out of the bndstatus register and making sure that the kernel-recorded bounds directory address matches the bndcfgu (configuration) register. We don't ever write to the registers. So why are you unlazying it? Oleg actually suggested it. IIUC, the xstae for current can be in one of three logical states: 1. Live in CPU regs. The in-memory copy is garbage and the state is in CPU regs. 2. Lazy. The in-memory copy and the CPU regs match. Writing to either copy is illegal. 3. In memory only. Writing to the in-memory copy is safe. IIUC, you want to read the xstate, do you're okay with #2 or #3. This would be tsk_get_xsave_field_for_read in my terminology. If you want to write the xstate, you'd need to be in state #3, which would be tsk_get_xsave_field_for_write. IIUC, unlazy_fpu just moves from from state 2 to 3. I won't completely claim to understand what's going on with the FPU code, but I think your analysis is a bit off. unlazy_fpu() does __save_init_fpu() which (among other things) calls xsave to dump the CPU registers to memory. That doesn't make any sense to do if The in-memory copy and the CPU regs match. IOW, unlazy_fpu() is called when the in-memory copy is garbage and takes us to a state where we can look at the in-memory copy. I think that __save_init_fpu (called by unlazy_fpu) does that, but __thread_fpu_end calls __thread_clear_has_fpu, which, in turn, zaps fpu_owner_task, which will force an unnecessary xrstor. Or maybe not if we have further bugs. Holy crap these functions are poorly named. Also, what, if anything, guarantees that fpu_owner_task is set on entry to userspace? Do we even need it to be set? Oleg, help? --Andy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer
On 03/24/2015 04:52 PM, Andy Lutomirski wrote: On Tue, Mar 24, 2015 at 4:42 PM, Dave Hansen dave.han...@intel.com wrote: On 03/24/2015 03:28 PM, Andy Lutomirski wrote: Your function appears to be getting it for write (I assume that's what the unlazy_fpu is for), so I'd rather have it called tsk_get_xsave_field_for_write or something like that. It should be entirely read-only. For MPX (the only user of get_xsave_addr() iirc), we are only worried about getting the status codes (and addresses) out of the bndstatus register and making sure that the kernel-recorded bounds directory address matches the bndcfgu (configuration) register. We don't ever write to the registers. So why are you unlazying it? Oleg actually suggested it. IIUC, the xstae for current can be in one of three logical states: 1. Live in CPU regs. The in-memory copy is garbage and the state is in CPU regs. 2. Lazy. The in-memory copy and the CPU regs match. Writing to either copy is illegal. 3. In memory only. Writing to the in-memory copy is safe. IIUC, you want to read the xstate, do you're okay with #2 or #3. This would be tsk_get_xsave_field_for_read in my terminology. If you want to write the xstate, you'd need to be in state #3, which would be tsk_get_xsave_field_for_write. IIUC, unlazy_fpu just moves from from state 2 to 3. I won't completely claim to understand what's going on with the FPU code, but I think your analysis is a bit off. unlazy_fpu() does __save_init_fpu() which (among other things) calls xsave to dump the CPU registers to memory. That doesn't make any sense to do if The in-memory copy and the CPU regs match. IOW, unlazy_fpu() is called when the in-memory copy is garbage and takes us to a state where we can look at the in-memory copy. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
