Re: [PATCH] MIPS: Fix MSA ld unaligned failure cases

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 3795 bytes --]

On Wed, Feb 03, 2016 at 03:35:49AM +0000, Paul Burton wrote:
> Copying the content of an MSA vector from user memory may involve TLB
> faults & mapping in pages. This will fail when preemption is disabled
> due to an inability to acquire mmap_sem from do_page_fault, which meant
> such vector loads to unmapped pages would always fail to be emulated.
> Fix this by disabling preemption later only around the updating of
> vector register state.
> 
> This change does however introduce a race between performing the load
> into thread context & the thread being preempted, saving its current
> live context & clobbering the loaded value. This should be a rare
> occureence, so optimise for the fast path by simply repeating the load if
> we are preempted.
> 
> Additionally if the copy failed then the failure path was taken with
> preemption left disabled, leading to the kernel typically encountering
> further issues around sleeping whilst atomic. The change to where
> preemption is disabled avoids this issue.
> 
> Fixes: e4aa1f153add "MIPS: MSA unaligned memory access support"
> Reported-by: James Hogan <james.hogan@imgtec.com>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: stable <stable@vger.kernel.org> # v4.3

Reviewed-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

> 
> ---
> 
>  arch/mips/kernel/unaligned.c | 51 ++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/mips/kernel/unaligned.c b/arch/mips/kernel/unaligned.c
> index 490cea5..5c62065 100644
> --- a/arch/mips/kernel/unaligned.c
> +++ b/arch/mips/kernel/unaligned.c
> @@ -885,7 +885,7 @@ static void emulate_load_store_insn(struct pt_regs *regs,
>  {
>  	union mips_instruction insn;
>  	unsigned long value;
> -	unsigned int res;
> +	unsigned int res, preempted;
>  	unsigned long origpc;
>  	unsigned long orig31;
>  	void __user *fault_addr = NULL;
> @@ -1226,27 +1226,36 @@ static void emulate_load_store_insn(struct pt_regs *regs,
>  			if (!access_ok(VERIFY_READ, addr, sizeof(*fpr)))
>  				goto sigbus;
>  
> -			/*
> -			 * Disable preemption to avoid a race between copying
> -			 * state from userland, migrating to another CPU and
> -			 * updating the hardware vector register below.
> -			 */
> -			preempt_disable();
> -
> -			res = __copy_from_user_inatomic(fpr, addr,
> -							sizeof(*fpr));
> -			if (res)
> -				goto fault;
> -
> -			/*
> -			 * Update the hardware register if it is in use by the
> -			 * task in this quantum, in order to avoid having to
> -			 * save & restore the whole vector context.
> -			 */
> -			if (test_thread_flag(TIF_USEDMSA))
> -				write_msa_wr(wd, fpr, df);
> +			do {
> +				/*
> +				 * If we have live MSA context keep track of
> +				 * whether we get preempted in order to avoid
> +				 * the register context we load being clobbered
> +				 * by the live context as it's saved during
> +				 * preemption. If we don't have live context
> +				 * then it can't be saved to clobber the value
> +				 * we load.
> +				 */
> +				preempted = test_thread_flag(TIF_USEDMSA);
> +
> +				res = __copy_from_user_inatomic(fpr, addr,
> +								sizeof(*fpr));
> +				if (res)
> +					goto fault;
>  
> -			preempt_enable();
> +				/*
> +				 * Update the hardware register if it is in use
> +				 * by the task in this quantum, in order to
> +				 * avoid having to save & restore the whole
> +				 * vector context.
> +				 */
> +				preempt_disable();
> +				if (test_thread_flag(TIF_USEDMSA)) {
> +					write_msa_wr(wd, fpr, df);
> +					preempted = 0;
> +				}
> +				preempt_enable();
> +			} while (preempted);
>  			break;
>  
>  		case msa_st_op:
> -- 
> 2.7.0
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]