Messages in this thread | | | From | "Yu, Fenghua" <> | Subject | RE: [PATCH 02/17] x86, mpx: use new tsk_get_xsave_addr() | Date | Wed, 22 Apr 2015 19:18:23 +0000 |
| |
> From: Dave Hansen [mailto:dave@sr71.net] > Sent: Wednesday, April 22, 2015 11:28 AM > To: linux-kernel@vger.kernel.org > Cc: x86@kernel.org; tglx@linutronix.de; Dave Hansen; > dave.hansen@linux.intel.com; oleg@redhat.com; bp@alien8.de; > riel@redhat.com; sbsiddha@gmail.com; luto@amacapital.net; > mingo@redhat.com; hpa@zytor.com; Yu, Fenghua > Subject: [PATCH 02/17] x86, mpx: use new tsk_get_xsave_addr() > > > From: Dave Hansen <dave.hansen@linux.intel.com> > > The MPX registers (bndcsr/bndcfgu/bndstatus) are not directly accessible via > normal instructions. They essentially act as if they were floating point > registers and are saved/restored along with those registers. > > There are two main paths in the MPX code where we care about the > contents of these registers: > 1. #BR (bounds) faults > 2. the prctl() code where we are setting MPX up > > Both of those paths _might_ be called without the FPU having been used. > That means that 'tsk->thread.fpu.state' might never be allocated. > > Also, fpu_save_init() is not preempt-safe. It was a bug to call it without > disabling preemption. The new > tsk_get_xsave_addr() calls unlazy_fpu() instead and properly disables > preemption. > > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com> > Cc: Oleg Nesterov <oleg@redhat.com> > Cc: bp@alien8.de > Cc: Rik van Riel <riel@redhat.com> > Cc: Suresh Siddha <sbsiddha@gmail.com> > Cc: Andy Lutomirski <luto@amacapital.net> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Fenghua Yu <fenghua.yu@intel.com> > Cc: the arch/x86 maintainers <x86@kernel.org> > Cc: linux-kernel <linux-kernel@vger.kernel.org> > --- > > b/arch/x86/include/asm/mpx.h | 8 ++++---- > b/arch/x86/kernel/traps.c | 10 ++++------ > b/arch/x86/mm/mpx.c | 23 +++++++++++------------ > 3 files changed, 19 insertions(+), 22 deletions(-) > > diff -puN arch/x86/include/asm/mpx.h~use-new-tsk_get_xsave_addr > arch/x86/include/asm/mpx.h > --- a/arch/x86/include/asm/mpx.h~use-new-tsk_get_xsave_addr 2015- > 04-22 11:16:18.188818959 -0700 > +++ b/arch/x86/include/asm/mpx.h 2015-04-22 11:16:18.195819274 -0700 > @@ -60,8 +60,8 @@ > > #ifdef CONFIG_X86_INTEL_MPX > siginfo_t *mpx_generate_siginfo(struct pt_regs *regs, > - struct xsave_struct *xsave_buf); > -int mpx_handle_bd_fault(struct xsave_struct *xsave_buf); > + struct task_struct *tsk); > +int mpx_handle_bd_fault(struct task_struct *tsk); > static inline int kernel_managing_mpx_tables(struct mm_struct *mm) { > return (mm->bd_addr != MPX_INVALID_BOUNDS_DIR); @@ -78,11 > +78,11 @@ void mpx_notify_unmap(struct mm_struct * > unsigned long start, unsigned long end); #else static > inline siginfo_t *mpx_generate_siginfo(struct pt_regs *regs, > - struct xsave_struct *xsave_buf) > + struct task_struct *tsk) > { > return NULL; > } > -static inline int mpx_handle_bd_fault(struct xsave_struct *xsave_buf) > +static inline int mpx_handle_bd_fault(struct task_struct *tsk) > { > return -EINVAL; > } > diff -puN arch/x86/kernel/traps.c~use-new-tsk_get_xsave_addr > arch/x86/kernel/traps.c > --- a/arch/x86/kernel/traps.c~use-new-tsk_get_xsave_addr 2015-04-22 > 11:16:18.190819049 -0700 > +++ b/arch/x86/kernel/traps.c 2015-04-22 11:16:18.195819274 -0700 > @@ -61,6 +61,7 @@ > #include <asm/mach_traps.h> > #include <asm/alternative.h> > #include <asm/mpx.h> > +#include <asm/xsave.h> > > #ifdef CONFIG_X86_64 > #include <asm/x86_init.h> > @@ -373,7 +374,6 @@ dotraplinkage void do_double_fault(struc > dotraplinkage void do_bounds(struct pt_regs *regs, long error_code) { > struct task_struct *tsk = current; > - struct xsave_struct *xsave_buf; > enum ctx_state prev_state; > struct bndcsr *bndcsr; > siginfo_t *info; > @@ -397,9 +397,7 @@ dotraplinkage void do_bounds(struct pt_r > * It is not directly accessible, though, so we need to > * do an xsave and then pull it out of the xsave buffer. > */ > - fpu_save_init(&tsk->thread.fpu); > - xsave_buf = &(tsk->thread.fpu.state->xsave); > - bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR); > + bndcsr = get_xsave_field(XSTATE_BNDCSR); > if (!bndcsr) > goto exit_trap; > > @@ -410,11 +408,11 @@ dotraplinkage void do_bounds(struct pt_r > */ > switch (bndcsr->bndstatus & MPX_BNDSTA_ERROR_CODE) { > case 2: /* Bound directory has invalid entry. */ > - if (mpx_handle_bd_fault(xsave_buf)) > + if (mpx_handle_bd_fault(tsk)) > goto exit_trap; > break; /* Success, it was handled */ > case 1: /* Bound violation. */ > - info = mpx_generate_siginfo(regs, xsave_buf); > + info = mpx_generate_siginfo(regs, tsk); > if (IS_ERR(info)) { > /* > * We failed to decode the MPX instruction. Act as if > diff -puN arch/x86/mm/mpx.c~use-new-tsk_get_xsave_addr > arch/x86/mm/mpx.c > --- a/arch/x86/mm/mpx.c~use-new-tsk_get_xsave_addr 2015-04-22 > 11:16:18.192819139 -0700 > +++ b/arch/x86/mm/mpx.c 2015-04-22 11:16:18.196819320 -0700 > @@ -273,7 +273,7 @@ bad_opcode: > * The caller is expected to kfree() the returned siginfo_t. > */ > siginfo_t *mpx_generate_siginfo(struct pt_regs *regs, > - struct xsave_struct *xsave_buf) > + struct task_struct *tsk) > {
Do you need the parameter "tsk"? It's not used in this function. Seems this function only needs to take one parameter "regs". Other parameters are unnecessary.
> struct bndreg *bndregs, *bndreg; > siginfo_t *info = NULL; > @@ -295,8 +295,8 @@ siginfo_t *mpx_generate_siginfo(struct p > err = -EINVAL; > goto err_out; > } > - /* get the bndregs _area_ of the xsave structure */ > - bndregs = get_xsave_addr(xsave_buf, XSTATE_BNDREGS); > + /* get bndregs field from urrent task's xsave area */ > + bndregs = get_xsave_field(XSTATE_BNDREGS); > if (!bndregs) { > err = -EINVAL; > goto err_out; > @@ -358,8 +358,7 @@ static __user void *task_get_bounds_dir( > * The bounds directory pointer is stored in a register > * only accessible if we first do an xsave. > */ > - fpu_save_init(&tsk->thread.fpu); > - bndcsr = get_xsave_addr(&tsk->thread.fpu.state->xsave, > XSTATE_BNDCSR); > + bndcsr = get_xsave_field(XSTATE_BNDCSR); > if (!bndcsr) > return MPX_INVALID_BOUNDS_DIR; > > @@ -390,9 +389,9 @@ int mpx_enable_management(struct task_st > * directory into XSAVE/XRSTOR Save Area and enable MPX through > * XRSTOR instruction. > * > - * fpu_xsave() is expected to be very expensive. Storing the bounds > - * directory here means that we do not have to do xsave in the > unmap > - * path; we can just use mm->bd_addr instead. > + * xsaves are expected to be very expensive. Storing the bounds > + * directory here means that we do not have to do xsave in the > + * unmap path; we can just use mm->bd_addr instead. > */ > bd_base = task_get_bounds_dir(tsk); > down_write(&mm->mmap_sem); > @@ -498,12 +497,12 @@ out_unmap: > * bound table is 16KB. With 64-bit mode, the size of BD is 2GB, > * and the size of each bound table is 4MB. > */ > -static int do_mpx_bt_fault(struct xsave_struct *xsave_buf) > +static int do_mpx_bt_fault(struct task_struct *tsk)
Do you need the parameter "tsk"? It's not used in this function. Seems this function doesn't need to take any parameter.
> { > unsigned long bd_entry, bd_base; > struct bndcsr *bndcsr; > > - bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR); > + bndcsr = get_xsave_field(XSTATE_BNDCSR); > if (!bndcsr) > return -EINVAL; > /* > @@ -526,7 +525,7 @@ static int do_mpx_bt_fault(struct xsave_ > return allocate_bt((long __user *)bd_entry); } > > -int mpx_handle_bd_fault(struct xsave_struct *xsave_buf) > +int mpx_handle_bd_fault(struct task_struct *tsk) > { > /* > * Userspace never asked us to manage the bounds tables, @@ - > 535,7 +534,7 @@ int mpx_handle_bd_fault(struct xsave_str > if (!kernel_managing_mpx_tables(current->mm)) > return -EINVAL; > > - if (do_mpx_bt_fault(xsave_buf)) { > + if (do_mpx_bt_fault(tsk)) { > force_sig(SIGSEGV, current); > /* > * The force_sig() is essentially "handling" this _
| |