Re: [PATCH 03/19] x86, mpx: use new tsk_get_xsave_addr()

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, 8 May 2015, Dave Hansen wrote:

> Subject: x86, mpx: use new tsk_get_xsave_addr()

-NOSUCHFUNCTION

And please let the sentence start with an upper case letter.

x86, mpx: Use ...

>  #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);

So you still keep *tsk and remove it in the next patch. I guess either
ordering has its value, so no problem to do it this way.

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

But that makes no sense. Introduce a new instance just to remove it
one patch later. So maybe cleaning up the *tsk thing first might be
the better approach, but I let you decide.

>  #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>
> @@ -372,7 +373,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;
> @@ -393,12 +393,11 @@ dotraplinkage void do_bounds(struct pt_r
>  
>  	/*
>  	 * We need to look at BNDSTATUS to resolve this exception.
> -	 * It is not directly accessible, though, so we need to
> -	 * do an xsave and then pull it out of the xsave buffer.
> +	 * A NULL here might mean that it is in its 'init state',
> +	 * which is all zeros which indicates MPX was not
> +	 * responsible for the exception.
>  	 */
> -	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);

Hmm, I did not notice when I looked at the patch which introduced
get_xsave_field(). But with the removed get_xsave_addr() I really had
to look twice.

get_xsave_field() suggest that you get the field content. So wouldn't
it be better to name it get_xsave_field_ptr() or such?

>  	if (!bndcsr)
>  		goto exit_trap;
>  
> @@ -409,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-05-08 11:46:11.388599581 -0700
> +++ b/arch/x86/mm/mpx.c	2015-05-08 11:46:11.393599807 -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)

Looking at the implementation makes me wonder about the extra *tsk
argument some more ....

>  {
>  	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 */

  urrent?

> +	bndregs = get_xsave_field(XSTATE_BNDREGS);
>  	if (!bndregs) {
>  		err = -EINVAL;
>  		goto err_out;

Still no user of *tsk ....

> @@ -358,8 +358,7 @@ static __user void *task_get_bounds_dir(
> -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)) {

Same problem actually with do_mpx_bt_fault().

Thanks,

	tglx