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@xxxxxxxxxx>
> Signed-off-by: Paul Burton <paul.burton@xxxxxxxxxx>
> Cc: stable <stable@xxxxxxxxxxxxxxx> # v4.3
Reviewed-by: James Hogan <james.hogan@xxxxxxxxxx>
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:
signature.asc
Description: Digital signature